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

PG Bug reporting form <noreply(at)postgresql(dot)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