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 867ccf855d9252f91b39481f23abf952a4b73824..637f0dd9a4603873a0885872bf22770d7f10737d 100644 (file)
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -70,9 +70,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;
@@ -310,6 +312,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);
@@ -1126,6 +1130,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 4f1891f844074cfa4269b16a21884eb8a99d6a76..7b8e108982b11b53dfd7efb41ce0d82e1aa81543 100644 (file)
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -424,6 +424,7 @@ bool                default_with_oids = false;
 bool           SQL_inheritance = true;
 
 bool           Password_encryption = true;
+bool           session_auth_is_superuser;
 
 int                    log_min_error_statement = ERROR;
 int                    log_min_messages = WARNING;
@@ -470,7 +471,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;
@@ -8873,12 +8873,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;
 }
 
 /*
@@ -9139,7 +9145,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));
@@ -9147,19 +9152,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 0bf9f21067824daed538bb6ba477ddfda7686e78..4e3e15906ac1979e9c6970dc8b30e742f451d989 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 SQL_inheritance;
+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 2286fafab399f72397440f82d8fff7a85a37ad5e..164c4bd61c9dba6370c151893d683c917a353f17 100644 (file)
--- a/src/test/regress/expected/select_parallel.out
+++ b/src/test/regress/expected/select_parallel.out
@@ -99,7 +99,21 @@ explain (costs off)
    ->  Index Only Scan using tenk1_unique1 on tenk1
 (3 rows)
 
+-- test the sanity of parallel query after the active role is dropped.
 set force_parallel_mode=1;
+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;
+select count(*) from tenk1;
+ count 
+-------
+ 10000
+(1 row)
+
+reset role;
 explain (costs off)
   select stringu1::int2 from tenk1 where unique1 = 1;
                   QUERY PLAN                   

diff --git a/src/test/regress/sql/select_parallel.sql b/src/test/regress/sql/select_parallel.sql
index 38d3166742c950c1357505314e93d30f67e7b5e5..e310c096811a6126c4e79a041c5edd8aa41e7271 100644 (file)
--- a/src/test/regress/sql/select_parallel.sql
+++ b/src/test/regress/sql/select_parallel.sql
@@ -39,7 +39,15 @@ explain (costs off)
        select  sum(parallel_restricted(unique1)) from tenk1
        group by(parallel_restricted(unique1));
 
+-- test the sanity of parallel query after the active role is dropped.
 set force_parallel_mode=1;
+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;
+select count(*) from tenk1;
+reset role;
 
 explain (costs off)
   select stringu1::int2 from tenk1 where unique1 = 1;