Re: Don't pass NULL pointer to strcmp().

I wrote:
> Hmm ... if we're doing it ourselves, I suppose we've got to consider
> it supported :-(.  But I'm still wondering how many seldom-used
> code paths didn't get the message.  An example here is that this
> could lead to GetConfigOptionResetString returning NULL, which
> I think is outside its admittedly-vague API spec.

After digging around for a bit, I think part of the problem is a lack
of a clearly defined spec for what should happen with NULL string GUCs.
In the attached v3, I attempted to remedy that by adding a comment in
guc_tables.h (which is maybe not the best place but I didn't see a
better one).  That led me to a couple more changes beyond what you had.

It's possible that some of these are unreachable --- for example,
given that a NULL could only be the default value, I'm not sure that
the fix in write_one_nondefault_variable is a live bug.  But we ought
to code all this stuff defensively, and most of it already was
NULL-safe.

            regards, tom lane

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 39d3775e80..ccffaaad02 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;