Re: Improve GetConfigOptionValues function

Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> writes:
> LGTM. I've marked it RfC.

After looking at this, it seemed to me that the factorization
wasn't quite right after all: specifically, the new function
could be used in several more places if it confines itself to
being a privilege check and doesn't consider GUC_NO_SHOW_ALL.
So more like the attached.

You could argue that the factorization is illusory since each
of these additional call sites has an error message that knows
exactly what the conditions are to succeed.  But if we want to
go that direction then I'd be inclined to forget about the
permissions-check function altogether and just test the
conditions in-line everywhere.

Also, I intentionally dropped the GUC_NO_SHOW_ALL check in
get_explain_guc_options, because it seems redundant given
the preceding GUC_EXPLAIN check.  It's unlikely we'd ever have
a variable that's marked both GUC_EXPLAIN and GUC_NO_SHOW_ALL ...
but if we did, shouldn't the former take precedence here anyway?

            regards, tom lane

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index d52069f446..978b385568 100644
--- 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 d59706231b..52c361e975 100644
--- 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 1195abaa3d..d5a0880678 100644
--- 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);