Fix problems with the "role" GUC and parallel query.

Without this fix, dropping a role can sometimes result in parallel
query failures in sessions that have used "SET ROLE" to assume the
dropped role, even if that setting isn't active any more.

Report by Pavan Deolasee.  Patch by Amit Kapila, reviewed by me.

Discussion: http://postgr.es/m/CABOikdOomRcZsLsLK+Z+qENM1zxyaWnAvFh3MJZzZnnKiF+REg@mail.gmail.com

diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c
index d68305073303d23bf7f647cdad5d28e4d006da50..1f542ed8d86497661f92cc2824beb85e1e1edf12 100644 (file)
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -75,9 +75,11 @@ typedef struct FixedParallelState
        Oid                     database_id;
        Oid                     authenticated_user_id;
        Oid                     current_user_id;
+       Oid                     outer_user_id;
        Oid                     temp_namespace_id;
        Oid                     temp_toast_namespace_id;
        int                     sec_context;
+       bool            is_superuser;
        PGPROC     *parallel_master_pgproc;
        pid_t           parallel_master_pid;
        BackendId       parallel_master_backend_id;
@@ -296,6 +298,8 @@ InitializeParallelDSM(ParallelContext *pcxt)
                shm_toc_allocate(pcxt->toc, sizeof(FixedParallelState));
        fps->database_id = MyDatabaseId;
        fps->authenticated_user_id = GetAuthenticatedUserId();
+       fps->outer_user_id = GetCurrentRoleId();
+       fps->is_superuser = session_auth_is_superuser;
        GetUserIdAndSecContext(&fps->current_user_id, &fps->sec_context);
        GetTempNamespaceState(&fps->temp_namespace_id,
                                                  &fps->temp_toast_namespace_id);
@@ -1115,6 +1119,13 @@ ParallelWorkerMain(Datum main_arg)
         */
        InvalidateSystemCaches();
 
+       /*
+        * Restore current role id.  Skip verifying whether session user is
+        * allowed to become this role and blindly restore the leader's state for
+        * current role.
+        */
+       SetCurrentRoleId(fps->outer_user_id, fps->is_superuser);
+
        /* Restore user ID and security context. */
        SetUserIdAndSecContext(fps->current_user_id, fps->sec_context);
 

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index ae22185fbdbeaa954ca09e78a0c02c2b6091cea7..65372d7cc5542f6a6633137a8553158cda784c82 100644 (file)
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -446,6 +446,7 @@ char           *event_source;
 bool           row_security;
 bool           check_function_bodies = true;
 bool           default_with_oids = false;
+bool           session_auth_is_superuser;
 
 int                    log_min_error_statement = ERROR;
 int                    log_min_messages = WARNING;
@@ -492,7 +493,6 @@ int                 huge_pages;
  * and is kept in sync by assign_hooks.
  */
 static char *syslog_ident_str;
-static bool session_auth_is_superuser;
 static double phony_random_seed;
 static char *client_encoding_string;
 static char *datestyle_string;
@@ -8986,12 +8986,18 @@ read_nondefault_variables(void)
  * constants; a few, like server_encoding and lc_ctype, are handled specially
  * outside the serialize/restore procedure.  Therefore, SerializeGUCState()
  * never sends these, and RestoreGUCState() never changes them.
+ *
+ * Role is a special variable in the sense that its current value can be an
+ * invalid value and there are multiple ways by which that can happen (like
+ * after setting the role, someone drops it).  So we handle it outside of
+ * serialize/restore machinery.
  */
 static bool
 can_skip_gucvar(struct config_generic *gconf)
 {
        return gconf->context == PGC_POSTMASTER ||
-               gconf->context == PGC_INTERNAL || gconf->source == PGC_S_DEFAULT;
+               gconf->context == PGC_INTERNAL || gconf->source == PGC_S_DEFAULT ||
+               strcmp(gconf->name, "role") == 0;
 }
 
 /*
@@ -9252,7 +9258,6 @@ SerializeGUCState(Size maxsize, char *start_address)
        Size            actual_size;
        Size            bytes_left;
        int                     i;
-       int                     i_role = -1;
 
        /* Reserve space for saving the actual size of the guc state */
        Assert(maxsize > sizeof(actual_size));
@@ -9260,19 +9265,7 @@ SerializeGUCState(Size maxsize, char *start_address)
        bytes_left = maxsize - sizeof(actual_size);
 
        for (i = 0; i < num_guc_variables; i++)
-       {
-               /*
-                * It's pretty ugly, but we've got to force "role" to be initialized
-                * after "session_authorization"; otherwise, the latter will override
-                * the former.
-                */
-               if (strcmp(guc_variables[i]->name, "role") == 0)
-                       i_role = i;
-               else
-                       serialize_variable(&curptr, &bytes_left, guc_variables[i]);
-       }
-       if (i_role >= 0)
-               serialize_variable(&curptr, &bytes_left, guc_variables[i_role]);
+               serialize_variable(&curptr, &bytes_left, guc_variables[i]);
 
        /* Store actual size without assuming alignment of start_address. */
        actual_size = maxsize - bytes_left - sizeof(actual_size);

diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 467125a09da41b591917e2bcb3ec4e0aeb16d866..aa130cdb847bfec7970eee9edef511a159b35f6c 100644 (file)
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -245,6 +245,7 @@ extern bool log_btree_build_stats;
 
 extern PGDLLIMPORT bool check_function_bodies;
 extern bool default_with_oids;
+extern bool    session_auth_is_superuser;
 
 extern int     log_min_error_statement;
 extern int     log_min_messages;

diff --git a/src/test/regress/expected/select_parallel.out b/src/test/regress/expected/select_parallel.out
index 3c63ad1de86976fd96d23264b304d438a3e736e0..ac9ad0668d17c75ac8f4bd21f4613946f321b39c 100644 (file)
--- a/src/test/regress/expected/select_parallel.out
+++ b/src/test/regress/expected/select_parallel.out
@@ -508,6 +508,22 @@ SELECT make_record(x) FROM (SELECT generate_series(1, 5) x) ss ORDER BY x;
 
 ROLLBACK TO SAVEPOINT settings;
 DROP function make_record(n int);
+-- test the sanity of parallel query after the active role is dropped.
+drop role if exists regress_parallel_worker;
+NOTICE:  role "regress_parallel_worker" does not exist, skipping
+create role regress_parallel_worker;
+set role regress_parallel_worker;
+reset session authorization;
+drop role regress_parallel_worker;
+set force_parallel_mode = 1;
+select count(*) from tenk1;
+ count 
+-------
+ 10000
+(1 row)
+
+reset force_parallel_mode;
+reset role;
 -- to increase the parallel query test coverage
 SAVEPOINT settings;
 SET LOCAL force_parallel_mode = 1;

diff --git a/src/test/regress/sql/select_parallel.sql b/src/test/regress/sql/select_parallel.sql
index 720495c81173982a309729c4928197ef1346befd..495f0335dccd1120aa73b6c69fbf601f0c3fcd76 100644 (file)
--- a/src/test/regress/sql/select_parallel.sql
+++ b/src/test/regress/sql/select_parallel.sql
@@ -201,6 +201,17 @@ SELECT make_record(x) FROM (SELECT generate_series(1, 5) x) ss ORDER BY x;
 ROLLBACK TO SAVEPOINT settings;
 DROP function make_record(n int);
 
+-- test the sanity of parallel query after the active role is dropped.
+drop role if exists regress_parallel_worker;
+create role regress_parallel_worker;
+set role regress_parallel_worker;
+reset session authorization;
+drop role regress_parallel_worker;
+set force_parallel_mode = 1;
+select count(*) from tenk1;
+reset force_parallel_mode;
+reset role;
+
 -- to increase the parallel query test coverage
 SAVEPOINT settings;
 SET LOCAL force_parallel_mode = 1;