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
