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

robertmhaas · robertmhaas · commit f74f871b80a1 · 2017-10-29T13:14:37.000+05:30
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
@@ -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
@@ -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,27 +9145,14 @@ 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));
 	curptr = start_address + sizeof(actual_size);
 	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
@@ -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
@@ -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
@@ -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;
