From 653b37f2eb3adc429c8365c922a76d3979bde10f Mon Sep 17 00:00:00 2001 From: Bryan Green Date: Sun, 28 Jun 2026 11:40:58 -0500 Subject: [PATCH v2 1/5] Give fmtId()'s temporary buffer thread-local storage getLocalPQExpBuffer() returns a function-scope static buffer, unsafe when fmtId() and fmtQualifiedId() run in multiple threads. On Windows, where pg_dump's parallel workers are threads, this was patched over at runtime by swapping in a TLS-based buffer from ParallelBackupStart(). Mark the default buffer C11 _Thread_local instead. Each thread gets its own, so the Windows TlsAlloc/TlsGetValue workaround goes. With no caller left to override it the getLocalPQExpBuffer indirection is pointless too: drop the function pointer, fold the default into a plain static getLocalPQExpBuffer(), and remove its now-obsolete extern. --- src/bin/pg_dump/parallel.c | 52 ----------------------------- src/fe_utils/string_utils.c | 13 +++----- src/include/fe_utils/string_utils.h | 1 - 3 files changed, 5 insertions(+), 61 deletions(-) diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c index 4a0d04b646..b30e7db5a6 100644 --- a/src/bin/pg_dump/parallel.c +++ b/src/bin/pg_dump/parallel.c @@ -193,9 +193,6 @@ static CRITICAL_SECTION signal_info_lock; #ifdef WIN32 -/* file-scope variables */ -static DWORD tls_index; - /* globally visible variables (needed by exit_nicely) */ bool parallel_init_done = false; DWORD mainThreadId; @@ -243,8 +240,6 @@ init_parallel_dump_utils(void) WSADATA wsaData; int err; - /* Prepare for threaded operation */ - tls_index = TlsAlloc(); mainThreadId = GetCurrentThreadId(); /* Initialize socket access */ @@ -280,48 +275,6 @@ GetMyPSlot(ParallelState *pstate) return NULL; } -/* - * A thread-local version of getLocalPQExpBuffer(). - * - * Non-reentrant but reduces memory leakage: we'll consume one buffer per - * thread, which is much better than one per fmtId/fmtQualifiedId call. - */ -#ifdef WIN32 -static PQExpBuffer -getThreadLocalPQExpBuffer(void) -{ - /* - * The Tls code goes awry if we use a static var, so we provide for both - * static and auto, and omit any use of the static var when using Tls. We - * rely on TlsGetValue() to return 0 if the value is not yet set. - */ - static PQExpBuffer s_id_return = NULL; - PQExpBuffer id_return; - - if (parallel_init_done) - id_return = (PQExpBuffer) TlsGetValue(tls_index); - else - id_return = s_id_return; - - if (id_return) /* first time through? */ - { - /* same buffer, just wipe contents */ - resetPQExpBuffer(id_return); - } - else - { - /* new buffer */ - id_return = createPQExpBuffer(); - if (parallel_init_done) - TlsSetValue(tls_index, id_return); - else - s_id_return = id_return; - } - - return id_return; -} -#endif /* WIN32 */ - /* * pg_dump and pg_restore call this to register the cleanup handler * as soon as they've created the ArchiveHandle. @@ -914,11 +867,6 @@ ParallelBackupStart(ArchiveHandle *AH) pstate->parallelSlot = pg_malloc0_array(ParallelSlot, pstate->numWorkers); -#ifdef WIN32 - /* Make fmtId() and fmtQualifiedId() use thread-local storage */ - getLocalPQExpBuffer = getThreadLocalPQExpBuffer; -#endif - /* * Set the pstate in shutdown_info, to tell the exit handler that it must * clean up workers as well as the main database connection. But we don't diff --git a/src/fe_utils/string_utils.c b/src/fe_utils/string_utils.c index 7a762251f3..f05bbf89bd 100644 --- a/src/fe_utils/string_utils.c +++ b/src/fe_utils/string_utils.c @@ -21,11 +21,8 @@ #include "fe_utils/string_utils.h" #include "mb/pg_wchar.h" -static PQExpBuffer defaultGetLocalPQExpBuffer(void); - /* Globals exported by this file */ int quote_all_identifiers = 0; -PQExpBuffer (*getLocalPQExpBuffer) (void) = defaultGetLocalPQExpBuffer; static int fmtIdEncoding = -1; @@ -34,14 +31,14 @@ static int fmtIdEncoding = -1; * Returns a temporary PQExpBuffer, valid until the next call to the function. * This is used by fmtId and fmtQualifiedId. * - * Non-reentrant and non-thread-safe but reduces memory leakage. You can - * replace this with a custom version by setting the getLocalPQExpBuffer - * function pointer. + * The buffer is thread-local, so this is safe to call from multiple threads + * at once; each thread gets its own. It is not reentrant within a thread, + * but it reduces memory leakage. */ static PQExpBuffer -defaultGetLocalPQExpBuffer(void) +getLocalPQExpBuffer(void) { - static PQExpBuffer id_return = NULL; + static _Thread_local PQExpBuffer id_return = NULL; if (id_return) /* first time through? */ { diff --git a/src/include/fe_utils/string_utils.h b/src/include/fe_utils/string_utils.h index 0680bb3c19..48e3b69afc 100644 --- a/src/include/fe_utils/string_utils.h +++ b/src/include/fe_utils/string_utils.h @@ -21,7 +21,6 @@ /* Global variables controlling behavior of fmtId() and fmtQualifiedId() */ extern PGDLLIMPORT int quote_all_identifiers; -extern PQExpBuffer (*getLocalPQExpBuffer) (void); /* Functions */ extern const char *fmtId(const char *rawid); -- 2.54.0.windows.1