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

On Mon, Aug 31, 2026 at 11:14 AM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Mon, Aug 31, 2026 at 3:31 AM Antonin Houska <ah(at)cybertec(dot)at> wrote:
> >
> > Thanks for the fix(es). One thing I'm not sure I understand is:
>
> Thanks for reviewing.
>
> > @@ -3851,9 +3900,11 @@ ProcessRepackMessages(void)
> >
> > /*
> > * Nothing to do if we haven't launched the worker yet or have already
> > - * terminated it.
> > + * terminated it. stop_repack_decoding_worker() detaches the error queue
> > + * before clearing decoding_worker, so also bail out once error_mqh is
> > + * gone.
> > */
> > - if (decoding_worker == NULL)
> > + if (decoding_worker == NULL || decoding_worker->error_mqh == NULL)
> > return;
> >
> > /*
> >
> > I don't think that stop_repack_decoding_worker() can clear ->error_mqh w/o
> > also clearing decoding_worker.
>
> With the patch, there's a window where stop_repack_decoding_worker()
> sets the error queue pointer to NULL before setting the worker's
> shared memory pointer to NULL. I would like to keep this check.|
>
> > Other than that, I'm not sure you need to mention the condition variable in
> > the comments. And maybe even the mentions of parallel workers are not
> > necessary.
>
> Reworded the comments.
>
> > > While here, I noticed that the same wait event is used for both the
> > > worker startup wait and the file export wait. Ideally these would have
> > > separate wait events, but given that the startup wait is typically
> > > very short, reusing the same one seems fine.
> >
> > I think that initially I also considered this situation not worth a new wait
> > event, but I probably had missed an existing one:
> > WAIT_EVENT_BGWORKER_STARTUP. It's already used for multiple workers, so we
> > could perhaps use it here.
>
> I think using WAIT_EVENT_BGWORKER_STARTUP is fine, because if a worker
> is ever stuck here, one can look at pg_stat_activity to tell whether
> this is the REPACK decoding worker. I've done that in the 0002 patch.
>
> Please find the attached v2 patches.

I've reviewed the patch and I have two questions:

+ /*
+ * 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?

---
+ if (status == BGWH_STOPPED)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("REPACK decoding worker failed to start"),
+ errhint("More details may be available in the
server log."));
+ if (status == BGWH_POSTMASTER_DIED)

parallel.c handles BGWH_STOPPED differently; it checks that the worker
stopped without attaching to the error queue. IIUC if the worker
stopped after attaching to the error queue, an error message should
arrive at the leader and the leader could handle it in the next CFI.
Is there any reason why start_repack_decoding_worker() handles it
differently?

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com