From 9f5836d224e876399dfdd7d6d4343300dbc2f664 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 4 Oct 2011 16:13:16 -0400 Subject: [PATCH] Remember the source GucContext for each GUC parameter. We used to just remember the GucSource, but saving GucContext too provides a little more information --- notably, whether a SET was done by a superuser or regular user. This allows us to rip out the fairly dodgy code that define_custom_variable used to use to try to infer the context to re-install a pre-existing setting with. In particular, it now works for a superuser to SET a extension's SUSET custom variable before loading the associated extension, because GUC can remember whether the SET was done as a superuser or not. The plperl regression tests contain an example where this is useful. --- src/backend/utils/misc/guc-file.l | 18 ++-- src/backend/utils/misc/guc.c | 108 ++++++++++------------- src/include/utils/guc_tables.h | 14 +-- src/pl/plperl/expected/plperl_init.out | 2 +- src/pl/plperl/expected/plperl_shared.out | 3 +- src/pl/plperl/expected/plperlu.out | 2 +- src/pl/plperl/sql/plperl_init.sql | 2 +- src/pl/plperl/sql/plperl_shared.sql | 3 +- src/pl/plperl/sql/plperlu.sql | 2 +- 9 files changed, 73 insertions(+), 81 deletions(-) diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l index 7728544c54..b91da01e86 100644 --- a/src/backend/utils/misc/guc-file.l +++ b/src/backend/utils/misc/guc-file.l @@ -296,11 +296,7 @@ ProcessConfigFile(GucContext context) GUC_ACTION_SET, true); if (scres > 0) { - /* variable was updated, so remember the source location */ - set_config_sourcefile(item->name, item->filename, - item->sourceline); - - /* and log the change if appropriate */ + /* variable was updated, so log the change if appropriate */ if (pre_value) { const char *post_value = GetConfigOption(item->name, true, false); @@ -315,7 +311,17 @@ ProcessConfigFile(GucContext context) } else if (scres == 0) error = true; - /* else no error but variable was not changed, do nothing */ + /* else no error but variable's active value was not changed */ + + /* + * We should update source location unless there was an error, since + * even if the active value didn't change, the reset value might have. + * (In the postmaster, there won't be a difference, but it does matter + * in backends.) + */ + if (scres != 0) + set_config_sourcefile(item->name, item->filename, + item->sourceline); if (pre_value) pfree(pre_value); diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 2fd4867d25..84702aa46e 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -3861,8 +3861,10 @@ static void InitializeOneGUCOption(struct config_generic * gconf) { gconf->status = 0; - gconf->reset_source = PGC_S_DEFAULT; gconf->source = PGC_S_DEFAULT; + gconf->reset_source = PGC_S_DEFAULT; + gconf->scontext = PGC_INTERNAL; + gconf->reset_scontext = PGC_INTERNAL; gconf->stack = NULL; gconf->extra = NULL; gconf->sourcefile = NULL; @@ -4213,6 +4215,7 @@ ResetAllOptions(void) } gconf->source = gconf->reset_source; + gconf->scontext = gconf->reset_scontext; if (gconf->flags & GUC_REPORT) ReportGUCOption(gconf); @@ -4254,6 +4257,7 @@ push_old_value(struct config_generic * gconf, GucAction action) if (stack->state == GUC_SET) { /* SET followed by SET LOCAL, remember SET's value */ + stack->masked_scontext = gconf->scontext; set_stack_value(gconf, &stack->masked); stack->state = GUC_SET_LOCAL; } @@ -4291,6 +4295,7 @@ push_old_value(struct config_generic * gconf, GucAction action) break; } stack->source = gconf->source; + stack->scontext = gconf->scontext; set_stack_value(gconf, &stack->prior); gconf->stack = stack; @@ -4431,6 +4436,7 @@ AtEOXact_GUC(bool isCommit, int nestLevel) if (prev->state == GUC_SET) { /* LOCAL migrates down */ + prev->masked_scontext = stack->scontext; prev->masked = stack->prior; prev->state = GUC_SET_LOCAL; } @@ -4445,6 +4451,7 @@ AtEOXact_GUC(bool isCommit, int nestLevel) /* prior state at this level no longer wanted */ discard_stack_value(gconf, &stack->prior); /* copy down the masked state */ + prev->masked_scontext = stack->masked_scontext; if (prev->state == GUC_SET_LOCAL) discard_stack_value(gconf, &prev->masked); prev->masked = stack->masked; @@ -4460,16 +4467,19 @@ AtEOXact_GUC(bool isCommit, int nestLevel) /* Perform appropriate restoration of the stacked value */ config_var_value newvalue; GucSource newsource; + GucContext newscontext; if (restoreMasked) { newvalue = stack->masked; newsource = PGC_S_SESSION; + newscontext = stack->masked_scontext; } else { newvalue = stack->prior; newsource = stack->source; + newscontext = stack->scontext; } switch (gconf->vartype) @@ -4581,7 +4591,9 @@ AtEOXact_GUC(bool isCommit, int nestLevel) set_extra_field(gconf, &(stack->prior.extra), NULL); set_extra_field(gconf, &(stack->masked.extra), NULL); + /* And restore source information */ gconf->source = newsource; + gconf->scontext = newscontext; } /* Finish popping the state stack */ @@ -5255,6 +5267,7 @@ set_config_option(const char *name, const char *value, newval = conf->reset_val; newextra = conf->reset_extra; source = conf->gen.reset_source; + context = conf->gen.reset_scontext; } if (prohibitValueChange) @@ -5282,6 +5295,7 @@ set_config_option(const char *name, const char *value, set_extra_field(&conf->gen, &conf->gen.extra, newextra); conf->gen.source = source; + conf->gen.scontext = context; } if (makeDefault) { @@ -5293,6 +5307,7 @@ set_config_option(const char *name, const char *value, set_extra_field(&conf->gen, &conf->reset_extra, newextra); conf->gen.reset_source = source; + conf->gen.reset_scontext = context; } for (stack = conf->gen.stack; stack; stack = stack->prev) { @@ -5302,6 +5317,7 @@ set_config_option(const char *name, const char *value, set_extra_field(&conf->gen, &stack->prior.extra, newextra); stack->source = source; + stack->scontext = context; } } } @@ -5355,6 +5371,7 @@ set_config_option(const char *name, const char *value, newval = conf->reset_val; newextra = conf->reset_extra; source = conf->gen.reset_source; + context = conf->gen.reset_scontext; } if (prohibitValueChange) @@ -5382,6 +5399,7 @@ set_config_option(const char *name, const char *value, set_extra_field(&conf->gen, &conf->gen.extra, newextra); conf->gen.source = source; + conf->gen.scontext = context; } if (makeDefault) { @@ -5393,6 +5411,7 @@ set_config_option(const char *name, const char *value, set_extra_field(&conf->gen, &conf->reset_extra, newextra); conf->gen.reset_source = source; + conf->gen.reset_scontext = context; } for (stack = conf->gen.stack; stack; stack = stack->prev) { @@ -5402,6 +5421,7 @@ set_config_option(const char *name, const char *value, set_extra_field(&conf->gen, &stack->prior.extra, newextra); stack->source = source; + stack->scontext = context; } } } @@ -5452,6 +5472,7 @@ set_config_option(const char *name, const char *value, newval = conf->reset_val; newextra = conf->reset_extra; source = conf->gen.reset_source; + context = conf->gen.reset_scontext; } if (prohibitValueChange) @@ -5479,6 +5500,7 @@ set_config_option(const char *name, const char *value, set_extra_field(&conf->gen, &conf->gen.extra, newextra); conf->gen.source = source; + conf->gen.scontext = context; } if (makeDefault) { @@ -5490,6 +5512,7 @@ set_config_option(const char *name, const char *value, set_extra_field(&conf->gen, &conf->reset_extra, newextra); conf->gen.reset_source = source; + conf->gen.reset_scontext = context; } for (stack = conf->gen.stack; stack; stack = stack->prev) { @@ -5499,6 +5522,7 @@ set_config_option(const char *name, const char *value, set_extra_field(&conf->gen, &stack->prior.extra, newextra); stack->source = source; + stack->scontext = context; } } } @@ -5567,6 +5591,7 @@ set_config_option(const char *name, const char *value, newval = conf->reset_val; newextra = conf->reset_extra; source = conf->gen.reset_source; + context = conf->gen.reset_scontext; } if (prohibitValueChange) @@ -5596,6 +5621,7 @@ set_config_option(const char *name, const char *value, set_extra_field(&conf->gen, &conf->gen.extra, newextra); conf->gen.source = source; + conf->gen.scontext = context; } if (makeDefault) @@ -5608,6 +5634,7 @@ set_config_option(const char *name, const char *value, set_extra_field(&conf->gen, &conf->reset_extra, newextra); conf->gen.reset_source = source; + conf->gen.reset_scontext = context; } for (stack = conf->gen.stack; stack; stack = stack->prev) { @@ -5618,6 +5645,7 @@ set_config_option(const char *name, const char *value, set_extra_field(&conf->gen, &stack->prior.extra, newextra); stack->source = source; + stack->scontext = context; } } } @@ -5673,6 +5701,7 @@ set_config_option(const char *name, const char *value, newval = conf->reset_val; newextra = conf->reset_extra; source = conf->gen.reset_source; + context = conf->gen.reset_scontext; } if (prohibitValueChange) @@ -5700,6 +5729,7 @@ set_config_option(const char *name, const char *value, set_extra_field(&conf->gen, &conf->gen.extra, newextra); conf->gen.source = source; + conf->gen.scontext = context; } if (makeDefault) { @@ -5711,6 +5741,7 @@ set_config_option(const char *name, const char *value, set_extra_field(&conf->gen, &conf->reset_extra, newextra); conf->gen.reset_source = source; + conf->gen.reset_scontext = context; } for (stack = conf->gen.stack; stack; stack = stack->prev) { @@ -5720,6 +5751,7 @@ set_config_option(const char *name, const char *value, set_extra_field(&conf->gen, &stack->prior.extra, newextra); stack->source = source; + stack->scontext = context; } } } @@ -6252,7 +6284,6 @@ define_custom_variable(struct config_generic * variable) const char **nameAddr = &name; const char *value; struct config_string *pHolder; - GucContext phcontext; struct config_generic **res; /* @@ -6298,56 +6329,6 @@ define_custom_variable(struct config_generic * variable) */ *res = variable; - /* - * Infer context for assignment based on source of existing value. We - * can't tell this with exact accuracy, but we can at least do something - * reasonable in typical cases. - */ - switch (pHolder->gen.source) - { - case PGC_S_DEFAULT: - case PGC_S_DYNAMIC_DEFAULT: - case PGC_S_ENV_VAR: - case PGC_S_FILE: - case PGC_S_ARGV: - - /* - * If we got past the check in init_custom_variable, we can safely - * assume that any existing value for a PGC_POSTMASTER variable - * was set in postmaster context. - */ - if (variable->context == PGC_POSTMASTER) - phcontext = PGC_POSTMASTER; - else - phcontext = PGC_SIGHUP; - break; - - case PGC_S_DATABASE: - case PGC_S_USER: - case PGC_S_DATABASE_USER: - - /* - * The existing value came from an ALTER ROLE/DATABASE SET - * command. We can assume that at the time the command was issued, - * we checked that the issuing user was superuser if the variable - * requires superuser privileges to set. So it's safe to use - * SUSET context here. - */ - phcontext = PGC_SUSET; - break; - - case PGC_S_CLIENT: - case PGC_S_SESSION: - default: - - /* - * We must assume that the value came from an untrusted user, even - * if the current_user is a superuser. - */ - phcontext = PGC_USERSET; - break; - } - /* * Assign the string value stored in the placeholder to the real variable. * @@ -6360,8 +6341,8 @@ define_custom_variable(struct config_generic * variable) if (value) { if (set_config_option(name, value, - phcontext, pHolder->gen.source, - GUC_ACTION_SET, true) > 0) + pHolder->gen.scontext, pHolder->gen.source, + GUC_ACTION_SET, true) != 0) { /* Also copy over any saved source-location information */ if (pHolder->gen.sourcefile) @@ -7284,6 +7265,7 @@ _ShowOption(struct config_generic * record, bool use_units) * variable name, string, null terminated * variable value, string, null terminated * variable source, integer + * variable scontext, integer */ static void write_one_nondefault_variable(FILE *fp, struct config_generic * gconf) @@ -7319,8 +7301,7 @@ write_one_nondefault_variable(FILE *fp, struct config_generic * gconf) { struct config_real *conf = (struct config_real *) gconf; - /* Could lose precision here? */ - fprintf(fp, "%f", *conf->variable); + fprintf(fp, "%.17g", *conf->variable); } break; @@ -7344,7 +7325,8 @@ write_one_nondefault_variable(FILE *fp, struct config_generic * gconf) fputc(0, fp); - fwrite(&gconf->source, sizeof(gconf->source), 1, fp); + fwrite(&gconf->source, 1, sizeof(gconf->source), fp); + fwrite(&gconf->scontext, 1, sizeof(gconf->scontext), fp); } void @@ -7436,7 +7418,8 @@ read_nondefault_variables(void) FILE *fp; char *varname, *varvalue; - int varsource; + GucSource varsource; + GucContext varscontext; /* * Open file @@ -7464,11 +7447,14 @@ read_nondefault_variables(void) elog(FATAL, "failed to locate variable %s in exec config params file", varname); if ((varvalue = read_string_with_null(fp)) == NULL) elog(FATAL, "invalid format of exec config params file"); - if (fread(&varsource, sizeof(varsource), 1, fp) == 0) + if (fread(&varsource, 1, sizeof(varsource), fp) != sizeof(varsource)) + elog(FATAL, "invalid format of exec config params file"); + if (fread(&varscontext, 1, sizeof(varscontext), fp) != sizeof(varscontext)) elog(FATAL, "invalid format of exec config params file"); - (void) set_config_option(varname, varvalue, record->context, - varsource, GUC_ACTION_SET, true); + (void) set_config_option(varname, varvalue, + varscontext, varsource, + GUC_ACTION_SET, true); free(varname); free(varvalue); } diff --git a/src/include/utils/guc_tables.h b/src/include/utils/guc_tables.h index 84e9fb54d2..21342f59ac 100644 --- a/src/include/utils/guc_tables.h +++ b/src/include/utils/guc_tables.h @@ -118,9 +118,11 @@ typedef struct guc_stack int nest_level; /* nesting depth at which we made entry */ GucStackState state; /* see enum above */ GucSource source; /* source of the prior value */ + /* masked value's source must be PGC_S_SESSION, so no need to store it */ + GucContext scontext; /* context that set the prior value */ + GucContext masked_scontext; /* context that set the masked value */ config_var_value prior; /* previous value of variable */ config_var_value masked; /* SET value in a GUC_SET_LOCAL entry */ - /* masked value's source must be PGC_S_SESSION, so no need to store it */ } GucStack; /* @@ -143,21 +145,21 @@ struct config_generic enum config_group group; /* to help organize variables by function */ const char *short_desc; /* short desc. of this variable's purpose */ const char *long_desc; /* long desc. of this variable's purpose */ - int flags; /* flag bits, see below */ + int flags; /* flag bits, see guc.h */ /* variable fields, initialized at runtime: */ enum config_type vartype; /* type of variable (set only at startup) */ int status; /* status bits, see below */ - GucSource reset_source; /* source of the reset_value */ GucSource source; /* source of the current actual value */ + GucSource reset_source; /* source of the reset_value */ + GucContext scontext; /* context that set the current value */ + GucContext reset_scontext; /* context that set the reset value */ GucStack *stack; /* stacked prior values */ void *extra; /* "extra" pointer for current actual value */ char *sourcefile; /* file current setting is from (NULL if not - * file) */ + * set in config file) */ int sourceline; /* line in source file */ }; -/* bit values in flags field are defined in guc.h */ - /* bit values in status field */ #define GUC_IS_IN_FILE 0x0001 /* found it in config file */ /* diff --git a/src/pl/plperl/expected/plperl_init.out b/src/pl/plperl/expected/plperl_init.out index e8a8e9bd83..a21ea0b621 100644 --- a/src/pl/plperl/expected/plperl_init.out +++ b/src/pl/plperl/expected/plperl_init.out @@ -1,5 +1,5 @@ -- test plperl.on_plperl_init errors are fatal --- Must load plperl before we can set on_plperl_init +-- This test tests setting on_plperl_init after loading plperl LOAD 'plperl'; SET SESSION plperl.on_plperl_init = ' system("/nonesuch") '; SHOW plperl.on_plperl_init; diff --git a/src/pl/plperl/expected/plperl_shared.out b/src/pl/plperl/expected/plperl_shared.out index 67478ab454..464e22090c 100644 --- a/src/pl/plperl/expected/plperl_shared.out +++ b/src/pl/plperl/expected/plperl_shared.out @@ -1,7 +1,6 @@ -- test plperl.on_plperl_init via the shared hash -- (must be done before plperl is first used) --- Must load plperl before we can set on_plperl_init -LOAD 'plperl'; +-- This test tests setting on_plperl_init before loading plperl -- testing on_plperl_init gets run, and that it can alter %_SHARED SET plperl.on_plperl_init = '$_SHARED{on_init} = 42'; -- test the shared hash diff --git a/src/pl/plperl/expected/plperlu.out b/src/pl/plperl/expected/plperlu.out index 1ba07eed9d..3daf4ced86 100644 --- a/src/pl/plperl/expected/plperlu.out +++ b/src/pl/plperl/expected/plperlu.out @@ -1,6 +1,6 @@ -- Use ONLY plperlu tests here. For plperl/plerlu combined tests -- see plperl_plperlu.sql --- Must load plperl before we can set on_plperlu_init +-- This test tests setting on_plperlu_init after loading plperl LOAD 'plperl'; -- Test plperl.on_plperlu_init gets run SET plperl.on_plperlu_init = '$_SHARED{init} = 42'; diff --git a/src/pl/plperl/sql/plperl_init.sql b/src/pl/plperl/sql/plperl_init.sql index 51ac9192bd..d60268d033 100644 --- a/src/pl/plperl/sql/plperl_init.sql +++ b/src/pl/plperl/sql/plperl_init.sql @@ -1,6 +1,6 @@ -- test plperl.on_plperl_init errors are fatal --- Must load plperl before we can set on_plperl_init +-- This test tests setting on_plperl_init after loading plperl LOAD 'plperl'; SET SESSION plperl.on_plperl_init = ' system("/nonesuch") '; diff --git a/src/pl/plperl/sql/plperl_shared.sql b/src/pl/plperl/sql/plperl_shared.sql index d2fa8cbf93..b60e114faf 100644 --- a/src/pl/plperl/sql/plperl_shared.sql +++ b/src/pl/plperl/sql/plperl_shared.sql @@ -1,8 +1,7 @@ -- test plperl.on_plperl_init via the shared hash -- (must be done before plperl is first used) --- Must load plperl before we can set on_plperl_init -LOAD 'plperl'; +-- This test tests setting on_plperl_init before loading plperl -- testing on_plperl_init gets run, and that it can alter %_SHARED SET plperl.on_plperl_init = '$_SHARED{on_init} = 42'; diff --git a/src/pl/plperl/sql/plperlu.sql b/src/pl/plperl/sql/plperlu.sql index 831b8f4460..be43df5d90 100644 --- a/src/pl/plperl/sql/plperlu.sql +++ b/src/pl/plperl/sql/plperlu.sql @@ -1,7 +1,7 @@ -- Use ONLY plperlu tests here. For plperl/plerlu combined tests -- see plperl_plperlu.sql --- Must load plperl before we can set on_plperlu_init +-- This test tests setting on_plperlu_init after loading plperl LOAD 'plperl'; -- Test plperl.on_plperlu_init gets run