From 8d64af82b377d7d48d6efe77717ccec4cc0dbdfa Mon Sep 17 00:00:00 2001 From: rahulb-yb Date: Mon, 29 Apr 2024 04:21:09 +0000 Subject: [PATCH] [#21505] YSQL: Use role_oid instead of role GUC variable in YSQL Connection 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 --- .../yb/ysqlconnmgr/TestSessionParameters.java | 4 +- src/odyssey/sources/backend.c | 19 +++++++-- src/odyssey/sources/client.h | 6 +++ src/odyssey/sources/frontend.c | 19 +++++++++ src/odyssey/sources/relay.h | 16 ++++++++ src/odyssey/sources/status.h | 4 ++ src/odyssey/third_party/kiwi/kiwi/fe_read.h | 3 +- src/odyssey/third_party/kiwi/kiwi/header.h | 3 ++ src/odyssey/third_party/kiwi/kiwi/var.h | 13 +++++-- src/postgres/src/backend/utils/misc/guc.c | 39 +++++++++++++++++++ 10 files changed, 117 insertions(+), 9 deletions(-) diff --git a/java/yb-ysql-conn-mgr/src/test/java/org/yb/ysqlconnmgr/TestSessionParameters.java b/java/yb-ysql-conn-mgr/src/test/java/org/yb/ysqlconnmgr/TestSessionParameters.java index 869e405e6d46..f24d0834149d 100644 --- a/java/yb-ysql-conn-mgr/src/test/java/org/yb/ysqlconnmgr/TestSessionParameters.java +++ b/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; diff --git a/src/odyssey/sources/backend.c b/src/odyssey/sources/backend.c index 585249af9a6b..97a00dcfb8bf 100644 --- a/src/odyssey/sources/backend.c +++ b/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, diff --git a/src/odyssey/sources/client.h b/src/odyssey/sources/client.h index 60e9d31c620e..8ea6d239c76e 100644 --- a/src/odyssey/sources/client.h +++ b/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); } diff --git a/src/odyssey/sources/frontend.c b/src/odyssey/sources/frontend.c index e77a35fb36d7..891368e846d2 100644 --- a/src/odyssey/sources/frontend.c +++ b/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, diff --git a/src/odyssey/sources/relay.h b/src/odyssey/sources/relay.h index 7dc3de6694f5..8a6821b358f2 100644 --- a/src/odyssey/sources/relay.h +++ b/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 */ diff --git a/src/odyssey/sources/status.h b/src/odyssey/sources/status.h index 6c63e29fe351..12bb4812a8c8 100644 --- a/src/odyssey/sources/status.h +++ b/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 \ diff --git a/src/odyssey/third_party/kiwi/kiwi/fe_read.h b/src/odyssey/third_party/kiwi/kiwi/fe_read.h index 3e84a116bab4..5d20ed815d19 100644 --- a/src/odyssey/third_party/kiwi/kiwi/fe_read.h +++ b/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); diff --git a/src/odyssey/third_party/kiwi/kiwi/header.h b/src/odyssey/third_party/kiwi/kiwi/header.h index f897082eb66f..e1b870b47ee3 100644 --- a/src/odyssey/third_party/kiwi/kiwi/header.h +++ b/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"; } diff --git a/src/odyssey/third_party/kiwi/kiwi/var.h b/src/odyssey/third_party/kiwi/kiwi/var.h index 9a5c3e33d39c..864f260b0fe8 100644 --- a/src/odyssey/third_party/kiwi/kiwi/var.h +++ b/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); diff --git a/src/postgres/src/backend/utils/misc/guc.c b/src/postgres/src/backend/utils/misc/guc.c index 0a90cbbad491..0d0cc2f7fa8d 100644 --- a/src/postgres/src/backend/utils/misc/guc.c +++ b/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)