Minor GUC code refactoring.

Split out "ConfigOptionIsVisible" to perform the privilege
check for GUC_SUPERUSER_ONLY GUCs (which these days can also
be read by pg_read_all_settings role members), and move the
should-we-show-it checks from GetConfigOptionValues to its
sole caller.

This commit also removes get_explain_guc_options's check of
GUC_NO_SHOW_ALL, which seems to have got cargo-culted in there.
While there's no obvious use-case for marking a GUC both
GUC_EXPLAIN and GUC_NO_SHOW_ALL, if it were set up that way
one would expect EXPLAIN to show it --- if that's not what
you want, then don't set GUC_EXPLAIN.

In passing, simplify the loop logic in show_all_settings.

Nitin Jadhav, Bharath Rupireddy, Tom Lane

Discussion: https://postgr.es/m/CAMm1aWYgfekpRK-Jz5=pM_bV+Om=ktGq1vxTZ_dr1Z6MV-qokA@mail.gmail.com

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index d52069f446af86a68f07d992e34df12948f2a006..978b3855682f7cf480d69920bac175f24a2dbea2 100644 (file)
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -4187,8 +4187,7 @@ GetConfigOption(const char *name, bool missing_ok, bool restrict_privileged)
        if (record == NULL)
                return NULL;
        if (restrict_privileged &&
-               (record->flags & GUC_SUPERUSER_ONLY) &&
-               !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))
+               !ConfigOptionIsVisible(record))
                ereport(ERROR,
                                (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
                                 errmsg("must be superuser or have privileges of pg_read_all_settings to examine \"%s\"",
@@ -4234,8 +4233,7 @@ GetConfigOptionResetString(const char *name)
 
        record = find_option(name, false, false, ERROR);
        Assert(record != NULL);
-       if ((record->flags & GUC_SUPERUSER_ONLY) &&
-               !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))
+       if (!ConfigOptionIsVisible(record))
                ereport(ERROR,
                                (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
                                 errmsg("must be superuser or have privileges of pg_read_all_settings to examine \"%s\"",
@@ -5160,9 +5158,7 @@ get_explain_guc_options(int *num)
                        continue;
 
                /* return only options visible to the current user */
-               if ((conf->flags & GUC_NO_SHOW_ALL) ||
-                       ((conf->flags & GUC_SUPERUSER_ONLY) &&
-                        !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)))
+               if (!ConfigOptionIsVisible(conf))
                        continue;
 
                /* return only options that are different from their boot values */
@@ -5243,8 +5239,7 @@ GetConfigOptionByName(const char *name, const char **varname, bool missing_ok)
                return NULL;
        }
 
-       if ((record->flags & GUC_SUPERUSER_ONLY) &&
-               !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))
+       if (!ConfigOptionIsVisible(record))
                ereport(ERROR,
                                (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
                                 errmsg("must be superuser or have privileges of pg_read_all_settings to examine \"%s\"",

diff --git a/src/backend/utils/misc/guc_funcs.c b/src/backend/utils/misc/guc_funcs.c
index d59706231b869f5ca9a3a4880d9d803ef416371b..52c361e9755865af17daccfae79316f8b7901f10 100644 (file)
--- a/src/backend/utils/misc/guc_funcs.c
+++ b/src/backend/utils/misc/guc_funcs.c
@@ -492,9 +492,12 @@ ShowAllGUCConfig(DestReceiver *dest)
                struct config_generic *conf = guc_vars[i];
                char       *setting;
 
-               if ((conf->flags & GUC_NO_SHOW_ALL) ||
-                       ((conf->flags & GUC_SUPERUSER_ONLY) &&
-                        !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)))
+               /* skip if marked NO_SHOW_ALL */
+               if (conf->flags & GUC_NO_SHOW_ALL)
+                       continue;
+
+               /* return only options visible to the current user */
+               if (!ConfigOptionIsVisible(conf))
                        continue;
 
                /* assign to the values array */
@@ -581,25 +584,27 @@ pg_settings_get_flags(PG_FUNCTION_ARGS)
        PG_RETURN_ARRAYTYPE_P(a);
 }
 
+/*
+ * Return whether or not the GUC variable is visible to the current user.
+ */
+bool
+ConfigOptionIsVisible(struct config_generic *conf)
+{
+       if ((conf->flags & GUC_SUPERUSER_ONLY) &&
+               !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))
+               return false;
+       else
+               return true;
+}
+
 /*
  * Extract fields to show in pg_settings for given variable.
  */
 static void
-GetConfigOptionValues(struct config_generic *conf, const char **values,
-                                         bool *noshow)
+GetConfigOptionValues(struct config_generic *conf, const char **values)
 {
        char            buffer[256];
 
-       if (noshow)
-       {
-               if ((conf->flags & GUC_NO_SHOW_ALL) ||
-                       ((conf->flags & GUC_SUPERUSER_ONLY) &&
-                        !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)))
-                       *noshow = true;
-               else
-                       *noshow = false;
-       }
-
        /* first get the generic attributes */
 
        /* name */
@@ -940,30 +945,23 @@ show_all_settings(PG_FUNCTION_ARGS)
        max_calls = funcctx->max_calls;
        attinmeta = funcctx->attinmeta;
 
-       if (call_cntr < max_calls)      /* do when there is more left to send */
+       while (call_cntr < max_calls)   /* do when there is more left to send */
        {
+               struct config_generic *conf = guc_vars[call_cntr];
                char       *values[NUM_PG_SETTINGS_ATTS];
-               bool            noshow;
                HeapTuple       tuple;
                Datum           result;
 
-               /*
-                * Get the next visible GUC variable name and value
-                */
-               do
+               /* skip if marked NO_SHOW_ALL or if not visible to current user */
+               if ((conf->flags & GUC_NO_SHOW_ALL) ||
+                       !ConfigOptionIsVisible(conf))
                {
-                       GetConfigOptionValues(guc_vars[call_cntr], (const char **) values,
-                                                                 &noshow);
-                       if (noshow)
-                       {
-                               /* bump the counter and get the next config setting */
-                               call_cntr = ++funcctx->call_cntr;
+                       call_cntr = ++funcctx->call_cntr;
+                       continue;
+               }
 
-                               /* make sure we haven't gone too far now */
-                               if (call_cntr >= max_calls)
-                                       SRF_RETURN_DONE(funcctx);
-                       }
-               } while (noshow);
+               /* extract values for the current variable */
+               GetConfigOptionValues(conf, (const char **) values);
 
                /* build a tuple */
                tuple = BuildTupleFromCStrings(attinmeta, values);
@@ -973,11 +971,9 @@ show_all_settings(PG_FUNCTION_ARGS)
 
                SRF_RETURN_NEXT(funcctx, result);
        }
-       else
-       {
-               /* do when there is no more left */
-               SRF_RETURN_DONE(funcctx);
-       }
+
+       /* do when there is no more left */
+       SRF_RETURN_DONE(funcctx);
 }
 
 /*

diff --git a/src/include/utils/guc_tables.h b/src/include/utils/guc_tables.h
index 1195abaa3d98e96a61821918787244f04a9c1d14..d5a08806782fe39dceb4e3b26a92ff2d07f714d6 100644 (file)
--- a/src/include/utils/guc_tables.h
+++ b/src/include/utils/guc_tables.h
@@ -292,6 +292,9 @@ extern struct config_generic **get_explain_guc_options(int *num);
 /* get string value of variable */
 extern char *ShowGUCOption(struct config_generic *record, bool use_units);
 
+/* get whether or not the GUC variable is visible to current user */
+extern bool ConfigOptionIsVisible(struct config_generic *conf);
+
 /* get the current set of variables */
 extern struct config_generic **get_guc_variables(int *num_vars);