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

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) &&
+		!ConfigOptionIsVisible(record))
-		!has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))
 		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) &&
+	if (!ConfigOptionIsVisible(record))
-		!has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))
 		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) ||
+		if (!ConfigOptionIsVisible(conf))
-			((conf->flags & GUC_SUPERUSER_ONLY) &&
-			 !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)))
 			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) &&
+	if (!ConfigOptionIsVisible(record))
-		!has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS))
 		ereport(ERROR,
 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
 				 errmsg("must be superuser or have privileges of pg_read_all_settings to examine \"%s\"",

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) ||
+		/* skip if marked NO_SHOW_ALL */
-			((conf->flags & GUC_SUPERUSER_ONLY) &&
+		if (conf->flags & GUC_NO_SHOW_ALL)
-			 !has_privs_of_role(GetUserId(), ROLE_PG_READ_ALL_SETTINGS)))
+			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,
+GetConfigOptionValues(struct config_generic *conf, const char **values)
-					  bool *noshow)
 {
 	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;
 
-		/*
+		/* skip if marked NO_SHOW_ALL or if not visible to current user */
-		 * Get the next visible GUC variable name and value
+		if ((conf->flags & GUC_NO_SHOW_ALL) ||
-		 */
+			!ConfigOptionIsVisible(conf))
-		do
 		{
-			GetConfigOptionValues(guc_vars[call_cntr], (const char **) values,
+			call_cntr = ++funcctx->call_cntr;
-								  &noshow);
+			continue;
-			if (noshow)
+		}
-			{
-				/* bump the counter and get the next config setting */
-				call_cntr = ++funcctx->call_cntr;
 
-				/* make sure we haven't gone too far now */
+		/* extract values for the current variable */
-				if (call_cntr >= max_calls)
+		GetConfigOptionValues(conf, (const char **) values);
-					SRF_RETURN_DONE(funcctx);
-			}
-		} while (noshow);
 
 		/* 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 */
-		/* do when there is no more left */
+	SRF_RETURN_DONE(funcctx);
-		SRF_RETURN_DONE(funcctx);
-	}
 }
 
 /*

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);