Re: BUG #18545: \dt breaks transaction, calling error when executed in SET SESSION AUTHORIZATION

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;