Clean up handling of client_encoding GUC in parallel workers.

The previous coding here threw an error from assign_client_encoding
if it was invoked in a parallel worker.  That's a very fundamental
violation of the GUC hook API: assign hooks must not throw errors.
The place to complain is in the check hook, so move the test to
there, and use the regular check-hook API (ie return false) to
report it.

The reason this coding is a problem is that it breaks GUC rollback,
which may occur after we leave InitializingParallelWorker state.
That case seems not actually reachable before now, but commit
f5f30c22e made it reachable, so we need to fix this before that
can be un-reverted.

In passing, improve the commentary in ParallelWorkerMain, and
add a check for failure of SetClientEncoding.  That's another
case that can't happen now but might become possible after
foreseeable code rearrangements (notably, if the shortcut of
skipping PrepareClientEncoding stops being OK).

Discussion: https://postgr.es/m/18545-feba138862f19aaa@postgresql.org

diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c
index 8613fc6fb547c05d30d01a44282dba2b3f2178a3..27551566ac421933ea0b6d35cd0ecd30aae695a7 100644 (file)
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -1398,9 +1398,13 @@ ParallelWorkerMain(Datum main_arg)
 
    /*
     * Set the client encoding to the database encoding, since that is what
-    * the leader will expect.
+    * the leader will expect.  (We're cheating a bit by not calling
+    * PrepareClientEncoding first.  It's okay because this call will always
+    * result in installing a no-op conversion.  No error should be possible,
+    * but check anyway.)
     */
-   SetClientEncoding(GetDatabaseEncoding());
+   if (SetClientEncoding(GetDatabaseEncoding()) < 0)
+       elog(ERROR, "SetClientEncoding(%d) failed", GetDatabaseEncoding());
 
    /*
     * Load libraries that were loaded by original backend.  We want to do

diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index f44d942aa4d45cd3eddfc2b54326911ae6bd8538..6ba6d08b24123df2ed8254f89072d31fe13706fe 100644 (file)
--- 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)