On Mon, Aug 22, 2022 at 08:10:10AM -0700, Jacob Champion wrote: > otherwise I'll sit tight.
So am I. I have done an extra round of checks around the serialization/deserialization logic where I put some elog()'s to look at the output passed down with some workers and a couple of auth methods, and after an indentation and some comment polishing I finish with the attached. There was one thing that annoyed me with the patch, though, as of the lack of initialization of MyClientConnectionInfo at backend startup, as we may finish by not calling set_authn() to fill in some of its data, so I have placed an extra memset(0) in InitProcessGlobals() (note that Port does a calloc() much earlier than that, but I think that we don't really want to do more in such code paths, especially for the parallelized client information). I have written a commit message, while on it. Does that look fine to you? -- Michael
From 73c28e17f1c77af134dc117500226c201099499b Mon Sep 17 00:00:00 2001 From: Michael Paquier <mich...@paquier.xyz> Date: Tue, 23 Aug 2022 11:23:55 +0900 Subject: [PATCH v4] Allow parallel workers to retrieve some data from Port This commit moves authn_id into a new global structure called ClientConnectionInfo (mapping to a MyClientConnectionInfo for each backend) which is intended to hold all the client information that should be shared between the backend and any of its parallel workers, access for extensions and triggers being the primary use case. There is no need to push all the data of Port to the workers, and authn_id is quite a generic concept so using a separate structure provides the best balance (the name of the structure has been suggested by Robert Haas). While on it, and per discussion as this would be useful for a potential SYSTEM_USER that can be accessed through parallel workers, a second field is added for the authentication method, copied directly from Port. ClientConnectionInfo is serialized and restored using a new parallel key and a structure tracks the length of the authn_id, making the addition of more fields straight-forward. Author: Jocob Champion Reviewed-by: Bertrand Drouvot, Stephen Frost, Robert Haas, Tom Lane, Michael Paquier, Julien Rouhaud Discussion: https://postgr.es/m/793d990837ae5c06a558d58d62de9378ab525d83.ca...@vmware.com --- src/include/libpq/libpq-be.h | 45 +++++++++---- src/include/miscadmin.h | 4 ++ src/backend/access/transam/parallel.c | 19 +++++- src/backend/libpq/auth.c | 23 ++++--- src/backend/postmaster/postmaster.c | 6 +- src/backend/utils/init/miscinit.c | 93 +++++++++++++++++++++++++++ src/tools/pgindent/typedefs.list | 2 + 7 files changed, 166 insertions(+), 26 deletions(-) diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h index 32d3a4b085..d3c8c83e51 100644 --- a/src/include/libpq/libpq-be.h +++ b/src/include/libpq/libpq-be.h @@ -88,6 +88,37 @@ typedef struct } pg_gssinfo; #endif +/* + * ClientConnectionInfo includes the fields describing the client connection + * that are copied over to parallel workers as nothing from Port does that. + * The same rules apply for allocations here as for Port (everything must be + * malloc'd or palloc'd in TopMemoryContext). + * + * If you add a struct member here, remember to also handle serialization in + * SerializeClientConnectionInfo() and co. + */ +typedef struct ClientConnectionInfo +{ + /* + * Authenticated identity. The meaning of this identifier is dependent on + * auth_method; it is the identity (if any) that the user presented during + * the authentication cycle, before they were assigned a database role. + * (It is effectively the "SYSTEM-USERNAME" of a pg_ident usermap -- + * though the exact string in use may be different, depending on pg_hba + * options.) + * + * authn_id is NULL if the user has not actually been authenticated, for + * example if the "trust" auth method is in use. + */ + const char *authn_id; + + /* + * The HBA method that determined the above authn_id. This only has + * meaning if authn_id is not NULL; otherwise it's undefined. + */ + UserAuth auth_method; +} ClientConnectionInfo; + /* * This is used by the postmaster in its communication with frontends. It * contains all state information needed during this communication before the @@ -148,19 +179,6 @@ typedef struct Port */ HbaLine *hba; - /* - * Authenticated identity. The meaning of this identifier is dependent on - * hba->auth_method; it is the identity (if any) that the user presented - * during the authentication cycle, before they were assigned a database - * role. (It is effectively the "SYSTEM-USERNAME" of a pg_ident usermap - * -- though the exact string in use may be different, depending on pg_hba - * options.) - * - * authn_id is NULL if the user has not actually been authenticated, for - * example if the "trust" auth method is in use. - */ - const char *authn_id; - /* * TCP keepalive and user timeout settings. * @@ -317,6 +335,7 @@ extern ssize_t be_gssapi_write(Port *port, void *ptr, size_t len); #endif /* ENABLE_GSS */ extern PGDLLIMPORT ProtocolVersion FrontendProtocol; +extern PGDLLIMPORT ClientConnectionInfo MyClientConnectionInfo; /* TCP keepalives configuration. These are no-ops on an AF_UNIX socket. */ diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h index 067b729d5a..0a88e70efe 100644 --- a/src/include/miscadmin.h +++ b/src/include/miscadmin.h @@ -481,6 +481,10 @@ extern bool has_rolreplication(Oid roleid); typedef void (*shmem_request_hook_type) (void); extern PGDLLIMPORT shmem_request_hook_type shmem_request_hook; +extern Size EstimateClientConnectionInfoSpace(void); +extern void SerializeClientConnectionInfo(Size maxsize, char *start_address); +extern void RestoreClientConnectionInfo(char *conninfo); + /* in executor/nodeHash.c */ extern size_t get_hash_memory_limit(void); diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c index df0cd77558..bc93101ff7 100644 --- a/src/backend/access/transam/parallel.c +++ b/src/backend/access/transam/parallel.c @@ -76,6 +76,7 @@ #define PARALLEL_KEY_REINDEX_STATE UINT64CONST(0xFFFFFFFFFFFF000C) #define PARALLEL_KEY_RELMAPPER_STATE UINT64CONST(0xFFFFFFFFFFFF000D) #define PARALLEL_KEY_UNCOMMITTEDENUMS UINT64CONST(0xFFFFFFFFFFFF000E) +#define PARALLEL_KEY_CLIENTCONNINFO UINT64CONST(0xFFFFFFFFFFFF000F) /* Fixed-size parallel state. */ typedef struct FixedParallelState @@ -212,6 +213,7 @@ InitializeParallelDSM(ParallelContext *pcxt) Size reindexlen = 0; Size relmapperlen = 0; Size uncommittedenumslen = 0; + Size clientconninfolen = 0; Size segsize = 0; int i; FixedParallelState *fps; @@ -272,8 +274,10 @@ InitializeParallelDSM(ParallelContext *pcxt) shm_toc_estimate_chunk(&pcxt->estimator, relmapperlen); uncommittedenumslen = EstimateUncommittedEnumsSpace(); shm_toc_estimate_chunk(&pcxt->estimator, uncommittedenumslen); + clientconninfolen = EstimateClientConnectionInfoSpace(); + shm_toc_estimate_chunk(&pcxt->estimator, clientconninfolen); /* If you add more chunks here, you probably need to add keys. */ - shm_toc_estimate_keys(&pcxt->estimator, 11); + shm_toc_estimate_keys(&pcxt->estimator, 12); /* Estimate space need for error queues. */ StaticAssertStmt(BUFFERALIGN(PARALLEL_ERROR_QUEUE_SIZE) == @@ -352,6 +356,7 @@ InitializeParallelDSM(ParallelContext *pcxt) char *session_dsm_handle_space; char *entrypointstate; char *uncommittedenumsspace; + char *clientconninfospace; Size lnamelen; /* Serialize shared libraries we have loaded. */ @@ -422,6 +427,12 @@ InitializeParallelDSM(ParallelContext *pcxt) shm_toc_insert(pcxt->toc, PARALLEL_KEY_UNCOMMITTEDENUMS, uncommittedenumsspace); + /* Serialize our ClientConnectionInfo. */ + clientconninfospace = shm_toc_allocate(pcxt->toc, clientconninfolen); + SerializeClientConnectionInfo(clientconninfolen, clientconninfospace); + shm_toc_insert(pcxt->toc, PARALLEL_KEY_CLIENTCONNINFO, + clientconninfospace); + /* Allocate space for worker information. */ pcxt->worker = palloc0(sizeof(ParallelWorkerInfo) * pcxt->nworkers); @@ -1270,6 +1281,7 @@ ParallelWorkerMain(Datum main_arg) char *reindexspace; char *relmapperspace; char *uncommittedenumsspace; + char *clientconninfospace; StringInfoData msgbuf; char *session_dsm_handle_space; Snapshot tsnapshot; @@ -1479,6 +1491,11 @@ ParallelWorkerMain(Datum main_arg) false); RestoreUncommittedEnums(uncommittedenumsspace); + /* Restore the ClientConnectionInfo. */ + clientconninfospace = shm_toc_lookup(toc, PARALLEL_KEY_CLIENTCONNINFO, + false); + RestoreClientConnectionInfo(clientconninfospace); + /* Attach to the leader's serializable transaction, if SERIALIZABLE. */ AttachSerializableXact(fps->serializable_xact_handle); diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c index 1545ff9f16..2e7330f7bc 100644 --- a/src/backend/libpq/auth.c +++ b/src/backend/libpq/auth.c @@ -333,23 +333,23 @@ auth_failed(Port *port, int status, const char *logdetail) /* * Sets the authenticated identity for the current user. The provided string - * will be copied into the TopMemoryContext. The ID will be logged if - * log_connections is enabled. + * will be stored into MyClientConnectionInfo, alongside the current HBA + * method in use. The ID will be logged if log_connections is enabled. * * Auth methods should call this routine exactly once, as soon as the user is * successfully authenticated, even if they have reasons to know that * authorization will fail later. * * The provided string will be copied into TopMemoryContext, to match the - * lifetime of the Port, so it is safe to pass a string that is managed by an - * external library. + * lifetime of MyClientConnectionInfo, so it is safe to pass a string that is + * managed by an external library. */ static void set_authn_id(Port *port, const char *id) { Assert(id); - if (port->authn_id) + if (MyClientConnectionInfo.authn_id) { /* * An existing authn_id should never be overwritten; that means two @@ -360,18 +360,20 @@ set_authn_id(Port *port, const char *id) ereport(FATAL, (errmsg("authentication identifier set more than once"), errdetail_log("previous identifier: \"%s\"; new identifier: \"%s\"", - port->authn_id, id))); + MyClientConnectionInfo.authn_id, id))); } - port->authn_id = MemoryContextStrdup(TopMemoryContext, id); + MyClientConnectionInfo.authn_id = MemoryContextStrdup(TopMemoryContext, id); + MyClientConnectionInfo.auth_method = port->hba->auth_method; if (Log_connections) { ereport(LOG, errmsg("connection authenticated: identity=\"%s\" method=%s " "(%s:%d)", - port->authn_id, hba_authname(port->hba->auth_method), HbaFileName, - port->hba->linenumber)); + MyClientConnectionInfo.authn_id, + hba_authname(MyClientConnectionInfo.auth_method), + HbaFileName, port->hba->linenumber)); } } @@ -1907,7 +1909,8 @@ auth_peer(hbaPort *port) */ set_authn_id(port, pw->pw_name); - ret = check_usermap(port->hba->usermap, port->user_name, port->authn_id, false); + ret = check_usermap(port->hba->usermap, port->user_name, + MyClientConnectionInfo.authn_id, false); return ret; #else diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c index 8a038d1b2a..1fd70fba83 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -2678,9 +2678,10 @@ ClosePostmasterPorts(bool am_syslogger) /* - * InitProcessGlobals -- set MyProcPid, MyStartTime[stamp], random seeds + * InitProcessGlobals -- set some global variables for this process * - * Called early in the postmaster and every backend. + * This sets MyProcPid, MyStartTime[stamp], random seeds, and initializes + * MyClientConnectionInfo. Called early in the postmaster and every backend. */ void InitProcessGlobals(void) @@ -2688,6 +2689,7 @@ InitProcessGlobals(void) MyProcPid = getpid(); MyStartTimestamp = GetCurrentTimestamp(); MyStartTime = timestamptz_to_time_t(MyStartTimestamp); + memset(&MyClientConnectionInfo, 0, sizeof(MyClientConnectionInfo)); /* * Set a different global seed in every process. We want something diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c index bd973ba613..2e02d681e6 100644 --- a/src/backend/utils/init/miscinit.c +++ b/src/backend/utils/init/miscinit.c @@ -931,6 +931,99 @@ GetUserNameFromId(Oid roleid, bool noerr) return result; } +/* ------------------------------------------------------------------------ + * Client connection state shared with parallel workers + * + * ClientConnectionInfo contains pieces of information about the client that + * need to be synced to parallel workers when they initialize. + *------------------------------------------------------------------------- + */ + +ClientConnectionInfo MyClientConnectionInfo; + +/* + * Intermediate representation of ClientConnectionInfo for easier + * serialization. Variable-length fields are allocated right after this + * header. + */ +typedef struct SerializedClientConnectionInfo +{ + int32 authn_id_len; /* strlen(authn_id), or -1 if NULL */ + UserAuth auth_method; +} SerializedClientConnectionInfo; + +/* + * Calculate the space needed to serialize MyClientConnectionInfo. + */ +Size +EstimateClientConnectionInfoSpace(void) +{ + Size size = 0; + + size = add_size(size, sizeof(SerializedClientConnectionInfo)); + + if (MyClientConnectionInfo.authn_id) + size = add_size(size, strlen(MyClientConnectionInfo.authn_id) + 1); + + return size; +} + +/* + * Serialize MyClientConnectionInfo for use by parallel workers. + */ +void +SerializeClientConnectionInfo(Size maxsize, char *start_address) +{ + SerializedClientConnectionInfo serialized = {0}; + + serialized.authn_id_len = -1; + serialized.auth_method = MyClientConnectionInfo.auth_method; + + if (MyClientConnectionInfo.authn_id) + serialized.authn_id_len = strlen(MyClientConnectionInfo.authn_id); + + /* Copy serialized representation to buffer */ + Assert(maxsize >= sizeof(serialized)); + memcpy(start_address, &serialized, sizeof(serialized)); + + maxsize -= sizeof(serialized); + start_address += sizeof(serialized); + + /* Copy authn_id into the space after the struct */ + if (serialized.authn_id_len >= 0) + { + Assert(maxsize >= (serialized.authn_id_len + 1)); + memcpy(start_address, + MyClientConnectionInfo.authn_id, + /* include the NULL terminator to ease deserialization */ + serialized.authn_id_len + 1); + } +} + +/* + * Restore MyClientConnectionInfo from its serialized representation. + */ +void +RestoreClientConnectionInfo(char *conninfo) +{ + SerializedClientConnectionInfo serialized; + + memcpy(&serialized, conninfo, sizeof(serialized)); + + /* Copy the fields back into place */ + MyClientConnectionInfo.authn_id = NULL; + MyClientConnectionInfo.auth_method = serialized.auth_method; + + if (serialized.authn_id_len >= 0) + { + char *authn_id; + + authn_id = conninfo + sizeof(serialized); + MyClientConnectionInfo.authn_id = MemoryContextStrdup(TopMemoryContext, + authn_id); + } +} + /*------------------------------------------------------------------------- * Interlock-file support diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 35c9f1efce..a4a4e356e5 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -373,6 +373,7 @@ CkptTsStatus ClientAuthentication_hook_type ClientCertMode ClientCertName +ClientConnectionInfo ClientData ClonePtrType ClosePortalStmt @@ -2455,6 +2456,7 @@ SerCommitSeqNo SerialControl SerializableXactHandle SerializedActiveRelMaps +SerializedClientConnectionInfo SerializedRanges SerializedReindexState SerializedSnapshotData -- 2.37.2
signature.asc
Description: PGP signature
