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
diff --git i/src/backend/utils/misc/guc.c w/src/backend/utils/misc/guc.c
index fc463601ff3..0dde7d3bf40 100644
--- i/src/backend/utils/misc/guc.c
+++ w/src/backend/utils/misc/guc.c
@@ -7008,7 +7008,7 @@ set_config_option(const char *name, const char *value,
if (prohibitValueChange)
{
- if (*conf->variable != newval)
+ if (changeVal && *conf->variable != newval)
{
record->status |= GUC_PENDING_RESTART;
ereport(elevel,
@@ -7098,7 +7098,7 @@ set_config_option(const char *name, const char *value,
if (prohibitValueChange)
{
- if (*conf->variable != newval)
+ if (changeVal && *conf->variable != newval)
{
record->status |= GUC_PENDING_RESTART;
ereport(elevel,
@@ -7188,7 +7188,7 @@ set_config_option(const char *name, const char *value,
if (prohibitValueChange)
{
- if (*conf->variable != newval)
+ if (changeVal && *conf->variable != newval)
{
record->status |= GUC_PENDING_RESTART;
ereport(elevel,
@@ -7295,8 +7295,9 @@ set_config_option(const char *name, const char *value,
if (prohibitValueChange)
{
/* newval shouldn't be NULL, so we're a bit sloppy here */
- if (*conf->variable == NULL || newval == NULL ||
- strcmp(*conf->variable, newval) != 0)
+ if (changeVal && (*conf->variable == NULL ||
+ newval == NULL ||
+ strcmp(*conf->variable, newval) != 0))
{
record->status |= GUC_PENDING_RESTART;
ereport(elevel,
@@ -7391,7 +7392,7 @@ set_config_option(const char *name, const char *value,
if (prohibitValueChange)
{
- if (*conf->variable != newval)
+ if (changeVal && *conf->variable != newval)
{
record->status |= GUC_PENDING_RESTART;
ereport(elevel,
