| From: | Manu <manuelreyesbravo(at)gmail(dot)com> |
|---|---|
| To: | Sami Imseih <samimseih(dot)pg(at)gmail(dot)com> |
| Cc: | Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: Add REPACK progress phases for logical decoding setup |
| Date: | 2026-09-22 23:29:44 |
| Message-ID: | 179011978413.427813.14463910337689263262@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi Sami,
I checked v2. Both patches apply and build clean, and the two claims
the new documentation makes that a reader cannot easily verify are
true. Measurements below are on the tree with PROGRESS_DEBUG, which
logs every phase value a backend emits, so the sequences are recorded
rather than sampled.
1. "This phase is skipped when not in concurrent mode"
REPACK t phases emitted: 1 6 7 8
REPACK (CONCURRENTLY) t phases emitted: 9 10 1 7 5 6 8
9 and 10 appear only in the concurrent case, and they come before the
heap scan, as the table now describes.
2. catch-up "also covers waiting for the ACCESS EXCLUSIVE lock needed
to swap the files, so a command blocked by a conflicting lock stays
in this phase"
With a session holding the table open in a transaction and a REPACK
(CONCURRENTLY) behind it:
pg_stat_progress_repack.phase catch-up
pg_stat_activity Lock / relation
So it does stay in catch-up, and that is where an operator will find a
blocked REPACK.
3. The catversion bump
Confirmed missing, as you noted: catversion.h is still 202609152 in
v2-0002. Only flagging it so it does not get lost.
4. One thing the documentation still does not say
The phase table lists the phases in an order that is not the order
they happen in. From the run above, the concurrent sequence is
9 (enabling logical decoding)
10 (initializing logical decoding)
1 (seq scanning heap)
7 (rebuilding index)
5 (catch-up)
6 (swapping relation files)
8 (final cleanup)
but the table lists "rebuilding index" after "swapping relation
files", near the end. This is pre-existing - build_new_index() sets
REBUILD_INDEX and REPACK (CONCURRENTLY) reaches it before catch-up -
and not something v2 introduces. Since 0001 is specifically about
making this table less misleading for someone watching a long REPACK,
it might be worth either ordering the rows the way the phases occur or
saying that the order of the rows is not the order of execution.
Our own progress test module (the invariant checker from the debug
framework thread) passes on v2 without any change: it derives the
phase values from progress.h by prefix, so it picked up 9 and 10 on
its own.
Regards,
Manu
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Manu | 2026-09-22 23:32:27 | Re: Distinguish publication exclusions in object addresses |
| Previous Message | Melanie Plageman | 2026-09-22 23:12:20 | Re: WAL_LOG CREATE DATABASE strategy broken for non-standard page layouts |