public inbox for [email protected]help / color / mirror / Atom feed
[PATCH] Rename whitelist/blacklist in pgindent to additional/excluded 3+ messages / 3 participants [nested] [flat]
* [PATCH] Rename whitelist/blacklist in pgindent to additional/excluded @ 2021-01-05 00:10 Dagfinn Ilmari Mannsåker <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Dagfinn Ilmari Mannsåker @ 2021-01-05 00:10 UTC (permalink / raw) --- src/tools/pgindent/pgindent | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent index 4124d27dea..feb02067c5 100755 --- a/src/tools/pgindent/pgindent +++ b/src/tools/pgindent/pgindent @@ -54,12 +54,12 @@ $excludes ||= "$code_base/src/tools/pgindent/exclude_file_patterns" # some names we want to treat like typedefs, e.g. "bool" (which is a macro # according to <stdbool.h>), and may include some names we don't want # treated as typedefs, although various headers that some builds include -# might make them so. For the moment we just hardwire a whitelist of names -# to add and a blacklist of names to remove; eventually this may need to be +# might make them so. For the moment we just hardwire a list of names +# to add and a list of names to exclude; eventually this may need to be # easier to configure. Note that the typedefs need trailing newlines. -my @whitelist = ("bool\n"); +my @additional = ("bool\n"); -my %blacklist = map { +"$_\n" => 1 } qw( +my %excluded = map { +"$_\n" => 1 } qw( ANY FD_SET U abs allocfunc boolean date digit ilist interval iterator other pointer printfunc reference string timestamp type wrap ); @@ -134,11 +134,11 @@ sub load_typedefs } } - # add whitelisted entries - push(@typedefs, @whitelist); + # add additional entries + push(@typedefs, @additional); - # remove blacklisted entries - @typedefs = grep { !$blacklist{$_} } @typedefs; + # remove excluded entries + @typedefs = grep { !$excluded{$_} } @typedefs; # write filtered typedefs my $filter_typedefs_fh = new File::Temp(TEMPLATE => "pgtypedefXXXXX"); -- 2.29.2 --=-=-=-- ^ permalink raw reply [nested|flat] 3+ messages in thread
* BUG #18545: \dt breaks transaction, calling error when executed in SET SESSION AUTHORIZATION @ 2024-07-19 09:25 PG Bug reporting form <[email protected]> 2024-07-19 19:03 ` Re: BUG #18545: \dt breaks transaction, calling error when executed in SET SESSION AUTHORIZATION Tom Lane <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: PG Bug reporting form @ 2024-07-19 09:25 UTC (permalink / raw) To: [email protected]; +Cc: [email protected] The following bug has been logged on the website: Bug reference: 18545 Logged by: Andrey Rachitskiy Email address: [email protected] PostgreSQL version: 16.3 Operating system: Debian 12 Description: \dt breaks transaction, calling error when executed in SET SESSION AUTHORIZATION. This happens in two cases, with different SET. Case one: postgres@debian-test:~$ psql -U postgres psql (16.3) Type "help" for help. postgres=# \set VERBOSITY verbose postgres=# BEGIN; BEGIN postgres=*# CREATE USER regress_priv_user8; CREATE ROLE postgres=*# SET SESSION AUTHORIZATION regress_priv_user8; SET postgres=*> \dt+; Did not find any relations. 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 LOCATION: call_string_check_hook, guc.c:6734 postgres=!# \q Case two: postgres@debian-test:~$ psql -U postgres psql (16.3) Type "help" for help. postgres=# \set VERBOSITY verbose postgres=# BEGIN; BEGIN postgres=*# CREATE USER regress_priv_user8; CREATE ROLE postgres=*# SET SESSION AUTHORIZATION regress_priv_user8; SET postgres=*> \dt+ Did not find any relations. postgres=*> set local parallel_setup_cost = 0; SET postgres=*> set local min_parallel_table_scan_size = 0; SET postgres=*> \dt+ ERROR: 22023: role "regress_priv_user8" does not exist CONTEXT: while setting parameter "session_authorization" to "regress_priv_user8" parallel worker LOCATION: call_string_check_hook, guc.c:6734 postgres=!# \q ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: BUG #18545: \dt breaks transaction, calling error when executed in SET SESSION AUTHORIZATION 2024-07-19 09:25 BUG #18545: \dt breaks transaction, calling error when executed in SET SESSION AUTHORIZATION PG Bug reporting form <[email protected]> @ 2024-07-19 19:03 ` Tom Lane <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Tom Lane @ 2024-07-19 19:03 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; Robert Haas <[email protected]> PG Bug reporting form <[email protected]> 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 Attachments: [text/x-diff] fix-bug-18545-wip.patch (3.2K, ../../[email protected]/2-fix-bug-18545-wip.patch) download | inline diff: 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; ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2024-07-19 19:03 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-01-05 00:10 [PATCH] Rename whitelist/blacklist in pgindent to additional/excluded Dagfinn Ilmari Mannsåker <[email protected]> 2024-07-19 09:25 BUG #18545: \dt breaks transaction, calling error when executed in SET SESSION AUTHORIZATION PG Bug reporting form <[email protected]> 2024-07-19 19:03 ` Re: BUG #18545: \dt breaks transaction, calling error when executed in SET SESSION AUTHORIZATION Tom Lane <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox