On Tue, Aug 16, 2022 at 2:02 AM Drouvot, Bertrand <bdrou...@amazon.com> wrote:
> On 8/14/22 11:57 AM, Michael Paquier wrote:
> > One thing was itching me about the serialization and
> > deserialization logic though: could it be more readable if we used an
> > intermediate structure to store the length of the serialized strings?
> > We use this approach in other areas, like for the snapshot data in
> > snapmgr.c. This would handle the case of an empty and NULL string, by
> > storing -1 as length for NULL and >= 0 for the string length if there
> > is something set, while making the addition of more fields a
> > no-brainer.
>
> I think that's a good idea and I think that would be more readable (as
> compare to storing a "hint" in the first byte).
Sounds good. v3, attached, should make the requested changes:
- declare `struct ClientConnectionInfo`
- use an intermediate serialization struct
- switch to length-"prefixing" for the string
I do like the way this reads compared to before.
Thanks,
--Jacob
commit 753c46352adc967a903a60ea65a3068252d685e6
Author: Jacob Champion <jchamp...@timescale.com>
Date: Tue Aug 16 09:14:58 2022 -0700
squash! Allow parallel workers to read authn_id
Per review,
- add an intermediate struct for serialization,
- switch to length-prefixing for the authn_id string, and
- make sure `struct ClientConnectionInfo` is declared for use elsewhere.
diff --git a/src/backend/utils/init/miscinit.c
b/src/backend/utils/init/miscinit.c
index 155ba92c67..58772d0a4a 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -943,19 +943,29 @@ GetUserNameFromId(Oid roleid, bool noerr)
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 = 1;
+ Size size = 0;
+
+ size = add_size(size, sizeof(SerializedClientConnectionInfo));
if (MyClientConnectionInfo.authn_id)
size = add_size(size, strlen(MyClientConnectionInfo.authn_id) +
1);
- size = add_size(size, sizeof(UserAuth));
-
return size;
}
@@ -965,32 +975,29 @@ EstimateClientConnectionInfoSpace(void)
void
SerializeClientConnectionInfo(Size maxsize, char *start_address)
{
- /*
- * First byte is an indication of whether or not authn_id has been set
to
- * non-NULL, to differentiate that case from the empty string.
- */
- Assert(maxsize > 0);
- start_address[0] = MyClientConnectionInfo.authn_id ? 1 : 0;
- start_address++;
- maxsize--;
+ SerializedClientConnectionInfo serialized = {0};
+
+ serialized.authn_id_len = -1;
+ serialized.auth_method = MyClientConnectionInfo.auth_method;
if (MyClientConnectionInfo.authn_id)
- {
- Size len;
+ serialized.authn_id_len =
strlen(MyClientConnectionInfo.authn_id);
- len = strlcpy(start_address, MyClientConnectionInfo.authn_id,
maxsize) + 1;
- Assert(len <= maxsize);
- maxsize -= len;
- start_address += len;
- }
+ /* Copy serialized representation to buffer */
+ Assert(maxsize >= sizeof(serialized));
+ memcpy(start_address, &serialized, sizeof(serialized));
- {
- UserAuth *auth_method = (UserAuth*) start_address;
+ maxsize -= sizeof(serialized);
+ start_address += sizeof(serialized);
- Assert(sizeof(*auth_method) <= maxsize);
- *auth_method = MyClientConnectionInfo.auth_method;
- maxsize -= sizeof(*auth_method);
- start_address += sizeof(*auth_method);
+ /* 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);
}
}
@@ -1000,25 +1007,19 @@ SerializeClientConnectionInfo(Size maxsize, char
*start_address)
void
RestoreClientConnectionInfo(char *conninfo)
{
- if (conninfo[0] == 0)
- {
- MyClientConnectionInfo.authn_id = NULL;
- conninfo++;
- }
- else
- {
- conninfo++;
- MyClientConnectionInfo.authn_id =
MemoryContextStrdup(TopMemoryContext,
-
conninfo);
- conninfo += strlen(conninfo) + 1;
- }
+ SerializedClientConnectionInfo serialized;
+ char *authn_id;
- {
- UserAuth *auth_method = (UserAuth*) conninfo;
+ memcpy(&serialized, conninfo, sizeof(serialized));
+ authn_id = conninfo + sizeof(serialized);
- MyClientConnectionInfo.auth_method = *auth_method;
- conninfo += sizeof(*auth_method);
- }
+ /* Copy the fields back into place. */
+ MyClientConnectionInfo.authn_id = NULL;
+ MyClientConnectionInfo.auth_method = serialized.auth_method;
+
+ if (serialized.authn_id_len >= 0)
+ MyClientConnectionInfo.authn_id =
MemoryContextStrdup(TopMemoryContext,
+
authn_id);
}
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 0643733765..84a6bdea6f 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -107,7 +107,7 @@ typedef struct
* If you add a struct member here, remember to also handle serialization in
* SerializeClientConnectionInfo() et al.
*/
-typedef struct
+typedef struct ClientConnectionInfo
{
/*
* Authenticated identity. The meaning of this identifier is dependent
on
From 2eea3ef097bbeee5323f78c827e56b42480b5c81 Mon Sep 17 00:00:00 2001
From: Jacob Champion <pchamp...@vmware.com>
Date: Wed, 23 Mar 2022 15:07:05 -0700
Subject: [PATCH v3 1/3] Allow parallel workers to read authn_id
Move authn_id into a new global, MyClientConnectionInfo, which is
intended to hold all the client information that needs to be shared
between the backend and any parallel workers. MyClientConnectionInfo is
serialized and restored using a new parallel key.
Additionally, make a copy of hba->auth_method in ClientConnectionInfo
when set_authn_id() is called, for use by SYSTEM_USER.
---
src/backend/access/transam/parallel.c | 19 +++++-
src/backend/libpq/auth.c | 25 ++++----
src/backend/utils/init/miscinit.c | 91 +++++++++++++++++++++++++++
src/include/libpq/libpq-be.h | 45 +++++++++----
src/include/miscadmin.h | 4 ++
5 files changed, 159 insertions(+), 25 deletions(-)
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 2d9ab7edce..9113f04189 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -333,24 +333,24 @@ 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.
+ * Sets the authenticated identity for the current user. The provided string
+ * 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
@@ -361,18 +361,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));
}
}
@@ -1908,7 +1910,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/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index eb43b2c5e5..58772d0a4a 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -931,6 +931,97 @@ GetUserNameFromId(Oid roleid, bool noerr)
return result;
}
+/* ------------------------------------------------------------------------
+ * Parallel connection state
+ *
+ * ClientConnectionInfo contains pieces of information about the client that
+ * need to be synced to parallel workers when they initialize. Over time, this
+ * list will probably grow, and may subsume some of the "user state" variables
+ * above.
+ *-------------------------------------------------------------------------
+ */
+
+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;
+ char *authn_id;
+
+ memcpy(&serialized, conninfo, sizeof(serialized));
+ authn_id = 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)
+ MyClientConnectionInfo.authn_id = MemoryContextStrdup(TopMemoryContext,
+ authn_id);
+}
+
/*-------------------------------------------------------------------------
* Interlock-file support
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 90c20da22b..84a6bdea6f 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -98,6 +98,37 @@ typedef struct
} pg_gssinfo;
#endif
+/*
+ * Fields describing the client connection, that also need to be copied over to
+ * parallel workers, go into the ClientConnectionInfo rather than Port. The same
+ * rules apply for allocations here as for Port (must be malloc'd or palloc'd in
+ * TopMemoryContext).
+ *
+ * If you add a struct member here, remember to also handle serialization in
+ * SerializeClientConnectionInfo() et al.
+ */
+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
@@ -158,19 +189,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.
*
@@ -327,6 +345,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..3e9297e399 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 *procinfo);
+
/* in executor/nodeHash.c */
extern size_t get_hash_memory_limit(void);
--
2.25.1
