v3-0001-Allow-complex-data-for-GUC-extra-data.patch
text/plain
Filename: v3-0001-Allow-complex-data-for-GUC-extra-data.patch
Type: text/plain
Part: 0
From 5233b6e70b2704741749f3e7dcc6ce02118f6bb2 Mon Sep 17 00:00:00 2001
From: Bryan Green <dbryan.green@gmail.com>
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