Re: is_superuser versus set_config_option's parallelism check - Mailing list pgsql-hackers

Nathan Bossart <nathandbossart@gmail.com> writes:
> On Fri, Aug 09, 2024 at 02:43:59PM -0400, Tom Lane wrote:
>> The simplest fix would be to hack this test to allow the action anyway
>> when context == PGC_INTERNAL, excusing that as "assume the caller
>> knows what it's doing".  That feels pretty grotty though.  Perhaps
>> a cleaner way would be to move this check to some higher code level,
>> but I'm not sure where would be a good place.

> From a couple of quick tests, it looks like setting
> "current_role_is_superuser" directly works.

Yeah, I had been thinking along the same lines.  Here's a draft
patch.  (Still needs some attention to nearby comments, and I can't
avoid the impression that the miscinit.c code in this area could
use refactoring.)

A problem with this is that it couldn't readily be back-patched
further than v14, since we didn't have ReportChangedGUCOptions
before that.  Maybe that doesn't matter; given the lack of
previous complaints, maybe we only need to fix this in HEAD.

            regards, tom lane

diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index 6202c5ebe4..7b98b03c10 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -1026,6 +1026,19 @@ show_role(void)
     return role_string ? role_string : "none";
 }

+const char *
+show_is_superuser(void)
+{
+    /*
+     * Report the real value of current_role_is_superuser, not the
+     * possibly-stale value in the GUC mechanism.  The reason we do things
+     * this way is to avoid the overhead and extra failure modes that'd be
+     * involved in using SetConfigOption to set is_superuser every time we
+     * update the session_authorization or role GUCs.
+     */
+    return current_role_is_superuser ? "on" : "off";
+}
+

 /*
  * PATH VARIABLES
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index 537d92c0cf..4052b16ecb 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -822,9 +822,7 @@ InitializeSessionUserId(const char *rolename, Oid roleid, bool bypass_login_chec
     /* Record username and superuser status as GUC settings too */
     SetConfigOption("session_authorization", rname,
                     PGC_BACKEND, PGC_S_OVERRIDE);
-    SetConfigOption("is_superuser",
-                    is_superuser ? "on" : "off",
-                    PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
+    current_role_is_superuser = is_superuser;

     ReleaseSysCache(roleTup);
 }
@@ -854,8 +852,7 @@ InitializeSessionUserIdStandalone(void)
      * Since we don't, C code will get NULL, and current_setting() will get an
      * empty string.
      */
-    SetConfigOption("is_superuser", "on",
-                    PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
+    current_role_is_superuser = true;
 }

 /*
@@ -909,9 +906,7 @@ SetSessionAuthorization(Oid userid, bool is_superuser)
 {
     SetSessionUserId(userid, is_superuser);

-    SetConfigOption("is_superuser",
-                    is_superuser ? "on" : "off",
-                    PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
+    current_role_is_superuser = is_superuser;
 }

 /*
@@ -966,9 +961,7 @@ SetCurrentRoleId(Oid roleid, bool is_superuser)

     SetOuterUserId(roleid);

-    SetConfigOption("is_superuser",
-                    is_superuser ? "on" : "off",
-                    PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
+    current_role_is_superuser = is_superuser;
 }


diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 0c593b81b4..f99975ed61 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2610,6 +2610,15 @@ ReportChangedGUCOptions(void)
         SetConfigOption("in_hot_standby", "false",
                         PGC_INTERNAL, PGC_S_OVERRIDE);

+    /*
+     * Also, check to see if we need to update the is_superuser GUC.  This is
+     * to queue a guc_report_list entry if needed.
+     */
+    if (current_role_is_superuser != current_role_is_superuser_guc)
+        SetConfigOption("is_superuser",
+                        current_role_is_superuser ? "on" : "off",
+                        PGC_INTERNAL, PGC_S_DYNAMIC_DEFAULT);
+
     /* Transmit new values of interesting variables */
     slist_foreach_modify(iter, &guc_report_list)
     {
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index c0a52cdcc3..cf09c59e40 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -516,7 +516,8 @@ bool        check_function_bodies = true;
  */
 static bool default_with_oids = false;

-bool        current_role_is_superuser;
+bool        current_role_is_superuser;    /* this is the "real" status */
+bool        current_role_is_superuser_guc;    /* but the GUC table points here */

 int            log_min_error_statement = ERROR;
 int            log_min_messages = WARNING;
@@ -1017,9 +1018,9 @@ struct config_bool ConfigureNamesBool[] =
             NULL,
             GUC_REPORT | GUC_NO_SHOW_ALL | GUC_NO_RESET_ALL | GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
         },
-        ¤t_role_is_superuser,
+        ¤t_role_is_superuser_guc,
         false,
-        NULL, NULL, NULL
+        NULL, NULL, show_is_superuser
     },
     {
         /*
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 4129ea37ee..efdb4fc815 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -260,6 +260,7 @@ extern PGDLLIMPORT char *event_source;

 extern PGDLLIMPORT bool check_function_bodies;
 extern PGDLLIMPORT bool current_role_is_superuser;
+extern PGDLLIMPORT bool current_role_is_superuser_guc;    /* don't use this */

 extern PGDLLIMPORT bool AllowAlterSystem;
 extern PGDLLIMPORT bool log_duration;
diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h
index 5813dba0a2..d10eb44a2c 100644
--- a/src/include/utils/guc_hooks.h
+++ b/src/include/utils/guc_hooks.h
@@ -121,6 +121,7 @@ extern void assign_recovery_target_xid(const char *newval, void *extra);
 extern bool check_role(char **newval, void **extra, GucSource source);
 extern void assign_role(const char *newval, void *extra);
 extern const char *show_role(void);
+extern const char *show_is_superuser(void);
 extern bool check_restrict_nonsystem_relation_kind(char **newval, void **extra,
                                                    GucSource source);
 extern void assign_restrict_nonsystem_relation_kind(const char *newval, void *extra);