| From: | Nikolay Samokhvalov <nik(at)postgres(dot)ai> |
|---|---|
| To: | Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
| Cc: | Antonin Houska <ah(at)cybertec(dot)at>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Nathan Bossart <nathandbossart(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, alvherre(at)kurilemu(dot)de |
| Subject: | Re: REPACK (CONCURRENTLY) backend waits indefinitely when decoding worker fails to start |
| Date: | 2026-09-13 18:37:49 |
| Message-ID: | CAM527d8cF-6azsRWOPaOUCmsdgMG=ene7-st91_NM7v0q3uLLg@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Mon, Sep 7, 2026 at 12:15 PM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> After thinking more about this, I simplified the handling. There are two
> things the backend needs to wait for. First, for the worker to come up and
> attach to the error message queue. Second, for the worker to set up the
> logical decoding machinery before it waits for snapshot export. The first
> wait catches fork failures and worker startup issues. I used similar logic
> to what parallel.c uses for this. The second wait catches failures that
> happen after the worker starts up and attaches to the error queue but
> before it finishes setting up the logical decoding machinery. I kept the
> shared memory initialized flag with the CV wait as-is for this.
Álvaro pointed me here from my thread:
https://postgr.es/m/CAM527d9A7FBXJn7A3w2VAPq0Gojm7tnUDFMh1uwJitGfi2kNeQ@mail.gmail.com
My AI harness tested v4. Silent worker exits still hang three
post-attachment waits. Mihail raised snapshot/replay liveness in
December 2025.
Snapshot repro on a test build configured with --enable-injection-points,
with v4 plus only the attached patch's test support:
create extension injection_points;
create table t (i int primary key);
insert into t select generate_series(1, 100);
set statement_timeout = '2s';
select injection_points_attach(
'repack-worker-before-snapshot-export',
'injection_points', 'injection_exit', null);
repack (concurrently) t;
This times out after worker exit. The attached patch applies atop v4.
As far as I can see, the current uncommitted patches cover overall 5 cases:
- startup before queue attach: Bharath v4
- full error queue teardown: Bharath v4
- silent exit after attach, before initialization: attached patch
- silent exit before snapshot export: attached patch
- silent exit before replay export: attached patch
Nik
On Sun, Sep 13, 2026 at 11:09 AM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Tue, Sep 8, 2026 at 12:17 AM Antonin Houska <ah(at)cybertec(dot)at> wrote:
> >
> > Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
> >
> > > On Fri, Sep 4, 2026 at 5:11 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > >
> > > After thinking more about this, I simplified the handling. There are two things the backend needs to wait for. First, for the worker to come up
> > > and attach to the error message queue. Second, for the worker to set up the logical decoding machinery before it waits for snapshot export.
> > > The first wait catches fork failures and worker startup issues. I used similar logic to what parallel.c uses for this. The second wait catches
> > > failures that happen after the worker starts up and attaches to the error queue but before it finishes setting up the logical decoding
> > > machinery. I kept the shared memory initialized flag with the CV wait as-is for this.
> > >
> > > Although the initialized flag wait may seem redundant with the snapshot export wait in get_initial_snapshot(), I would still keep it because it
> > > ensures the worker has fully set up the decoding before the backend proceeds.
> > >
> > > Dividing this into two separate waits (waiting for the worker to come up and attach to the error message queue, and then waiting for it to
> > > finish setup) makes the logic simpler, lets us reuse most of parallel.c's code, is easier to reason about, and fixes the hang issue without letting
> > > the backend reach the snapshot export wait with the worker not fully ready.
> >
> > Another reason for two separate waits is that two separate event types make
> > sense: WAIT_EVENT_BGWORKER_STARTUP and WAIT_EVENT_REPACK_WORKER_EXPORT.
>
> That's correct.
>
> > > Please find the attached v3 patch.
> >
> > Just two comments:
>
> Thanks for reviewing it.
>
> > * wait_for_repack_worker_to_attach() - as the decoding_worker variable is
> > declared static, this function does not necessarily need the argument.
>
> Removed the function parameter.
>
> > * Regarding comment: when the following is reached, the error message queue
> > has already been detached, so no implicit detaching should happen. Also,
> > there are no "other shared memory queues".
> >
> > + /*
> > + * If we have allocated a shared memory segment, detach it. This will
> > + * implicitly detach the error message queue, and any other shared memory
> > + * queues, stored there.
> > + */
> > + if (decoding_worker->seg != NULL)
> > + {
> > + dsm_detach(decoding_worker->seg);
> > + decoding_worker->seg = NULL;
> > + }
>
> I borrowed it from parallel.c. I agree it can be simplified, and I
> have done that.
>
> Please find the attached v4 patch.
>
> --
> Bharath Rupireddy
> Amazon Web Services: https://aws.amazon.com
| Attachment | Content-Type | Size |
|---|---|---|
| 0001-repack-worker-post-attachment-exit-minimal.patch | application/x-patch | 16.3 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Nikolay Samokhvalov | 2026-09-13 18:39:39 | Re: [PATCH] Avoid a REPACK worker startup hang |
| Previous Message | Andrew Dunstan | 2026-09-13 18:28:12 | Re: pg_get_*_ddl() needs a redesign |