[#21505] YSQL: Use role_oid instead of role GUC variable in YSQL Conn…

…ection Manager

Summary:
YSQL Connection Manager tracks GUC variables that were modified on a logical connection by maintaining a list of modified variables (as per ParameterStatus packets received). The `role` GUC variable can be modified through SET/RESET statements, but roles can also be renamed as such: `ALTER ROLE role_1 RENAME TO role_2`. Since Connection Manager would have `role_1` being stored as the role name, this leads to incorrect behaviour while roles are renamed mid-session.

To resolve this issue, we can track the role GUC variable using its OID instead. List of changes made to facilitate this provision:
- Whenever `role` ParameterStatus is being sent by the backend, additionally send a `role_oid` ParameterStatus packet (sent via custom RoleOidParameterStatus packet)
- Whenever the backend process receives a query like: `SET role_oid = 12345`, map the OID received to its role name (if valid, else throw invalid OID error) and accordingly set the `role` GUC variable
- Ignore tracking of `role` ParameterStatus packets on Connection Manager, simply forward them to client
- Whenever ConnectionManager receives a RoleOidParameterStatus packet, process it to store relevant details and deliberately skip relaying of the packet to client

Jira: DB-10389

Test Plan: ./yb_build.sh --enable-ysql-conn-mgr-test --java-test org.yb.pgsql.TestPgAuthorization#testRoleRenamingMidSession

Reviewers: mkumar, nkumar, jason

Reviewed By: nkumar

Subscribers: yql

Differential Revision: https://phorge.dev.yugabyte.com/D34316

java/yb-ysql-conn-mgr/src/test/java/org/yb/ysqlconnmgr/TestSessionParameters.java

@@ -123,7 +123,7 @@ private static enum ExceptionType {
     new SessionParameter("transaction_isolation", "read committed",
       "read committed", "serializable",
       new ExceptionType[] { ExceptionType.YB_CACHE_MAPPING, ExceptionType.TRANSACTION_SKIP,
-        ExceptionType.SET_UNIQUE }),
+        ExceptionType.SET_UNIQUE, ExceptionType.INVALID_STARTUP }),
     new SessionParameter("geqo", "on", "off",
       new ExceptionType[] {}),
     new SessionParameter("statement_timeout", "0", "999",
@@ -465,6 +465,8 @@ public void testStartupParameters() throws Exception {
       String parameterName = sp.parameterName;
       String expectedValue = sp.expectedValue;
 
+      // (GH#22248) (rbarigidad) Startup parameters with spaces are not parsed
+      // correctly, they are ignored for now as well.
       if (sp.exceptionSet.contains(ExceptionType.INVALID_STARTUP))
         continue;
 

src/odyssey/sources/backend.c

@@ -78,6 +78,13 @@ void od_backend_error(od_server_t *server, char *context, char *data,
 			 "DETAIL: %s", error.detail);
 	}
 
+	/* catch and store error to be forwarded later if we are in deploy phase */
+	if (od_server_in_deploy(server)) {
+		od_client_t* client = (od_client_t*) (server->client);
+		client->deploy_err = (kiwi_fe_error_t *) malloc(sizeof(kiwi_fe_error_t));
+		kiwi_fe_read_error(data, size, client->deploy_err);
+	}
+
 	if (error.hint) {
 		od_error(&instance->logger, context, server->client, server,
 			 "HINT: %s", error.hint);
@@ -210,6 +217,8 @@ static inline int od_backend_startup(od_server_t *server,
 				return -1;
 			}
 			break;
+		/* fallthrough */
+		case YB_ROLE_OID_PARAMETER_STATUS:
 		case KIWI_BE_PARAMETER_STATUS: {
 			char *name;
 			uint32_t name_len;
@@ -689,6 +698,10 @@ int od_backend_update_parameter(od_server_t *server, char *context, char *data,
 		return -1;
 	}
 
+	/* ignore caching of role, only store role_oid */
+	if (strcmp("role", name) == 0)
+		return 0;
+
 	/* update server only or client and server parameter */
 	od_debug(&instance->logger, context, client, server, "%.*s = %.*s",
 		 name_len, name, value_len, value);
@@ -700,9 +713,9 @@ int od_backend_update_parameter(od_server_t *server, char *context, char *data,
 		kiwi_vars_update_both(&client->vars, &server->vars, name,
 				      name_len, value, value_len);
 
-		/* reset role whenever session_authorization is changed by the user */
+		/* reset role oid whenever session_authorization is changed by the user */
 		if (strcmp(name, "session_authorization") == 0)
-			kiwi_vars_update_both(&client->vars, &server->vars, "role", 5, "none", 5);
+			kiwi_vars_update_both(&client->vars, &server->vars, "role_oid", 9, "-1", 3);
 	}
 
 	return 0;
@@ -729,7 +742,7 @@ int od_backend_ready_wait(od_server_t *server, char *context, int count,
 		od_debug(&instance->logger, context, server->client, server,
 			 "%s", kiwi_be_type_to_string(type));
 
-		if (type == KIWI_BE_PARAMETER_STATUS) {
+		if (type == KIWI_BE_PARAMETER_STATUS || type == YB_ROLE_OID_PARAMETER_STATUS) {
 			/* update server parameter */
 			int rc;
 			rc = od_backend_update_parameter(server, context,

src/odyssey/sources/client.h

@@ -75,6 +75,7 @@ struct od_client {
 
 	uint64_t client_id;
 	int64_t yb_db_oid;
+	kiwi_fe_error_t *deploy_err;
 };
 
 static const size_t OD_CLIENT_DEFAULT_HASHMAP_SZ = 420;
@@ -128,6 +129,7 @@ static inline void od_client_init(od_client_t *client)
 	client->prep_stmt_ids = NULL;
 	client->client_id = 0;
 	client->yb_db_oid = -1;
+	client->deploy_err = NULL;
 }
 
 static inline od_client_t *od_client_allocate(void)
@@ -154,6 +156,10 @@ static inline void od_client_free(od_client_t *client)
 		free(client->vars.vars);
 		client->vars.vars = NULL;
 	}
+	if (client->deploy_err) {
+		free(client->deploy_err);
+		client->deploy_err = NULL;
+	}
 	free(client);
 }
 

src/odyssey/sources/frontend.c

@@ -326,6 +326,9 @@ od_frontend_attach_and_deploy(od_client_t *client, char *context)
 	if (rc == -1)
 		return OD_ESERVER_WRITE;
 
+	if (client->deploy_err)
+		return YB_OD_DEPLOY_ERR;
+
 	/* set number of replies to discard */
 	client->server->deploy_sync = rc;
 
@@ -798,6 +801,8 @@ static od_frontend_status_t od_frontend_remote_server(od_relay_t *relay,
 	case KIWI_BE_ERROR_RESPONSE:
 		od_backend_error(server, "main", data, size);
 		break;
+	/* fallthrough */
+	case YB_ROLE_OID_PARAMETER_STATUS:
 	case KIWI_BE_PARAMETER_STATUS:
 		rc = od_backend_update_parameter(server, "main", data, size, 0);
 		if (rc == -1)
@@ -849,6 +854,10 @@ static od_frontend_status_t od_frontend_remote_server(od_relay_t *relay,
 	if (yb_is_route_invalid(server->route))
 		return OD_ESERVER_READ;
 
+	/* error was caught during the deploy phase, return and forward to client */
+	if (client->deploy_err)
+		return YB_OD_DEPLOY_ERR;
+
 	/* discard replies during configuration deploy */
 	if (is_deploy)
 		return OD_SKIP;
@@ -2017,6 +2026,16 @@ static void od_frontend_cleanup(od_client_t *client, char *context,
 			 od_frontend_status_to_str(status), (uint32)status);
 		od_router_close(router, client);
 		break;
+	case YB_OD_DEPLOY_ERR:
+		/* close both client and server connection */
+		/* backend connection is not in a usable state */ 
+		od_error(&instance->logger, context, client, server,
+				"deploy error: %s, status %s", client->deploy_err,
+				od_frontend_status_to_str(status));
+		od_frontend_fatal(client, client->deploy_err->code, client->deploy_err->message);
+		/* close backend connection */
+		od_router_close(router, client);
+		break;
 	default:
 		od_error(
 			&instance->logger, context, client, server,

src/odyssey/sources/relay.h

@@ -143,6 +143,16 @@ static inline int od_relay_stop(od_relay_t *relay)
 	return 0;
 }
 
+// Skip relaying custom packets back to the client
+static inline int yb_od_skip_relay(char *data)
+{
+	kiwi_header_t *header;
+	header = (kiwi_header_t *)data;
+	if (header->type == YB_ROLE_OID_PARAMETER_STATUS)
+		return 1;
+	return 0;
+}
+
 static inline int od_relay_full_packet_required(char *data)
 {
 	kiwi_header_t *header;
@@ -193,6 +203,12 @@ static inline od_frontend_status_t od_relay_on_packet(od_relay_t *relay,
 	od_frontend_status_t status;
 	status = relay->on_packet(relay, data, size);
 
+	/* skip relaying of custom packets back to the client */
+	if (yb_od_skip_relay(data)) {
+		relay->packet_skip = 1;
+		return OD_OK;
+	}
+
 	switch (status) {
 	case OD_OK:
 		/* fallthrough */

src/odyssey/sources/status.h

@@ -26,6 +26,7 @@ typedef enum {
 	OD_ECLIENT_WRITE,
 	OD_ESYNC_BROKEN,
 	OD_ECATCHUP_TIMEOUT,
+	YB_OD_DEPLOY_ERR,
 } od_frontend_status_t;
 
 static inline char *od_frontend_status_to_str(od_frontend_status_t status)
@@ -67,6 +68,8 @@ static inline char *od_frontend_status_to_str(od_frontend_status_t status)
 		return "OD_ESYNC_BROKEN";
 	case OD_ECATCHUP_TIMEOUT:
 		return "OD_ECATCHUP_TIMEOUT";
+	case YB_OD_DEPLOY_ERR:
+		return "YB_OD_DEPLOY_ERR";
 	}
 	return "UNKNOWN";
 }
@@ -82,6 +85,7 @@ static const od_frontend_status_t od_frontend_status_errs[] = {
 	OD_ECLIENT_READ,
 	OD_ESYNC_BROKEN,
 	OD_ECATCHUP_TIMEOUT,
+	YB_OD_DEPLOY_ERR,
 };
 
 #define OD_FRONTEND_STATUS_ERRORS_TYPES_COUNT \

src/odyssey/third_party/kiwi/kiwi/fe_read.h

@@ -131,7 +131,8 @@ kiwi_fe_read_parameter(char *data, uint32_t size, char **name,
 	int rc = kiwi_read(&len, &data, &size);
 	if (kiwi_unlikely(rc != 0))
 		return -1;
-	if (kiwi_unlikely(header->type != KIWI_BE_PARAMETER_STATUS))
+	if (kiwi_unlikely(header->type != KIWI_BE_PARAMETER_STATUS) &&
+		kiwi_unlikely(header->type != YB_ROLE_OID_PARAMETER_STATUS))
 		return -1;
 	uint32_t pos_size = len;
 	char *pos = kiwi_header_data(header);

src/odyssey/third_party/kiwi/kiwi/header.h

@@ -66,6 +66,7 @@ typedef enum {
 	KIWI_BE_COMPRESSION = 'z',
 	YB_KIWI_BE_FATAL_FOR_LOGICAL_CONNECTION = 'F',
 	YB_OID_DETAILS = 'O',
+	YB_ROLE_OID_PARAMETER_STATUS = 'r',
 } kiwi_be_type_t;
 
 struct kiwi_header {
@@ -168,6 +169,8 @@ static inline char *kiwi_be_type_to_string(int type)
 		return "FatalForLogicalConnection";
 	case YB_OID_DETAILS:
 		return "OidDetails";
+	case YB_ROLE_OID_PARAMETER_STATUS:
+		return "RoleOidParameterStatus";
 	}
 	return "Unknown";
 }

src/odyssey/third_party/kiwi/kiwi/var.h

@@ -219,9 +219,9 @@ static inline void kiwi_vars_init(kiwi_vars_t *vars)
 #else
 	vars->size = 0;
 
-	/* Ensure that role is "cached" after session_authorization. */
+	/* Ensure that role oid is "cached" after session_authorization. */
 	yb_kiwi_var_push(vars, "session_authorization", 22, "default", 8);
-	yb_kiwi_var_push(vars, "role", 5, "none", 5);
+	yb_kiwi_var_push(vars, "role_oid", 9, "-1", 3);
 #endif
 }
 
@@ -366,6 +366,11 @@ __attribute__((hot)) static inline int kiwi_vars_cas(kiwi_vars_t *client,
 
 		if (strcmp(var->name, "compression") == 0)
 			continue;
+
+		/* do not send default value role_oid packet to the server */
+		if ((strcmp(var->name, "role_oid") == 0) && (strcmp(var->value, "-1") == 0))
+			continue;
+
 		kiwi_var_t *server_var;
 		server_var = yb_kiwi_vars_get(server, var->name);
 #endif
@@ -384,8 +389,8 @@ __attribute__((hot)) static inline int kiwi_vars_cas(kiwi_vars_t *client,
 		pos += 1;
 
 		/* Do not enquote the default values for auth related params. */
-		if (!(strcmp(var->name, "role") == 0 && strcmp(var->value, "none") == 0) &&
-			!(strcmp(var->name, "session_authorization") == 0 && strcmp(var->value, "default") == 0)) {
+        if (!(strcmp(var->name, "session_authorization") == 0 &&
+			strcmp(var->value, "default") == 0)) {
 			int quote_len;
 			quote_len =
 				kiwi_enquote(var->value, query + pos, query_len - pos);

src/postgres/src/backend/utils/misc/guc.c

@@ -7014,6 +7014,29 @@ ReportGUCOption(struct config_generic *record)
 		pq_sendstring(&msgbuf, val);
 		pq_endmessage(&msgbuf);
 
+		/* 
+		 * Send the equivalent of a ParameterStatus packet corresponding to role_oid
+		 * back to YSQL Connection Manager.
+		 */
+		if ((strcmp(record->name, "role") == 0) && YbIsClientYsqlConnMgr())
+		{
+			StringInfoData rolebuf;
+
+			pq_beginmessage(&rolebuf, 'r');
+			pq_sendstring(&rolebuf, "role_oid");
+			if (val != NULL && strcmp(val, "none") != 0)
+			{
+				char oid[16];
+				snprintf(oid, 16, "%u", get_role_oid(val, false));
+
+				pq_sendstring(&rolebuf, oid);
+
+			}
+			else
+				pq_sendstring(&rolebuf, "-1");
+			pq_endmessage(&rolebuf);
+		}
+
 		pfree(val);
 	}
 }
@@ -7609,6 +7632,22 @@ set_config_option(const char *name, const char *value,
 	if (source == YSQL_CONN_MGR)
 		Assert(YbIsClientYsqlConnMgr());
 
+	/* 
+	 * role_oid is a provision made for YSQL Connection Manager to handle
+	 * scenarios around "ALTER ROLE RENAME" queries as it only caches the
+	 * previously used role by that client.
+	 */
+	if (strcmp(name, "role_oid") == 0)
+	{
+		Assert(YbIsClientYsqlConnMgr());
+		/* Handle RESET ROLE queries */
+		if (!value || strcmp(value, "0") == 0)
+			return set_config_option("role", NULL, context, source,
+								action, changeVal, elevel, is_reload);
+		return set_config_option("role", GetUserNameFromId(atoi(value), false), context, source,
+								action, changeVal, elevel, is_reload);
+	}
+
 	if (elevel == 0)
 	{
 		if (source == PGC_S_DEFAULT || source == PGC_S_FILE)