Re: REPACK (CONCURRENTLY) backend waits indefinitely when decoding worker fails to start

Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:

> + /*
> + * Associate the worker's handle with the error queue, just as if it had
> + * been passed to shm_mq_attach(); we passed NULL there because the worke
> + * did not exist yet. This lets ProcessRepackMessages() notice the worker
> + * is gone instead of blocking on the queue.
> + */
> + shm_mq_set_handle(decoding_worker->error_mqh, decoding_worker->handle);
> +
>
> The second sentence starting with "This lets ..." is unclear to me.
> ProcessRepackMessage() receives the message with nowait, no?

Although it probably does not break anything if we pass the handle to
shm_mq_set_handle() unnecessarily, I'm thinking if it's necessary for an error
queue.

AFAIU the purpose of the handle is to realize that it makes no sense to wait
for the background worker to attach to the queue. Once the worker has
attached, the worker's handle is no longer needed to detect that the worker
detached: the already-attached worker itself is supposed to call
shm_mq_detach().

If I use the parallel worker for reference, I think the leader should not be
asked (via the PROCSIG_PARALLEL_MESSAGE signal) to read from the error queue
until the worker has attached and redirected its messages to the queue - see
in ParallelWorkerMain():

mqh = shm_mq_attach(mq, seg, NULL);
pq_redirect_to_shm_mq(seg, mqh);

Therefore, the leader should not need the worker's handle.

I also thought about the other source of the PROCSIG_PARALLEL_MESSAGE signal,
a few lines above the call to shm_mq_attach():

before_shmem_exit(ParallelWorkerShutdown, PointerGetDatum(seg));

If the worker called proc_exit() after this, but before the call of
shm_mq_attach(), the worker handle might be useful for the leader to conclude
that the worker hasn't detached yet. However, the coding of
ProcessParallelMessages() is such that shm_mq_receive() can return
SHM_MQ_WOULD_BLOCK in such a case, so the ERROR "lost connection to parallel
worker" is not raised anyway.

While thinking about that ERROR, I realize that I might have misunderstood its
purpose, and therefore it's not present in
ProcessRepackMessage(). Nevertheless, I'm still not sure I understand when
exactly should that ERROR be raised. Per commit 2badb5afb8, the function
ParallelWorkerShutdown() is related. However, AFAICS, when the worker code
calls proc_exit(), the ParallelWorkerShutdown() callback can send the
PROCSIG_PARALLEL_MESSAGE signal while the worker is still attached to the
error queue. In that case, the ERROR is not raised.

I'm probably missing something. Thanks in advance for any hint.

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