From 5282c2a320bee615715a75a20a351c73bb2c50ef Mon Sep 17 00:00:00 2001 From: Bryan Green 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