Fix some inappropriately-disallowed uses of ALTER ROLE/DATABASE SET.

Most GUC check hooks that inspect database state have special checks
that prevent them from throwing hard errors for state-dependent issues
when source == PGC_S_TEST.  This allows, for example,
"ALTER DATABASE d SET default_text_search_config = foo" when the "foo"
configuration hasn't been created yet.  Without this, we have problems
during dump/reload or pg_upgrade, because pg_dump has no idea about
possible dependencies of GUC values and can't ensure a safe restore
ordering.

However, check_role() and check_session_authorization() hadn't gotten
the memo about that, and would throw hard errors anyway.  It's not
entirely clear what is the use-case for "ALTER ROLE x SET role = y",
but we've now heard two independent complaints about that bollixing
an upgrade, so apparently some people are doing it.

Hence, fix these two functions to act more like other check hooks
with similar needs.  (But I did not change their insistence on
being inside a transaction, as it's still not apparent that setting
either GUC from the configuration file would be wise.)

Also fix check_temp_buffers, which had a different form of the disease
of making state-dependent checks without any exception for PGC_S_TEST.
A cursory survey of other GUC check hooks did not find any more issues
of this ilk.  (There are a lot of interdependencies among
PGC_POSTMASTER and PGC_SIGHUP GUCs, which may be a bad idea, but
they're not relevant to the immediate concern because they can't be
set via ALTER ROLE/DATABASE.)

Per reports from Charlie Hornsby and Nathan Bossart.  Back-patch
to all supported branches.

Discussion: https://postgr.es/m/HE1P189MB0523B31598B0C772C908088DB7709@HE1P189MB0523.EURP189.PROD.OUTLOOK.COM
Discussion: https://postgr.es/m/20160711223641.1426.86096@wrigleys.postgresql.org

diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index c5cf08b423780da380fa17632532db18daa506c6..0c85679420ca028e9573c47dff9a1499bc3c4411 100644 (file)
--- 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 d0a51b507d77ba408d65f53fece6e4ec30673959..2ffefdf9438030f4762f13d3f283e3775799184b 100644 (file)
--- 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;