On 12/8/2025 10:48 AM, Robert Haas wrote:
> On Sat, Dec 6, 2025 at 2:08 AM Bryan Green <[email protected]> wrote:
>>> I think I'm missing something obvious here. call_string_check_hook
>>> doesn't do any memory context management - it just calls the hook.
>
> No, it does do memory management. It has a PG_TRY()/PG_CATCH() block
> to ensure that we don't forget to GUC_free(*newval) in case of error.
> I was trying to figure out where we were doing the relevant memory
> management today, and then extend that to handle the new thing. But I
> am guilty of fuzzy thinking here, because we're talking about where
> the "extra" memory is managed, not the memory for "newval". So the
> logic we care about is in set_config_with_handle() just as you said:
>
> /* Perhaps we didn't install newextra anywhere */
> if (newextra && !extra_field_used(record, newextra))
> guc_free(newextra);
>
> What I hadn't quite internalized previously was that there's no
> PG_TRY/PG_CATCH block here right now because we assume that (1) we
> assume the check hook won't allocate the extra value until it's ready
> to return, so it will never leak a value by allocating it and then
> erroring out and (2) we take care to ensure that no errors can happen
> in the GUC code itself after the extra value has been returned and
> before we either free it or save a pointer to it someplace.
>
> But having said that, I'm inclined to think that handling the memory
> management concerns inside call_WHATEVER_check_hook() still makes some
> sense. It seems to me that if we do that, set_config_with_handle()
> needs very little change. All it needs to do differently is: wherever
> it would guc_free(newextra), it can call some new helper function that
> will either just guc_free() or alternatively
> MemoryContextDelete(GetMemoryChunkContext()) depending on flags. I
> think this is good, because set_config_with_handle() is already pretty
> complicated, and I'd rather not inject more complexity into that
> function.
>
> For this to work, each call_WHATEVER_check_hook() function would need
> a PG_TRY()/PG_CATCH() block, rather than only call_string_check_hook()
> as currently. Or alternatively, and I think this might be an appealing
> option, we could say that this feature is only available for string
> values, and the other call_WHATEVER_check_hook() functions just assert
> that the GUC_EXTRA_IS_CONTEXT flag is not set. I don't see why you'd
> need a complex "extra" value for a bool or int or enum or real-valued
> GUC -- how much complex parsing can you need to do on a non-string
> value?
>
> I think the call_string_check_hook logic in the v2 patch is
> approximately correct. This can be tightened up:
>
> if (result)
> {
> if (*extra != NULL)
> MemoryContextSetParent(extra_cxt, GUCMemoryContext);
> else
> MemoryContextDelete(extra_cxt);
> }
> else
> {
> MemoryContextDelete(extra_cxt);
> }
>
> You can instead write:
>
> if (result != NULL && *extra != NULL)
> MemoryContextSetParent(extra_cxt, GUCMemoryContext);
> else
> MemoryContextDelete(extra_cxt);
>
>> One additional fix: if a check hook succeeds but returns NULL for extra,
>> we delete the empty context rather than reparenting it to avoid leaking
>> contexts that would never be cleaned up.
>
> Yeah, avoiding leaking contexts seems like one of the key challenges
> here. I'm not sure whether we would ever have a check hook that either
> returns a null or non-null *extra depending on the situation, but it
> seems good to be prepared for that case. I notice that guc_free()
> silently accepts a null pointer, so presumably a similar case with a
> "flat" GUC extra could exist and work today.
>
> Also, to respond to your later emails, I agree that the new context
> shouldn't be created under GUCMemoryContext. As discussed with Tom
> earlier, we don't want it to be a long-lived context. I think
> CurrentMemoryContext is OK provided that CurrentMemoryContext is
> always a child of TopTransactionContext, because even if we leak
> something, it will only survive until the end of the transaction at
> latest. However, if CurrentMemoryContext can be something like
> TopMemoryContext or CacheMemoryContext, then we might want to think a
> little harder. I'm not sure whether that's possible -- perhaps you
> would like to investigate? Think particularly about GUCs set during
> server startup -- maybe in the postmaster, maybe in a backend very
> early during initialization. Also maybe configuration reloads while
> the backend is idle.
>
> I think we ought to make this patch use MemoryContextSetIdentifier()
> to make any leaks easier to debug. If a memory context dump shows that
> you've got a whole bunch of contexts floating around, or one really
> big one, and they're all just named "GUC extra context" or whatever,
> that's going to be pretty unhelpful. If the patch does
> MemoryContextSetIdentifier(extra_cxt, conf->name), you'll be able to
> see which GUC is responsible.
>
> I think you should port a couple of the core GUCs use this new
> mechanism. I suggest specifically check_synchronous_standby_names and
> check_synchronized_standby_slots. That should give us some better
> insight into how well this mechanism really works and whether it is
> more convenient in practice than what we're making check hooks do
> today. I thought about proposing that if you do that, you might be
> able to just drop this test module, but both of those GUCs are
> PGC_SIGHUP, so they wouldn't be good for testing the behavior with
> SET, SET LOCAL, RESET, etc. So we might need to either find a case
> that can benefit from this mechanism that is PGC_USERSET or PGC_SUSET,
> or keep the test module in some form. backtrace_functions is a
> possibility, but it's not altogether clear that a non-flat
> representation is better in that case, and it doesn't seem great in
> terms of being able to write simple tests, either.
>
> --
> Robert Haas
> EDB: http://www.enterprisedb.com
Robert,
TLDR; There is a possibility of GUCs using contexts that do not reset;
combined with a small window of opportunity this would lead to a leak
given correct timing. We could accept the leak risk, only allow extra
data as a context for PGC_USERSET and PGC_SUSET GUCs, create a new
context that is global but resets, or look for a different solution.
I've incorporated your suggestions and investigated the memory context
safety concerns you raised.
I selected check_temp_tablespaces (PGC_USERSET) for testing SET and SET
LOCAL behavior, and ported check_synchronous_standby_names and
check_synchronized_standby_slots as you suggested. With working examples
in core, I removed the test module from this patch.
I traced through the code paths to determine which memory context is
active when check hooks execute. PGC_POSTMASTER uses PostmasterContext
during postmaster startup. PGC_SIGHUP uses PostmasterContext during
postmaster startup or TopMemoryContext during backend init, though
interestingly it uses MessageContext during actual SIGHUP reload in a
running backend. PGC_BACKEND uses TopMemoryContext during backend init.
PGC_USERSET and PGC_SUSET use MessageContext during interactive SET
commands.
The key distinction is whether these contexts are reset during error
recovery. PostmasterContext and TopMemoryContext are never reset, while
MessageContext is reset at the command loop and TopTransactionContext is
reset during transaction abort.
There's a small window after PG_END_TRY where we've completed the check
hook successfully but haven't yet reparented the context to
GUCMemoryContext or deleted it.
For PGC_USERSET and PGC_SUSET, this isn't a problem. Even if an error
occurs in that window, the context is a child of MessageContext, which
gets reset at the command loop, cleaning up the context automatically.
No leak occurs.
For PGC_POSTMASTER, PGC_SIGHUP (during startup), and PGC_BACKEND, an
error in that window would orphan a context under PostmasterContext or
TopMemoryContext, which are never reset. This would leak memory for the
backend's lifetime. The probability of hitting this timing is extremely
low, perhaps 1 in 100 million, but it's not zero.
We could restrict the feature to PGC_USERSET and PGC_SUSET only with a
simple assertion, which eliminates the leak possibility entirely.
Alternatively, we could create a dedicated GUCCheckContext that's
explicitly reset during error recovery, allowing all GUC contexts to use
the feature. Finally, we could document the limitation and accept the
extremely low probability of a leak for GUCs using contexts that don't
reset-- which seems...unseemly.
The attached patch works as described but would require either
restricting to specific GUC contexts, accepting the leak, or
implementing a more invasive solution. I'd appreciate your thoughts on
the appropriate path forward.
--
Bryan Green
EDB: https://www.enterprisedb.com
From 5282c2a320bee615715a75a20a351c73bb2c50ef Mon Sep 17 00:00:00 2001
From: Bryan Green <[email protected]>
Date: Sun, 14 Dec 2025 12:45:02 -0600
Subject: [PATCH v3] Allow complex data for GUC extra.
Add assertions in non-string check hook functions to enforce this.
Convert check_synchronous_standby_names, check_synchronized_standby_slots,
and check_temp_tablespaces to use the mechanism, replacing guc_malloc()
with palloc(). This provides test coverage for both SIGHUP and USERSET
contexts.
---
src/backend/commands/tablespace.c | 4 +-
src/backend/replication/slot.c | 6 +-
src/backend/replication/syncrep.c | 8 +--
src/backend/utils/misc/guc.c | 71 ++++++++++++++++++-----
src/backend/utils/misc/guc_parameters.dat | 6 +-
src/include/utils/guc.h | 1 +
6 files changed, 67 insertions(+), 29 deletions(-)
diff --git a/src/backend/commands/tablespace.c
b/src/backend/commands/tablespace.c
index df31eace47..a251bcdd67 100644
--- a/src/backend/commands/tablespace.c
+++ b/src/backend/commands/tablespace.c
@@ -1284,10 +1284,8 @@ 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 = 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;
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 682eccd116..59f11821e6 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -2833,10 +2833,7 @@ check_synchronized_standby_slots(char **newval, void
**extra, GucSource source)
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);
@@ -2852,6 +2849,7 @@ check_synchronized_standby_slots(char **newval, void
**extra, GucSource source)
pfree(rawname);
list_free(elemlist);
+
return true;
}
diff --git a/src/backend/replication/syncrep.c
b/src/backend/replication/syncrep.c
index 298a3766d7..0a431618c5 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -1089,11 +1089,11 @@ check_synchronous_standby_names(char **newval, void
**extra, GucSource source)
return false;
}
- /* GUC extra value must be guc_malloc'd, not palloc'd */
+ /*
+ * With GUC_EXTRA_IS_CONTEXT, we use palloc instead of
guc_malloc.
+ */
pconf = (SyncRepConfigData *)
- guc_malloc(LOG, syncrep_parse_result->config_size);
- if (pconf == NULL)
- return false;
+ palloc(syncrep_parse_result->config_size);
memcpy(pconf, syncrep_parse_result,
syncrep_parse_result->config_size);
*extra = pconf;
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 935c235e1b..40ea2b89cb 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -771,7 +771,15 @@ 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);
+ {
+ if (gconf->flags & GUC_EXTRA_IS_CONTEXT)
+ {
+ MemoryContext ctx = GetMemoryChunkContext(oldval);
+ MemoryContextDelete(ctx);
+ }
+ else
+ guc_free(oldval);
+ }
}
/*
@@ -6641,6 +6649,8 @@ static bool
call_bool_check_hook(const struct config_generic *conf, bool *newval, void
**extra,
GucSource source, int elevel)
{
+ Assert(!(conf->flags & GUC_EXTRA_IS_CONTEXT));
+
/* Quick success if no hook */
if (!conf->_bool.check_hook)
return true;
@@ -6675,6 +6685,8 @@ static bool
call_int_check_hook(const struct config_generic *conf, int *newval, void
**extra,
GucSource source, int elevel)
{
+ Assert(!(conf->flags & GUC_EXTRA_IS_CONTEXT));
+
/* Quick success if no hook */
if (!conf->_int.check_hook)
return true;
@@ -6709,6 +6721,8 @@ static bool
call_real_check_hook(const struct config_generic *conf, double *newval, void
**extra,
GucSource source, int elevel)
{
+ Assert(!(conf->flags & GUC_EXTRA_IS_CONTEXT));
+
/* Quick success if no hook */
if (!conf->_real.check_hook)
return true;
@@ -6743,17 +6757,28 @@ static bool
call_string_check_hook(const struct config_generic *conf, char **newval, void
**extra,
GucSource source, int elevel)
{
+ MemoryContext extra_ctx = NULL;
+ 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)
+ {
+ extra_ctx = AllocSetContextCreate(CurrentMemoryContext,
+
"GUC check extra",
+
ALLOCSET_DEFAULT_SIZES);
+ MemoryContextSetIdentifier(extra_ctx, conf->name);
+ old_ctx = MemoryContextSwitchTo(extra_ctx);
+ }
+
/*
* 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
* already-malloc'd newval string.
*/
PG_TRY();
{
/* Reset variables that might be set by hook */
@@ -6762,18 +6787,20 @@ call_string_check_hook(const struct config_generic
*conf, char **newval, void **
GUC_check_errdetail_string = NULL;
GUC_check_errhint_string = NULL;
- if (!conf->_string.check_hook(newval, extra, source))
+ result = conf->_string.check_hook(newval, extra, source);
+
+ if (!result)
{
ereport(elevel,
(errcode(GUC_check_errcode_value),
GUC_check_errmsg_string ?
errmsg_internal("%s",
GUC_check_errmsg_string) :
errmsg("invalid value for parameter
\"%s\": \"%s\"",
conf->name, *newval ?
*newval : ""),
GUC_check_errdetail_string ?
errdetail_internal("%s",
GUC_check_errdetail_string) : 0,
GUC_check_errhint_string ?
errhint("%s",
GUC_check_errhint_string) : 0));
/* Flush strings created in ErrorContext (ereport might
not have) */
FlushErrorState();
result = false;
@@ -6781,18 +6808,32 @@ call_string_check_hook(const struct config_generic
*conf, char **newval, void **
}
PG_CATCH();
{
+ if (conf->flags & GUC_EXTRA_IS_CONTEXT)
+ {
+ MemoryContextSwitchTo(old_ctx);
+ MemoryContextDelete(extra_ctx);
+ }
guc_free(*newval);
PG_RE_THROW();
}
PG_END_TRY();
-
+ if (conf->flags & GUC_EXTRA_IS_CONTEXT)
+ {
+ MemoryContextSwitchTo(old_ctx);
+ if (result && *extra != NULL)
+ MemoryContextSetParent(extra_ctx, GUCMemoryContext);
+ else
+ MemoryContextDelete(extra_ctx);
+ }
return result;
-}
+ }
static bool
call_enum_check_hook(const struct config_generic *conf, int *newval, void
**extra,
GucSource source, int elevel)
{
+ Assert(!(conf->flags & GUC_EXTRA_IS_CONTEXT));
+
/* 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 3b9d834907..94b519fd63 100644
--- a/src/backend/utils/misc/guc_parameters.dat
+++ b/src/backend/utils/misc/guc_parameters.dat
@@ -2816,7 +2816,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',
@@ -2833,7 +2833,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',
@@ -2937,7 +2937,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..c123a7af55 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 context */
#define GUC_UNIT_KB 0x01000000 /* value is in kilobytes */
#define GUC_UNIT_BLOCKS 0x02000000 /* value is in blocks */
--
2.52.0.windows.1
