Hi, Seems that Tom's patch cannot be applied to the current master branch. I just re-generate the patch for others to play with.
On 11/2/23, Nathan Bossart <nathandboss...@gmail.com> wrote: > On Wed, Nov 01, 2023 at 10:39:04PM -0400, Tom Lane wrote: >> Nathan Bossart <nathandboss...@gmail.com> writes: >>> What if we disallowed NULL string GUCs in v17? >> >> Well, we'd need to devise some other solution for hacks like the >> one used by timezone_abbreviations (see comment in >> check_timezone_abbreviations). I think it's not worth the trouble, >> especially seeing that 95% of guc.c is already set up for this. >> The bugs are mostly in newer code like get_explain_guc_options, >> and I think that's directly traceable to the lack of any comments >> or docs about this behavior. > > Eh, yeah, it's probably not worth it if we find ourselves trading one set > of hacks for another. > > -- > Nathan Bossart > Amazon Web Services: https://aws.amazon.com > -- Best Regards, Xing
From d620d3a2b44cef63024baca4ccdf55bf7724737f Mon Sep 17 00:00:00 2001 From: Xing Guo <higuox...@gmail.com> Date: Thu, 2 Nov 2023 16:42:00 +0800 Subject: [PATCH v3] Consider treating NULL is a valid boot_val for string GUCs. --- src/backend/utils/misc/guc.c | 28 ++++++++++++++++++++-------- src/include/utils/guc_tables.h | 10 ++++++++++ 2 files changed, 30 insertions(+), 8 deletions(-) diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 39d3775e80..28056af19c 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -1473,7 +1473,9 @@ check_GUC_init(struct config_generic *gconf) { struct config_string *conf = (struct config_string *) gconf; - if (*conf->variable != NULL && strcmp(*conf->variable, conf->boot_val) != 0) + if (*conf->variable != NULL && + (conf->boot_val == NULL || + strcmp(*conf->variable, conf->boot_val) != 0)) { elog(LOG, "GUC (PGC_STRING) %s, boot_val=%s, C-var=%s", conf->gen.name, conf->boot_val ? conf->boot_val : "<null>", *conf->variable); @@ -4213,8 +4215,7 @@ SetConfigOption(const char *name, const char *value, /* * Fetch the current value of the option `name', as a string. * - * If the option doesn't exist, return NULL if missing_ok is true (NOTE that - * this cannot be distinguished from a string variable with a NULL value!), + * If the option doesn't exist, return NULL if missing_ok is true, * otherwise throw an ereport and don't return. * * If restrict_privileged is true, we also enforce that only superusers and @@ -4257,7 +4258,8 @@ GetConfigOption(const char *name, bool missing_ok, bool restrict_privileged) return buffer; case PGC_STRING: - return *((struct config_string *) record)->variable; + return *((struct config_string *) record)->variable ? + *((struct config_string *) record)->variable : ""; case PGC_ENUM: return config_enum_lookup_by_value((struct config_enum *) record, @@ -4304,7 +4306,8 @@ GetConfigOptionResetString(const char *name) return buffer; case PGC_STRING: - return ((struct config_string *) record)->reset_val; + return ((struct config_string *) record)->reset_val ? + ((struct config_string *) record)->reset_val : ""; case PGC_ENUM: return config_enum_lookup_by_value((struct config_enum *) record, @@ -5255,7 +5258,14 @@ get_explain_guc_options(int *num) { struct config_string *lconf = (struct config_string *) conf; - modified = (strcmp(lconf->boot_val, *(lconf->variable)) != 0); + if (lconf->boot_val == NULL && + *lconf->variable == NULL) + modified = false; + else if (lconf->boot_val == NULL || + *lconf->variable == NULL) + modified = true; + else + modified = (strcmp(lconf->boot_val, *(lconf->variable)) != 0); } break; @@ -5482,7 +5492,8 @@ write_one_nondefault_variable(FILE *fp, struct config_generic *gconf) { struct config_string *conf = (struct config_string *) gconf; - fprintf(fp, "%s", *conf->variable); + if (*conf->variable) + fprintf(fp, "%s", *conf->variable); } break; @@ -6142,7 +6153,8 @@ RestoreGUCState(void *gucstate) { struct config_string *conf = (struct config_string *) gconf; - guc_free(*conf->variable); + if (*conf->variable) + guc_free(*conf->variable); if (conf->reset_val && conf->reset_val != *conf->variable) guc_free(conf->reset_val); if (conf->reset_extra && conf->reset_extra != gconf->extra) diff --git a/src/include/utils/guc_tables.h b/src/include/utils/guc_tables.h index 1ec9575570..0c38255961 100644 --- a/src/include/utils/guc_tables.h +++ b/src/include/utils/guc_tables.h @@ -240,6 +240,16 @@ struct config_real void *reset_extra; }; +/* + * A note about string GUCs: the boot_val is allowed to be NULL, which leads + * to the reset_val and the actual variable value (*variable) also being NULL. + * However, there is no way to set a NULL value subsequently using + * set_config_option or any other GUC API. Also, GUC APIs such as SHOW will + * display a NULL value as an empty string. Callers that choose to use a NULL + * boot_val should overwrite the setting later in startup, or else be careful + * that NULL doesn't have semantics that are visibly different from an empty + * string. + */ struct config_string { struct config_generic gen; -- 2.42.0