Re: BUG #18545: \dt breaks transaction, calling error when executed in SET SESSION AUTHORIZATION - Mailing list pgsql-bugs
From | Tom Lane |
---|---|
Subject | Re: BUG #18545: \dt breaks transaction, calling error when executed in SET SESSION AUTHORIZATION |
Date | |
Msg-id | 72802.1721415837@sss.pgh.pa.us Whole thread Raw |
In response to | BUG #18545: \dt breaks transaction, calling error when executed in SET SESSION AUTHORIZATION (PG Bug reporting form <noreply@postgresql.org>) |
Responses |
Re: BUG #18545: \dt breaks transaction, calling error when executed in SET SESSION AUTHORIZATION
|
List | pgsql-bugs |
PG Bug reporting form <noreply@postgresql.org> writes: > postgres=# BEGIN; > BEGIN > postgres=*# CREATE USER regress_priv_user8; > CREATE ROLE > postgres=*# SET SESSION AUTHORIZATION regress_priv_user8; > SET > postgres=*> SET LOCAL debug_parallel_query = 1; > SET > postgres=*> \dt+; > ERROR: 22023: role "regress_priv_user8" does not exist > CONTEXT: while setting parameter "session_authorization" to "regress_priv_user8" > parallel worker So this has exactly nothing to do with \dt+; any parallel query will hit it. The problem is that parallel workers do RestoreGUCState() before they've restored the leader's snapshot. Thus, in this example where session_authorization refers to an uncommitted pg_authid entry, the workers don't see that entry. It seems likely that similar failures are possible with other GUCs that perform catalog lookups. I experimented with two different ways to fix this: 1. Run RestoreGUCState() outside a transaction, thus preventing catalog lookups. Assume that individual GUC check hooks that would wish to do a catalog lookup will cope. Unfortunately, some of them don't and would need fixed; check_role and check_session_authorization for two. 2. Delay RestoreGUCState() into the parallel worker's main transaction, after we've restored the leader's snapshot. This turns out to break a different set of check hooks, notably check_transaction_deferrable. I think that the blast radius of option 2 is probably smaller than option 1's, because it should only matter to check hooks that think they should run before the transaction has set a snapshot, and there are few of those. check_transaction_read_only already had a guard, but I added similar ones to check_transaction_isolation and check_transaction_deferrable. The attached draft patch also contains changes to prevent check_session_authorization from doing anything during parallel worker startup. That's left over from experimenting with option 1, and is not strictly necessary with option 2. I left it in anyway because it's saving some unnecessary work. (For some reason, check_role seems not to fail if you modify the test case to use SET ROLE. I did not figure out why not. I kind of want to modify check_role to be a no-op too when InitializingParallelWorker, but did not touch that here pending more investigation.) Another thing I'm wondering about is whether to postpone RestoreLibraryState similarly. Its current placement is said to be "before restoring GUC values", so it looks a little out of place now. Moving it into the main transaction would save one StartTransactionCommand/CommitTransactionCommand pair during parallel worker start, which is worth something. But I think the real argument for it is that if any loaded libraries try to do catalog lookups during load, we'd rather that they see the same catalog state the leader does. As against that, it feels like there's a nonzero risk of breaking some third-party code if we move that call. Thoughts? regards, tom lane diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c index 8613fc6fb5..06331d906a 100644 --- a/src/backend/access/transam/parallel.c +++ b/src/backend/access/transam/parallel.c @@ -1410,10 +1410,6 @@ ParallelWorkerMain(Datum main_arg) libraryspace = shm_toc_lookup(toc, PARALLEL_KEY_LIBRARY, false); StartTransactionCommand(); RestoreLibraryState(libraryspace); - - /* Restore GUC values from launching backend. */ - gucspace = shm_toc_lookup(toc, PARALLEL_KEY_GUC, false); - RestoreGUCState(gucspace); CommitTransactionCommand(); /* Crank up a transaction state appropriate to a parallel worker. */ @@ -1455,6 +1451,14 @@ ParallelWorkerMain(Datum main_arg) */ InvalidateSystemCaches(); + /* + * Restore GUC values from launching backend. We can't do this earlier, + * because GUCs that do catalog lookups (such as session_authorization) + * need to see the same database state as the leader. + */ + gucspace = shm_toc_lookup(toc, PARALLEL_KEY_GUC, false); + RestoreGUCState(gucspace); + /* * Restore current role id. Skip verifying whether session user is * allowed to become this role and blindly restore the leader's state for diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c index 9345131711..b2a00ae0f0 100644 --- a/src/backend/commands/variable.c +++ b/src/backend/commands/variable.c @@ -577,14 +577,16 @@ check_transaction_read_only(bool *newval, void **extra, GucSource source) * We allow idempotent changes at any time, but otherwise this can only be * changed in a toplevel transaction that has not yet taken a snapshot. * - * As in check_transaction_read_only, allow it if not inside a transaction. + * As in check_transaction_read_only, allow it if not inside a transaction, + * or if restoring state in a parallel worker. */ bool check_transaction_isolation(int *newval, void **extra, GucSource source) { int newXactIsoLevel = *newval; - if (newXactIsoLevel != XactIsoLevel && IsTransactionState()) + if (newXactIsoLevel != XactIsoLevel && + IsTransactionState() && !InitializingParallelWorker) { if (FirstSnapshotSet) { @@ -619,6 +621,10 @@ check_transaction_isolation(int *newval, void **extra, GucSource source) bool check_transaction_deferrable(bool *newval, void **extra, GucSource source) { + /* Just accept the value when restoring state in a parallel worker */ + if (InitializingParallelWorker) + return true; + if (IsSubTransaction()) { GUC_check_errcode(ERRCODE_ACTIVE_SQL_TRANSACTION); @@ -811,6 +817,15 @@ check_session_authorization(char **newval, void **extra, GucSource source) if (*newval == NULL) return true; + /* + * Do nothing if initializing a parallel worker: ParallelWorkerMain is + * responsible for setting the active role. We just need to absorb the + * leader's value of session_authorization in case some user code looks at + * it. + */ + if (InitializingParallelWorker) + return true; + if (!IsTransactionState()) { /* @@ -886,7 +901,7 @@ assign_session_authorization(const char *newval, void *extra) { role_auth_extra *myextra = (role_auth_extra *) extra; - /* Do nothing for the boot_val default of NULL */ + /* Do nothing for the boot_val default of NULL, and in parallel workers */ if (!myextra) return;
pgsql-bugs by date: