On Wed, 23 Feb 2022 at 15:51, Andres Freund <and...@anarazel.de> wrote:
> The tests fail:
> +ERROR: invalid value for parameter "session_authorization": "andres"
> +CONTEXT: while setting parameter "session_authorization" to "andres"
> +parallel worker
> +while scanning relation "public.pvactst"
>
> but that's easily worked around. In fact, there's already code to do so, it
> just is lacking awareness of session_authorization:
I was experimenting with the attached patch which just has
check_session_authorization() return true when not in transaction and
when InitializingParallelWorker is true. It looks like there are
already some other check functions looking at the value of
InitializingParallelWorker (e.g. check_transaction_read_only(),
check_role())
With that done, I do not see any other errors when calling
RestoreGUCState() while not in transaction. The parallel worker seems
to pick up the session_authorization GUC correctly using the same
debug_parallel_query test that you tried out.
On looking at the other check functions, I noted down the following as
ones that may behave differently when called while not in transaction:
check_transaction_read_only --- checks InitializingParallelWorker
check_transaction_deferrable -- checks
IsSubTransaction/FirstSnapshotSet, returns true if not
check_client_encoding -- Checks IsTransactionState(). Behavioural change
check_default_table_access_method -- accesses catalog if in xact.
Behavioural change
check_default_tablespace -- accesses catalog if in xact. Behavioural change
check_temp_tablespaces -- accesses catalog if in xact. Behavioural change
check_role -- Returns false if not in xact. Handled specially in
RestoreGUCState()
check_session_authorization -- Modify by patch to return true when not
in xact and parallel worker starting
check_timezone -- calls interval_in. Can't see any catalog lookup there.
check_default_text_search_config -- Checks in IsTransactionState()
check_transaction_isolation -- Check in IsTransactionState()
It seems that RestoreLibraryState() was called while in transaction so
that extension GUCs could be initialized before restoring the GUCs.
So there could be repercussions for extensions if we did this.
Does anyone see any reasons why we can't fix this with the attached?
David
diff --git a/src/backend/access/transam/parallel.c
b/src/backend/access/transam/parallel.c
index 194a1207be..7c39903249 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -1430,13 +1430,18 @@ ParallelWorkerMain(Datum main_arg)
* variables.
*/
libraryspace = shm_toc_lookup(toc, PARALLEL_KEY_LIBRARY, false);
- StartTransactionCommand();
RestoreLibraryState(libraryspace);
- /* Restore GUC values from launching backend. */
+ /*
+ * Restore GUC values from launching backend. We want to do this
without
+ * starting a transaction as some GUC check hook functions require
catalog
+ * access. It may be unsafe to access the catalogs as, for example,
+ * reading buffer to load a catalog page may require writing dirty
buffers
+ * and we don't want to do that until I/O related GUCs such as fsync are
+ * correctly set.
+ */
gucspace = shm_toc_lookup(toc, PARALLEL_KEY_GUC, false);
RestoreGUCState(gucspace);
- CommitTransactionCommand();
/* Crank up a transaction state appropriate to a parallel worker. */
tstatespace = shm_toc_lookup(toc, PARALLEL_KEY_TRANSACTION_STATE,
false);
diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index c361bb2079..a52ed5fa75 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -813,6 +813,14 @@ check_session_authorization(char **newval, void **extra,
GucSource source)
if (!IsTransactionState())
{
+ /*
+ * If we're a parallel worker being started up, there's no need
to
+ * verify the role exists. We'll end up running the same
snapshot as
+ * the leader process anyway so the role must exist.
+ */
+ if (InitializingParallelWorker)
+ return true;
+
/*
* Can't do catalog lookups, so fail. The result of this is
that
* session_authorization cannot be set in postgresql.conf,
which seems
