warning on reload for PGC_POSTMASTER, guc.c duplication, ...

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