Skip to content

Commit 846fcc8

Browse files
robertmhaasrobertmhaas
committed
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
1 parent 5f39712 commit 846fcc8

File tree

5 files changed

+48
-16
lines changed

5 files changed

+48
-16
lines changed

‎src/backend/access/transam/parallel.c

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,9 +75,11 @@ typedef struct FixedParallelState
7575
Oid database_id;
7676
Oid authenticated_user_id;
7777
Oid current_user_id;
78+
Oid outer_user_id;
7879
Oid temp_namespace_id;
7980
Oid temp_toast_namespace_id;
8081
int sec_context;
82+
bool is_superuser;
8183
PGPROC *parallel_master_pgproc;
8284
pid_t parallel_master_pid;
8385
BackendId parallel_master_backend_id;
@@ -296,6 +298,8 @@ InitializeParallelDSM(ParallelContext *pcxt)
296298
shm_toc_allocate(pcxt->toc, sizeof(FixedParallelState));
297299
fps->database_id = MyDatabaseId;
298300
fps->authenticated_user_id = GetAuthenticatedUserId();
301+
fps->outer_user_id = GetCurrentRoleId();
302+
fps->is_superuser = session_auth_is_superuser;
299303
GetUserIdAndSecContext(&fps->current_user_id, &fps->sec_context);
300304
GetTempNamespaceState(&fps->temp_namespace_id,
301305
&fps->temp_toast_namespace_id);
@@ -1115,6 +1119,13 @@ ParallelWorkerMain(Datum main_arg)
11151119
*/
11161120
InvalidateSystemCaches();
11171121

1122+
/*
1123+
* Restore current role id. Skip verifying whether session user is
1124+
* allowed to become this role and blindly restore the leader's state for
1125+
* current role.
1126+
*/
1127+
SetCurrentRoleId(fps->outer_user_id, fps->is_superuser);
1128+
11181129
/* Restore user ID and security context. */
11191130
SetUserIdAndSecContext(fps->current_user_id, fps->sec_context);
11201131

‎src/backend/utils/misc/guc.c

Lines changed: 9 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -446,6 +446,7 @@ char *event_source;
446446
bool row_security;
447447
bool check_function_bodies = true;
448448
bool default_with_oids = false;
449+
bool session_auth_is_superuser;
449450

450451
int log_min_error_statement = ERROR;
451452
int log_min_messages = WARNING;
@@ -492,7 +493,6 @@ int huge_pages;
492493
* and is kept in sync by assign_hooks.
493494
*/
494495
static char *syslog_ident_str;
495-
static bool session_auth_is_superuser;
496496
static double phony_random_seed;
497497
static char *client_encoding_string;
498498
static char *datestyle_string;
@@ -8986,12 +8986,18 @@ read_nondefault_variables(void)
89868986
* constants; a few, like server_encoding and lc_ctype, are handled specially
89878987
* outside the serialize/restore procedure. Therefore, SerializeGUCState()
89888988
* never sends these, and RestoreGUCState() never changes them.
8989+
*
8990+
* Role is a special variable in the sense that its current value can be an
8991+
* invalid value and there are multiple ways by which that can happen (like
8992+
* after setting the role, someone drops it). So we handle it outside of
8993+
* serialize/restore machinery.
89898994
*/
89908995
static bool
89918996
can_skip_gucvar(struct config_generic *gconf)
89928997
{
89938998
return gconf->context == PGC_POSTMASTER ||
8994-
gconf->context == PGC_INTERNAL || gconf->source == PGC_S_DEFAULT;
8999+
gconf->context == PGC_INTERNAL || gconf->source == PGC_S_DEFAULT ||
9000+
strcmp(gconf->name, "role") == 0;
89959001
}
89969002

89979003
/*
@@ -9252,27 +9258,14 @@ SerializeGUCState(Size maxsize, char *start_address)
92529258
Size actual_size;
92539259
Size bytes_left;
92549260
int i;
9255-
int i_role = -1;
92569261

92579262
/* Reserve space for saving the actual size of the guc state */
92589263
Assert(maxsize > sizeof(actual_size));
92599264
curptr = start_address + sizeof(actual_size);
92609265
bytes_left = maxsize - sizeof(actual_size);
92619266

92629267
for (i = 0; i < num_guc_variables; i++)
9263-
{
9264-
/*
9265-
* It's pretty ugly, but we've got to force "role" to be initialized
9266-
* after "session_authorization"; otherwise, the latter will override
9267-
* the former.
9268-
*/
9269-
if (strcmp(guc_variables[i]->name, "role") == 0)
9270-
i_role = i;
9271-
else
9272-
serialize_variable(&curptr, &bytes_left, guc_variables[i]);
9273-
}
9274-
if (i_role >= 0)
9275-
serialize_variable(&curptr, &bytes_left, guc_variables[i_role]);
9268+
serialize_variable(&curptr, &bytes_left, guc_variables[i]);
92769269

92779270
/* Store actual size without assuming alignment of start_address. */
92789271
actual_size = maxsize - bytes_left - sizeof(actual_size);

‎src/include/utils/guc.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,7 @@ extern bool log_btree_build_stats;
245245

246246
extern PGDLLIMPORT bool check_function_bodies;
247247
extern bool default_with_oids;
248+
extern bool session_auth_is_superuser;
248249

249250
extern int log_min_error_statement;
250251
extern int log_min_messages;

‎src/test/regress/expected/select_parallel.out

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -508,6 +508,22 @@ SELECT make_record(x) FROM (SELECT generate_series(1, 5) x) ss ORDER BY x;
508508

509509
ROLLBACK TO SAVEPOINT settings;
510510
DROP function make_record(n int);
511+
-- test the sanity of parallel query after the active role is dropped.
512+
drop role if exists regress_parallel_worker;
513+
NOTICE: role "regress_parallel_worker" does not exist, skipping
514+
create role regress_parallel_worker;
515+
set role regress_parallel_worker;
516+
reset session authorization;
517+
drop role regress_parallel_worker;
518+
set force_parallel_mode = 1;
519+
select count(*) from tenk1;
520+
count
521+
-------
522+
10000
523+
(1 row)
524+
525+
reset force_parallel_mode;
526+
reset role;
511527
-- to increase the parallel query test coverage
512528
SAVEPOINT settings;
513529
SET LOCAL force_parallel_mode = 1;

‎src/test/regress/sql/select_parallel.sql

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,17 @@ SELECT make_record(x) FROM (SELECT generate_series(1, 5) x) ss ORDER BY x;
201201
ROLLBACK TO SAVEPOINT settings;
202202
DROP function make_record(n int);
203203

204+
-- test the sanity of parallel query after the active role is dropped.
205+
drop role if exists regress_parallel_worker;
206+
create role regress_parallel_worker;
207+
set role regress_parallel_worker;
208+
reset session authorization;
209+
drop role regress_parallel_worker;
210+
set force_parallel_mode = 1;
211+
select count(*) from tenk1;
212+
reset force_parallel_mode;
213+
reset role;
214+
204215
-- to increase the parallel query test coverage
205216
SAVEPOINT settings;
206217
SET LOCAL force_parallel_mode = 1;

0 commit comments

Comments
 (0)