From 3c1b8510a546a504dd583c7f0979f645c30b8ac6 Mon Sep 17 00:00:00 2001 From: Bryan Green Date: Sun, 28 Jun 2026 11:40:58 -0500 Subject: [PATCH v1 2/4] 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 and the getLocalPQExpBuffer function-pointer swap can go. --- src/bin/pg_dump/parallel.c | 52 ------------------------------------- src/fe_utils/string_utils.c | 7 ++--- 2 files changed, 4 insertions(+), 55 deletions(-) diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c index b77d2650df..12b462375d 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. @@ -918,11 +871,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..b3738c25db 100644 --- a/src/fe_utils/string_utils.c +++ b/src/fe_utils/string_utils.c @@ -34,14 +34,15 @@ 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 + * The buffer is thread-local, so this is safe to call from multiple threads + * at once; each thread gets its own. Not reentrant within a thread, but it + * reduces memory leakage. Replace it by setting the getLocalPQExpBuffer * function pointer. */ static PQExpBuffer defaultGetLocalPQExpBuffer(void) { - static PQExpBuffer id_return = NULL; + static _Thread_local PQExpBuffer id_return = NULL; if (id_return) /* first time through? */ { -- 2.54.0.windows.1