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