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 ce1b907debd0f7eb4eb732911da58307acd6a808..e1a6a75a87268a05c57c6a0955d8cc77c732501c 100644 (file)
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -71,9 +71,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;
@@ -275,6 +277,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);
@@ -1079,6 +1083,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 58a4cf97ba59cf94612256f1134cfdff539ecd82..bee9cc5be4dbb5eaed661bce5eedbbc4ec4b360e 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 c1870d213014342f84888746eac23f71691e73e5..aa2c7c160c01a371d925c6b4b32f966231fcae98 100644 (file)
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -244,6 +244,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 74588702a15102d0d1d4f36e9b1b5330077faa18..6bbcd34b800e0cc76f14f028bdd0de0eafe4ccd8 100644 (file)
--- a/src/test/regress/expected/select_parallel.out
+++ b/src/test/regress/expected/select_parallel.out
@@ -424,6 +424,22 @@ select string4 from tenk1 order by string4 limit 5;
 
 reset max_parallel_workers;
 reset enable_hashagg;
+-- 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;
 set force_parallel_mode=1;
 explain (costs off)
   select stringu1::int2 from tenk1 where unique1 = 1;

diff --git a/src/test/regress/sql/select_parallel.sql b/src/test/regress/sql/select_parallel.sql
index 09c1b03eb1d92fe0ba578625796f174313568198..cf02d84c8d8fa198f69be53ef09710b3619a690c 100644 (file)
--- a/src/test/regress/sql/select_parallel.sql
+++ b/src/test/regress/sql/select_parallel.sql
@@ -162,6 +162,17 @@ select string4 from tenk1 order by string4 limit 5;
 reset max_parallel_workers;
 reset enable_hashagg;
 
+-- 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;
+
 set force_parallel_mode=1;
 
 explain (costs off)