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

1 files changed, 26 insertions, 21 deletions

diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index f44d942aa4d..6ba6d08b241 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -691,6 +691,24 @@ check_client_encoding(char **newval, void **extra, GucSource source)
 	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
 	 * starting up, it will return "OK" anyway, and InitializeClientEncoding
@@ -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)