warning on reload for PGC_POSTMASTER, guc.c duplication, ...
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t40990psql -h localhost -U postgresBuilt from patchset v1 (message #1), July 28, 2026 at 03:56 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t40990_1 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t40990_1 && git checkout t40990_1Patchset v1 (message #1) is on t40990_1
Hi,
When specifying a config a PGC_POSTMASTER variable on the commandline
(i.e. -c something=other) the config processing blurts a wrong warning
about not being able to change that value. E.g. when specifying
shared_buffers via -c, I get:
2019-07-26 16:28:04.795 PDT [14464][] LOG: 00000: received SIGHUP, reloading configuration files
2019-07-26 16:28:04.795 PDT [14464][] LOCATION: SIGHUP_handler, postmaster.c:2629
2019-07-26 16:28:04.798 PDT [14464][] LOG: 55P02: parameter "shared_buffers" cannot be changed without restarting the server
2019-07-26 16:28:04.798 PDT [14464][] LOCATION: set_config_option, guc.c:7107
2019-07-26 16:28:04.798 PDT [14464][] LOG: F0000: configuration file "/srv/dev/pgdev-dev/postgresql.conf" contains errors; unaffected changes were applied
2019-07-26 16:28:04.798 PDT [14464][] LOCATION: ProcessConfigFileInternal, guc-file.l:502
ISTM that the codeblocks throwing these warnings:
if (prohibitValueChange)
{
if (*conf->variable != newval)
{
record->status |= GUC_PENDING_RESTART;
ereport(elevel,
(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
errmsg("parameter \"%s\" cannot be changed without restarting the server",
name)));
return 0;
}
record->status &= ~GUC_PENDING_RESTART;
return -1;
}
ought to only enter the error path if changeVal indicates that we're
actually intending to apply the value. I.e. something roughly like the
attached.
Two more things I noticed when looking at this code:
1) Aren't we leaking memory if prohibitValueChange is set, but newextra
is present? The cleanup path for that:
/* Perhaps we didn't install newextra anywhere */
if (newextra && !extra_field_used(&conf->gen, newextra))
free(newextra);
isn't reached in the prohibitValueChange path shown above. ISTM the
return -1 in the prohibitValueChange ought to be removed?
2) The amount of PGC_* dependant code duplication in set_config_option()
imo is over the top. ISTM that they should be merged, and
a call_*_check_hook wrapper take care of invoking the check hooks,
and a nother wrapper should take care of calling the assign hook,
->variable, and reset_val processing.
Those wrappers could probably also reduce the amount of code in
InitializeOneGUCOption(), parse_and_validate_value(),
ResetAllOptions(), AtEOXact_GUC().
I'm also wondering we shouldn't just use config_var_value for at
least config_*->{reset_val, boot_val}. It seems pretty clear that
reset_extra ought to be moved?
I'm even wondering the various hooks shouldn't actually just take
config_var_value. But changing that would probably cause more pain to
external users - in contrast to looking directly at reset_val,
boot_val, reset_extra they're much more likely to have hooks.
Greetings,
Andres Freund