| From: | shihao zhong <zhong950419(at)gmail(dot)com> |
|---|---|
| To: | Antonin Houska <ah(at)cybertec(dot)at> |
| Cc: | Nikolay Samokhvalov <nik(at)postgres(dot)ai>, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, 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-17 01:07:51 |
| Message-ID: | CAGRkXqQwu8gFrpPXAkFSTOOK0bmf1MBSuLR6wENQH-JwcY4hRQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi Bharath,
I reviewed v5 patch by patch. v6 is attached
0001
It works. I made fork() really fail with a low RLIMIT_NPROC. On master,
REPACK (CONCURRENTLY) waits until statement_timeout. With 0001 it fails
at once with "REPACK decoding worker failed to start".
One problem. stop_repack_decoding_worker() now calls dsm_detach() before
it waits for the worker to exit. If the worker has mapped the segment
but not yet called SharedFileSetAttach(), the detach destroys the file
set, and the worker fails with
ERROR: could not attach to a SharedFileSet that is already destroyed
I hit this 12 times in 1200 cancelled REPACKs. Only shm_mq_detach() has
to come before the wait to fix the deadlock, so v6-0001 moves
dsm_detach() back after the wait, as on master. That gives 0 in 1200.
0002
Looks good. It follows parallel.c, and detaching before signalling is
needed, since with the old order the backend can read the queue too
early and never look again. With 0002 in, polling the worker status in
the wait loops, as Nikolay's patch does, is not needed.
No false alarms in 100 REPACK (CONCURRENTLY) runs with concurrent updates
and the owner's client_min_messages at debug1 and debug5, so the worker
often blocks on a full queue. pg_terminate_backend() on the worker gives
the backend the worker's own error at once.
0003
Please keep it. I checked that it catches each fix. Without the attach
wait, without the lost connection error, or with the queue detached
after the wait again, the test no longer passes.
One problem. The REPACK calls have no timeout, so a regression hangs
the test instead of failing it. Under meson it prints nothing until the
test is killed, and under make check nothing stops it. v6-0003 passes
timeout_default to those calls, so it now fails with "psql timed out"
at the right line. The rest of the test already waits with
timeout_default in poll_query_until(), pump_until() and
background_psql(), so this adds no new risk on slow machines.
0004
Looks fine. It matches what parallel.c does.
Thanks,
Shihao
| Attachment | Content-Type | Size |
|---|---|---|
| v6-0001-Fix-hang-and-deadlock-in-concurrent-REPACK-worker.patch | application/x-patch | 10.0 KB |
| v6-0002-Detect-premature-exit-of-the-REPACK-decoding-work.patch | application/x-patch | 7.3 KB |
| v6-0003-Add-tests-for-a-REPACK-decoding-worker-that-goes-.patch | application/x-patch | 17.0 KB |
| v6-0004-Clear-the-REPACK-message-flag-when-there-is-nothi.patch | application/octet-stream | 2.2 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Richard Guo | 2026-09-17 01:14:01 | ERROR: too late to create a new PlaceHolderInfo |
| Previous Message | Jacob Champion | 2026-09-16 23:50:59 | Re: POC: Carefully exposing information without authentication |