Re: pg_upgrade check for invalid role-specific default config - Mailing list pgsql-hackers

I wrote:
> Another answer is that maybe the processing of the "role" case
> in particular is just broken.

After digging around a bit more, I think that that is indeed the
right answer.  Most of the GUC check functions that have
database-state-dependent behavior are programmed to behave specially
when checking a proposed ALTER USER/DATABASE setting; but check_role
and check_session_authorization did not get that memo.  I also
noted that check_temp_buffers would throw an error for no very good
reason.  There don't look to be any other troublesome cases.
So I ended up with the attached.

It feels a bit unsatisfactory to have these various check functions
know about this explicitly.  However, I'm not sure that there's a
good way to centralize it.  Only the check function knows whether
the check it's making is immutable or dependent on DB state --- and
in the former case, not throwing an error wouldn't be an improvement.

Anyway, I'm inclined to think that we should not only apply this
but back-patch it.  Two complaints is enough to suggest we have
an issue.  Plus, I think there is a dump/reload ordering problem
here: as things stand, "alter user joe set role = bob" would work
or not work depending on the order the roles are created in
and/or the order privileges are granted in.

            regards, tom lane

diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index c5cf08b423..0c85679420 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -767,6 +767,17 @@ check_session_authorization(char **newval, void **extra, GucSource source)
     roleTup = SearchSysCache1(AUTHNAME, PointerGetDatum(*newval));
     if (!HeapTupleIsValid(roleTup))
     {
+        /*
+         * When source == PGC_S_TEST, we don't throw a hard error for a
+         * nonexistent user name, only a NOTICE.  See comments in guc.h.
+         */
+        if (source == PGC_S_TEST)
+        {
+            ereport(NOTICE,
+                    (errcode(ERRCODE_UNDEFINED_OBJECT),
+                     errmsg("role \"%s\" does not exist", *newval)));
+            return true;
+        }
         GUC_check_errmsg("role \"%s\" does not exist", *newval);
         return false;
     }
@@ -837,10 +848,23 @@ check_role(char **newval, void **extra, GucSource source)
             return false;
         }

+        /*
+         * When source == PGC_S_TEST, we don't throw a hard error for a
+         * nonexistent user name or insufficient privileges, only a NOTICE.
+         * See comments in guc.h.
+         */
+
         /* Look up the username */
         roleTup = SearchSysCache1(AUTHNAME, PointerGetDatum(*newval));
         if (!HeapTupleIsValid(roleTup))
         {
+            if (source == PGC_S_TEST)
+            {
+                ereport(NOTICE,
+                        (errcode(ERRCODE_UNDEFINED_OBJECT),
+                         errmsg("role \"%s\" does not exist", *newval)));
+                return true;
+            }
             GUC_check_errmsg("role \"%s\" does not exist", *newval);
             return false;
         }
@@ -859,6 +883,14 @@ check_role(char **newval, void **extra, GucSource source)
         if (!InitializingParallelWorker &&
             !is_member_of_role(GetSessionUserId(), roleid))
         {
+            if (source == PGC_S_TEST)
+            {
+                ereport(NOTICE,
+                        (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+                         errmsg("permission will be denied to set role \"%s\"",
+                                *newval)));
+                return true;
+            }
             GUC_check_errcode(ERRCODE_INSUFFICIENT_PRIVILEGE);
             GUC_check_errmsg("permission denied to set role \"%s\"",
                              *newval);
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index d0a51b507d..2ffefdf943 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -11777,8 +11777,9 @@ check_temp_buffers(int *newval, void **extra, GucSource source)
 {
     /*
      * Once local buffers have been initialized, it's too late to change this.
+     * However, if this is only a test call, allow it.
      */
-    if (NLocBuffer && NLocBuffer != *newval)
+    if (source != PGC_S_TEST && NLocBuffer && NLocBuffer != *newval)
     {
         GUC_check_errdetail("\"temp_buffers\" cannot be changed after any temporary tables have been accessed in the
session.");
         return false;