On 12/16/2025 9:04 PM, Tom Lane wrote:
> Robert Haas <[email protected]> writes:
>> On Tue, Dec 16, 2025 at 4:49 PM Tom Lane <[email protected]> wrote:
>>> ... Therefore, there can be at most one of these
>>> operations in flight at a time, so you don't need any dynamic data
>>> structure. A simple static variable remembering a not-yet-reparented
>>> context would do it.
>
>> Oh, yeah, I actually wondered if that would be an acceptable
>> restriction and had it in an earlier version of the email, but it got
>> lost in the final draft. Maybe with this design you just do something
>> like:
>> if (TempCheckHookConteck != NULL)
>> MemoryContextReset(TempCheckHookConteck);
>> else
>> TempCheckHookConteck = AllocSetContextCreate(...);
>> So then if the context survives, you just reset and reuse it, but if
>> it gets reparented, you set the variable to NULL and create a new
>> context the next time. Then you don't need any integration with
>> (sub)transaction abort at all, which seems nice.
>
> You could do it like that, but I'd prefer a setup that would give
> an assertion failure if someone did try to invoke it recursively.
> So I'd opt for allocation like
>
> Assert(TempCheckHookContext == NULL);
> TempCheckHookContext = AllocSetContextCreate(...);
>
> and then you would need cleanup in AtEOXact_GUC, but that's
> hardly complicated.
>
> regards, tom lane
Robert, Tom,
Following your feedback, I've implemented the static context variable
approach. The attached v3 patch uses a single TempCheckHookContext
that gets created on first use and cleaned up during error recovery.
To catch recursive use, I've added Assert(TempCheckHookContext == NULL)
before creating the context.
One notable behavioral change: check hooks using GUC_EXTRA_IS_CONTEXT
now use palloc() instead of guc_malloc(). The old approach with
guc_malloc() allowed check hooks to return false on OOM, letting the
caller handle it at the appropriate error level. With palloc() an OOM
throws an immediate ERROR. This seemed like an acceptable tradeoff - if
we can't allocate memory for a small temporary context, we're likely
in dire straits anyway. However, this does mean check hooks lose the
ability to gracefully handle OOM by returning false.
Not all code paths flow through AtEOXact_GUC(). To handle orphaned
contexts in these cases, I've added CleanupTempCheckHookContext() as
a public function. AtEOXact_GUC() calls it at the end, and other
contexts can call it as needed to ensure cleanup happens regardless of
the execution path.
I've restricted GUC_EXTRA_IS_CONTEXT to string GUCs only, since that's
where complex parsing actually makes sense. The other check hook types
now assert the flag isn't set.
To demonstrate the mechanism works in practice, I've ported three GUCs:
check_synchronous_standby_names and check_synchronized_standby_slots
(both PGC_SIGHUP), plus check_temp_tablespaces (PGC_USERSET) for testing
SET and SET LOCAL behavior.
--
Bryan Green
EDB: https://www.enterprisedb.com
From 5233b6e70b2704741749f3e7dcc6ce02118f6bb2 Mon Sep 17 00:00:00 2001
From: Bryan Green <[email protected]>
Date: Thu, 18 Dec 2025 13:24:39 -0600
Subject: [PATCH v3] Allow complex data for GUC extra data
String GUC check hooks that allocate "extra" data previously used
guc_malloc() and manual cleanup. If an ERROR was thrown during
validation, cleanup code was bypassed, leaking memory in
non-transactional contexts (SIGHUP reload, postmaster startup).
Convert 3 check hooks to use this infrastructure, removing manual
cleanup code and fixing potential memory leaks.
Note: This changes OOM behavior from soft failure (return false) to
hard failure (ERROR).
---
src/backend/commands/tablespace.c | 10 +-
src/backend/postmaster/postmaster.c | 2 +
src/backend/replication/slot.c | 11 +--
src/backend/replication/syncrep.c | 20 +---
src/backend/utils/misc/guc.c | 113 ++++++++++++++++++++--
src/backend/utils/misc/guc_parameters.dat | 6 +-
src/include/utils/guc.h | 2 +
7 files changed, 121 insertions(+), 43 deletions(-)
diff --git a/src/backend/commands/tablespace.c
b/src/backend/commands/tablespace.c
index df31eace47..9a6a8a6429 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -1208,8 +1208,6 @@ check_temp_tablespaces(char **newval, void **extra,
GucSource source)
{
/* syntax error in name list */
GUC_check_errdetail("List syntax is invalid.");
- pfree(rawname);
- list_free(namelist);
return false;
}
@@ -1284,19 +1282,15 @@ check_temp_tablespaces(char **newval, void **extra,
GucSource source)
}
/* Now prepare an "extra" struct for assign_temp_tablespaces */
- myextra = guc_malloc(LOG, offsetof(temp_tablespaces_extra,
tblSpcs) +
+ myextra = (temp_tablespaces_extra *)
palloc(offsetof(temp_tablespaces_extra, tblSpcs) +
numSpcs * sizeof(Oid));
- if (!myextra)
- return false;
myextra->numSpcs = numSpcs;
memcpy(myextra->tblSpcs, tblSpcs, numSpcs * sizeof(Oid));
*extra = myextra;
- pfree(tblSpcs);
+
}
- pfree(rawname);
- list_free(namelist);
return true;
}
diff --git a/src/backend/postmaster/postmaster.c
b/src/backend/postmaster/postmaster.c
index cf44a67718..347fd9a744 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -785,6 +785,7 @@ PostmasterMain(int argc, char *argv[])
if (!SelectConfigFiles(userDoption, progname))
ExitPostmaster(2);
+ CleanupTempCheckHookContext();
if (output_config_variable != NULL)
{
/*
@@ -2014,6 +2015,7 @@ process_pm_reload_request(void)
ereport(LOG,
(errmsg("received SIGHUP, reloading
configuration files")));
ProcessConfigFile(PGC_SIGHUP);
+ CleanupTempCheckHookContext();
SignalChildren(SIGHUP, btmask_all_except(B_DEAD_END_BACKEND));
/* Reload authentication config files too */
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 58c41d4551..61f584cb9e 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -2962,21 +2962,14 @@ check_synchronized_standby_slots(char **newval, void
**extra, GucSource source)
ok = validate_sync_standby_slots(rawname, &elemlist);
if (!ok || elemlist == NIL)
- {
- pfree(rawname);
- list_free(elemlist);
return ok;
- }
/* Compute the size required for the SyncStandbySlotsConfigData struct
*/
size = offsetof(SyncStandbySlotsConfigData, slot_names);
foreach_ptr(char, slot_name, elemlist)
size += strlen(slot_name) + 1;
- /* GUC extra value must be guc_malloc'd, not palloc'd */
- config = (SyncStandbySlotsConfigData *) guc_malloc(LOG, size);
- if (!config)
- return false;
+ config = (SyncStandbySlotsConfigData *) palloc(size);
/* Transform the data into SyncStandbySlotsConfigData */
config->nslotnames = list_length(elemlist);
@@ -2990,8 +2983,6 @@ check_synchronized_standby_slots(char **newval, void
**extra, GucSource source)
*extra = config;
- pfree(rawname);
- list_free(elemlist);
return true;
}
diff --git a/src/backend/replication/syncrep.c
b/src/backend/replication/syncrep.c
index 298a3766d7..0450a2e56b 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -1060,7 +1060,6 @@ check_synchronous_standby_names(char **newval, void
**extra, GucSource source)
{
yyscan_t scanner;
int parse_rc;
- SyncRepConfigData *pconf;
/* Result of parsing is returned in one of these two variables
*/
SyncRepConfigData *syncrep_parse_result = NULL;
@@ -1089,22 +1088,13 @@ check_synchronous_standby_names(char **newval, void
**extra, GucSource source)
return false;
}
- /* GUC extra value must be guc_malloc'd, not palloc'd */
- pconf = (SyncRepConfigData *)
- guc_malloc(LOG, syncrep_parse_result->config_size);
- if (pconf == NULL)
- return false;
- memcpy(pconf, syncrep_parse_result,
syncrep_parse_result->config_size);
-
- *extra = pconf;
-
/*
- * We need not explicitly clean up syncrep_parse_result. It,
and any
- * other cruft generated during parsing, will be freed when the
- * current memory context is deleted. (This code is generally
run in
- * a short-lived context used for config file processing, so
that will
- * not be very long.)
+ * With GUC_EXTRA_IS_CONTEXT, the parser allocated
syncrep_parse_result
+ * in TempCheckHookContext. We can use it directly without
copying,
+ * as the infrastructure will reparent the context to
GUCMemoryContext
+ * on success or delete it on failure.
*/
+ *extra = syncrep_parse_result;
}
else
*extra = NULL;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 935c235e1b..f227af40b9 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -199,6 +199,13 @@ static const char *const map_old_guc_names[] = {
/* Memory context holding all GUC-related data */
static MemoryContext GUCMemoryContext;
+/*
+ * Temporary context for check hook allocations with GUC_EXTRA_IS_CONTEXT.
+ * This is to clean up contexts during error recovery that haven't been
+ * reparented to GUCMemoryContext. There is at most one check operation in
+ * progress at a time, so we can get away with a single static context.
+ */
+static MemoryContext TempCheckHookContext = NULL;
/*
* We use a dynahash table to look up GUCs by name, or to iterate through
* all the GUCs. The gucname field is redundant with gucvar->name, but
@@ -756,6 +763,18 @@ extra_field_used(struct config_generic *gconf, void *extra)
return false;
}
+static void
+guc_free_value(struct config_generic *gconf, void *ptr)
+{
+ if (ptr == NULL)
+ return;
+
+ if (gconf->flags & GUC_EXTRA_IS_CONTEXT)
+ MemoryContextDelete(GetMemoryChunkContext(ptr));
+ else
+ guc_free(ptr);
+}
+
/*
* Support for assigning to an "extra" field of a GUC item. Free the prior
* value if it's not referenced anywhere else in the item (including stacked
@@ -771,7 +790,9 @@ set_extra_field(struct config_generic *gconf, void **field,
void *newval)
/* Free old value if it's not NULL and isn't referenced anymore */
if (oldval && !extra_field_used(gconf, oldval))
- guc_free(oldval);
+ {
+ guc_free_value(gconf, oldval);
+ }
}
/*
@@ -2410,9 +2431,30 @@ AtEOXact_GUC(bool isCommit, int nestLevel)
/* Update nesting level */
GUCNestLevel = nestLevel - 1;
-}
+ /* Clean up any orphaned check hook context */
+ if (TempCheckHookContext)
+ {
+ MemoryContextDelete(TempCheckHookContext);
+ TempCheckHookContext = NULL;
+ }
+}
+/*
+ * CleanupTempCheckHookContext: public function to clean up check hook context
+ *
+ * This is for use by postmaster and other non-transactional contexts where
+ * AtEOXact_GUC won't be called.
+ */
+void
+CleanupTempCheckHookContext(void)
+{
+ if (TempCheckHookContext)
+ {
+ MemoryContextDelete(TempCheckHookContext);
+ TempCheckHookContext = NULL;
+ }
+}
/*
* Start up automatic reporting of changes to variables marked GUC_REPORT.
* This is executed at completion of backend startup.
@@ -3926,7 +3968,7 @@ set_config_with_handle(const char *name, config_handle
*handle,
guc_free(newval);
/* Release newextra, unless it's
reset_extra */
if (newextra &&
!extra_field_used(record, newextra))
- guc_free(newextra);
+ guc_free_value(record,
newextra);
if (newval_different)
{
@@ -4026,7 +4068,7 @@ set_config_with_handle(const char *name, config_handle
*handle,
guc_free(newval);
/* Perhaps we didn't install newextra anywhere
*/
if (newextra && !extra_field_used(record,
newextra))
- guc_free(newextra);
+ guc_free_value(record, newextra);
break;
#undef newval
@@ -4574,7 +4616,7 @@ AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt)
if (record->vartype == PGC_STRING &&
newval.stringval != NULL)
guc_free(newval.stringval);
- guc_free(newextra);
+ guc_free_value(record, newextra);
}
}
else
@@ -6114,7 +6156,7 @@ RestoreGUCState(void *gucstate)
* in.
*/
Assert(gconf->stack == NULL);
- guc_free(gconf->extra);
+ guc_free_value(gconf, gconf->extra);
guc_free(gconf->last_reported);
guc_free(gconf->sourcefile);
switch (gconf->vartype)
@@ -6136,7 +6178,7 @@ RestoreGUCState(void *gucstate)
}
}
if (gconf->reset_extra && gconf->reset_extra != gconf->extra)
- guc_free(gconf->reset_extra);
+ guc_free_value(gconf, gconf->reset_extra);
/* Remove it from any lists it's in. */
RemoveGUCFromLists(gconf);
/* Now we can reset the struct to PGS_S_DEFAULT state. */
@@ -6641,6 +6683,12 @@ static bool
call_bool_check_hook(const struct config_generic *conf, bool *newval, void
**extra,
GucSource source, int elevel)
{
+ /*
+ * TempCheckHookContext is used only by string check hooks, but we
+ * Assert it's not set here to catch any misuses.
+ */
+ Assert(!TempCheckHookContext);
+
/* Quick success if no hook */
if (!conf->_bool.check_hook)
return true;
@@ -6675,6 +6723,12 @@ static bool
call_int_check_hook(const struct config_generic *conf, int *newval, void
**extra,
GucSource source, int elevel)
{
+ /*
+ * TempCheckHookContext is used only by string check hooks, but we
+ * Assert it's not set here to catch any misuses.
+ */
+ Assert(!TempCheckHookContext);
+
/* Quick success if no hook */
if (!conf->_int.check_hook)
return true;
@@ -6709,6 +6763,12 @@ static bool
call_real_check_hook(const struct config_generic *conf, double *newval, void
**extra,
GucSource source, int elevel)
{
+ /*
+ * TempCheckHookContext is used only by string check hooks, but we
+ * Assert it's not set here to catch any misuses.
+ */
+ Assert(!TempCheckHookContext);
+
/* Quick success if no hook */
if (!conf->_real.check_hook)
return true;
@@ -6743,12 +6803,23 @@ static bool
call_string_check_hook(const struct config_generic *conf, char **newval, void
**extra,
GucSource source, int elevel)
{
+ MemoryContext old_ctx = NULL;
volatile bool result = true;
/* Quick success if no hook */
if (!conf->_string.check_hook)
return true;
+ if (conf->flags & GUC_EXTRA_IS_CONTEXT)
+ {
+ Assert(TempCheckHookContext == NULL);
+
+ TempCheckHookContext =
AllocSetContextCreate(CurrentMemoryContext,
+
"GUC string check context",
+
ALLOCSET_DEFAULT_SIZES);
+ MemoryContextSetIdentifier(TempCheckHookContext, conf->name);
+ old_ctx = MemoryContextSwitchTo(TempCheckHookContext);
+ }
/*
* If elevel is ERROR, or if the check_hook itself throws an elog
* (undesirable, but not always avoidable), make sure we don't leak the
@@ -6781,11 +6852,33 @@ call_string_check_hook(const struct config_generic
*conf, char **newval, void **
}
PG_CATCH();
{
+ if (TempCheckHookContext)
+ {
+ if (old_ctx)
+ MemoryContextSwitchTo(old_ctx);
+ MemoryContextDelete(TempCheckHookContext);
+ TempCheckHookContext = NULL;
+ }
guc_free(*newval);
PG_RE_THROW();
}
PG_END_TRY();
+ if (TempCheckHookContext)
+ {
+ MemoryContextSwitchTo(old_ctx);
+
+ if (result && *extra)
+ {
+ MemoryContextSetParent(TempCheckHookContext,
GUCMemoryContext);
+ }
+ else
+ {
+ MemoryContextDelete(TempCheckHookContext);
+ }
+ TempCheckHookContext = NULL;
+ }
+
return result;
}
@@ -6793,6 +6886,12 @@ static bool
call_enum_check_hook(const struct config_generic *conf, int *newval, void
**extra,
GucSource source, int elevel)
{
+ /*
+ * TempCheckHookContext is used only by string check hooks, but we
+ * Assert it's not set here to catch any misuses.
+ */
+ Assert(!TempCheckHookContext);
+
/* Quick success if no hook */
if (!conf->_enum.check_hook)
return true;
diff --git a/src/backend/utils/misc/guc_parameters.dat
b/src/backend/utils/misc/guc_parameters.dat
index ac0c7c36c5..4c8e19204e 100644
--- a/src/backend/utils/misc/guc_parameters.dat
+++ b/src/backend/utils/misc/guc_parameters.dat
@@ -2825,7 +2825,7 @@
{ name => 'synchronized_standby_slots', type => 'string', context =>
'PGC_SIGHUP', group => 'REPLICATION_PRIMARY',
short_desc => 'Lists streaming replication standby server replication slot
names that logical WAL sender processes will wait for.',
long_desc => 'Logical WAL sender processes will send decoded changes to
output plugins only after the specified replication slots have confirmed
receiving WAL.',
- flags => 'GUC_LIST_INPUT',
+ flags => 'GUC_LIST_INPUT | GUC_EXTRA_IS_CONTEXT',
variable => 'synchronized_standby_slots',
boot_val => '""',
check_hook => 'check_synchronized_standby_slots',
@@ -2842,7 +2842,7 @@
{ name => 'synchronous_standby_names', type => 'string', context =>
'PGC_SIGHUP', group => 'REPLICATION_PRIMARY',
short_desc => 'Number of synchronous standbys and list of names of potential
synchronous ones.',
- flags => 'GUC_LIST_INPUT',
+ flags => 'GUC_LIST_INPUT | GUC_EXTRA_IS_CONTEXT',
variable => 'SyncRepStandbyNames',
boot_val => '""',
check_hook => 'check_synchronous_standby_names',
@@ -2946,7 +2946,7 @@
{ name => 'temp_tablespaces', type => 'string', context => 'PGC_USERSET',
group => 'CLIENT_CONN_STATEMENT',
short_desc => 'Sets the tablespace(s) to use for temporary tables and sort
files.',
long_desc => 'An empty string means use the database\'s default tablespace.',
- flags => 'GUC_LIST_INPUT | GUC_LIST_QUOTE',
+ flags => 'GUC_LIST_INPUT | GUC_LIST_QUOTE | GUC_EXTRA_IS_CONTEXT',
variable => 'temp_tablespaces',
boot_val => '""',
check_hook => 'check_temp_tablespaces',
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index f21ec37da8..479fecef5c 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -228,6 +228,7 @@ typedef enum
0x002000 /* can't
set in PG_AUTOCONF_FILENAME */
#define GUC_RUNTIME_COMPUTED 0x004000 /* delay processing in 'postgres -C' */
#define GUC_ALLOW_IN_PARALLEL 0x008000 /* allow setting in parallel mode */
+#define GUC_EXTRA_IS_CONTEXT 0x010000 /* extra field is a MemoryContext */
#define GUC_UNIT_KB 0x01000000 /* value is in kilobytes */
#define GUC_UNIT_BLOCKS 0x02000000 /* value is in blocks */
@@ -432,6 +433,7 @@ extern void AtStart_GUC(void);
extern int NewGUCNestLevel(void);
extern void RestrictSearchPath(void);
extern void AtEOXact_GUC(bool isCommit, int nestLevel);
+extern void CleanupTempCheckHookContext(void);
extern void BeginReportingGUCOptions(void);
extern void ReportChangedGUCOptions(void);
extern void ParseLongOption(const char *string, char **name, char **value);
--
2.52.0.windows.1
