Re: BUG #18545: \dt breaks transaction, calling error when executed in SET SESSION AUTHORIZATION - Mailing list pgsql-bugs

I wrote:
>> So I pushed the core bug fix, and I'll take another look at
>> that part later.

> ... or not; the buildfarm didn't like that much.  It'll
> have to wait till after these releases, because I'm
> overdue to get to work on the release notes.

The reason the buildfarm found something I'd missed in testing
is that I didn't run check-world with debug_parallel_query set,
which was a bad idea for a patch messing with parallel query
mechanics :-(.  Mea culpa.

However, what the farm found is that assign_client_encoding
is flat-out broken.  It's ignoring the first commandment
for GUC hooks, which is "Thy assign hooks shalt not fail".
(If we didn't need that, there wouldn't be a separation
between check hooks and assign hooks in the first place.)

Because it's throwing an error at the wrong time, it spits
up if it sees an (irrelevant) rollback of client_encoding
during cleanup of a failed parallel worker.  We didn't see
this before because GUCRestoreState was being run in a separate
mini-transaction that (usually at least) doesn't fail.

The attached correction basically just moves that test into
check_client_encoding where it should have been to begin with.
After applying this, I can un-revert f5f30c22e and everything
passes.

However, this episode definitely gives me pause about back-patching
f5f30c22e as I did before.  It seems not impossible that there
are extensions with similarly mis-coded assign hooks, and if
so those are going to need to be fixed.  (I did check that none
of the other core GUCs have this problem.  assign_recovery_target
and friends would, except that they are for PGC_POSTMASTER variables
that won't be getting changed by GUCRestoreState.  Anyway they're a
known kluge, and this patch isn't making it worse.)  So we'd better
treat this as a minor API change, which means we probably shouldn't
put it in stable branches.

What I'm currently thinking is to apply in HEAD and perhaps v17,
but not further back.  Given that this bug has existed since
the beginning of parallel query yet wasn't reported till now,
it's not sufficiently problematic to take any risk for in
stable branches.

Any opinions about whether it's too late to do this in v17?
Post-beta3 is pretty late, for sure, but maybe we could get
away with it.  And we are fixing a bug here.

            regards, tom lane

diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index f44d942aa4..6ba6d08b24 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -690,6 +690,24 @@ check_client_encoding(char **newval, void **extra, GucSource source)
     /* Get the canonical name (no aliases, uniform case) */
     canonical_name = pg_encoding_to_char(encoding);

+    /*
+     * Parallel workers send data to the leader, not the client.  They always
+     * send data using the database encoding; therefore, we should never
+     * actually change the client encoding in a parallel worker.  However,
+     * during parallel worker startup, we want to accept the leader's
+     * client_encoding setting so that anyone who looks at the value in the
+     * worker sees the same value that they would see in the leader.  A change
+     * other than during startup, for example due to a SET clause attached to
+     * a function definition, should be rejected, as there is nothing we can
+     * do inside the worker to make it take effect.
+     */
+    if (IsParallelWorker() && !InitializingParallelWorker)
+    {
+        GUC_check_errcode(ERRCODE_INVALID_TRANSACTION_STATE);
+        GUC_check_errdetail("Cannot change \"client_encoding\" during a parallel operation.");
+        return false;
+    }
+
     /*
      * If we are not within a transaction then PrepareClientEncoding will not
      * be able to look up the necessary conversion procs.  If we are still
@@ -700,11 +718,15 @@ check_client_encoding(char **newval, void **extra, GucSource source)
      * It seems like a bad idea for client_encoding to change that way anyhow,
      * so we don't go out of our way to support it.
      *
+     * In a parallel worker, we might as well skip PrepareClientEncoding since
+     * we're not going to use its results.
+     *
      * Note: in the postmaster, or any other process that never calls
      * InitializeClientEncoding, PrepareClientEncoding will always succeed,
      * and so will SetClientEncoding; but they won't do anything, which is OK.
      */
-    if (PrepareClientEncoding(encoding) < 0)
+    if (!IsParallelWorker() &&
+        PrepareClientEncoding(encoding) < 0)
     {
         if (IsTransactionState())
         {
@@ -758,28 +780,11 @@ assign_client_encoding(const char *newval, void *extra)
     int            encoding = *((int *) extra);

     /*
-     * Parallel workers send data to the leader, not the client.  They always
-     * send data using the database encoding.
+     * In a parallel worker, we never override the client encoding that was
+     * set by ParallelWorkerMain().
      */
     if (IsParallelWorker())
-    {
-        /*
-         * During parallel worker startup, we want to accept the leader's
-         * client_encoding setting so that anyone who looks at the value in
-         * the worker sees the same value that they would see in the leader.
-         */
-        if (InitializingParallelWorker)
-            return;
-
-        /*
-         * A change other than during startup, for example due to a SET clause
-         * attached to a function definition, should be rejected, as there is
-         * nothing we can do inside the worker to make it take effect.
-         */
-        ereport(ERROR,
-                (errcode(ERRCODE_INVALID_TRANSACTION_STATE),
-                 errmsg("cannot change \"client_encoding\" during a parallel operation")));
-    }
+        return;

     /* We do not expect an error if PrepareClientEncoding succeeded */
     if (SetClientEncoding(encoding) < 0)