Race conditions during parallel worker (unclean) exit

In [1] I noted that ERROR like "lost connection to ... worker" (currently
issued in ProcessParallelMessages()) might also be appropriate for REPACK
(CONCURRENTLY). While I'm still not sure about that, I suspect that something
is wrong about handling unclean exits in parallel.c.

According to the header comment, ParallelWorkerShutdown() should handle cases
like direct call of proc_exit(). By adding one like this

diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c
index 17fcd246b0c..e8700ce8594 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -1385,6 +1385,7 @@ ParallelWorkerMain(Datum main_arg)
* Hooray! Primary initialization is complete. Now, we need to set up our
* backend-local state to match the original backend.
*/
+ proc_exit(1);

/*
* Join locking group. We must do this before anything that could try to

and by running the following

begin;
create table a(i int);
set parallel_setup_cost to 0;
set parallel_tuple_cost to 0;
set min_parallel_table_scan_size to 0;
set min_parallel_index_scan_size to 0;
set max_parallel_workers_per_gather to 1;
set parallel_leader_participation to off;
set debug_parallel_query to 'regress';
table a;
rollback;

I can reproduce the expected behavior:

ERROR: lost connection to parallel worker

However, an additional sleep() call in ParallelWorkerShutdown()

diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c
index 17fcd246b0c..1a05f16e0bc 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -1618,7 +1618,7 @@ ParallelWorkerShutdown(int code, Datum arg)
SendProcSignal(ParallelLeaderPid,
PROCSIG_PARALLEL_MESSAGE,
ParallelLeaderProcNumber);
-
+ sleep(1);
dsm_detach((dsm_segment *) DatumGetPointer(arg));
}

appears to cause race conditions. What I see is that the leader process runs
ProcessParallelMessages() while the worker is still attached to the error
queue, so the leader gets SHM_MQ_WOULD_BLOCK when trying to read from the
queue. Thus the ERROR is not raised.

Moreover, the leader, after having seen the worker detached from the tuple
queue (TupleQueueReaderNext) a bit later, starts executor cleanup and, as the
worker does not send signals anymore, it gets stuck in
WaitForParallelWorkersToExit().

Attached is what might be a fix, but I'm not sure if the Terminate message is
appropriate even if the worker in fact didn't finish with success.

[1] https://www.postgresql.org/message-id/32123.1788806780%40localhost

--
Antonin Houska
Web: https://www.cybertec-postgresql.com