Thread

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

Андрей Рачицкий <therealgofman@mail.ru> — 2024-07-29T08:24:48Z

Hi, Tom! Thank you for work on the subject.  After applying patch, problem is no longer reproducible.
 
---
Best regards,
Andrey Rachitskiy
Postgres Professional:  http://postgrespro.com
  
>Суббота, 20 июля 2024, 0:04 +05:00 от Tom Lane <tgl@sss.pgh.pa.us>:
> 
>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
>