Add a test for index_rebuild_count of REPACK (CONCURRENTLY)

Hello Manu,

On 2026-Sep-16, Manuel Reyes Bravo wrote:

Neither 4b445479f9e (TOAST index creation writing into
index_rebuild_count) nor 0765b48874a (the concurrent path not counting
its index builds) added a test, and both are easy to break again
without anyone noticing, since nothing checks the values the progress
views report.

I don't like this idea, because it adds no systematic mechanism to test
the progress-report feature as a whole. I don't see why REPACK should
be the place to start testing this. Also, injection points seem the
wrong tool for the job, even if you can achieve testing an increment of
a single progress counter within an existing test.

On the other hand, this proposed test uses the isolation framework,
which is by construction complicated enough. Not that this one
isolation test is particularly complicated; but other tests are, and we
do not benefit from added complexity that only supports testing a
feature unrelated to concurrency.

As I said in a reply to Fujii in the thread for the patch you replied to
in pgsql-committers, I think we need to come up with a test framework
specific to observing progress report counters. (Maybe, and I'm just
braindumping here, have them in debug mode print out a line for each
individual counter update that's made, so that a test file can
observe/match those lines somehow). That's more work upfront, but it
can allow us systematically test all the counters in a coherent way.

Thanks for spending time on this,

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Doing what he did amounts to sticking his fingers under the hood of the
implementation; if he gets his fingers burnt, it's his problem." (Tom Lane)

Hi Álvaro,

Álvaro Herrera <alvherre@kurilemu.de> wrote:

As I said in a reply to Fujii in the thread for the patch you replied to
in pgsql-committers, I think we need to come up with a test framework
specific to observing progress report counters. (Maybe, and I'm just
braindumping here, have them in debug mode print out a line for each
individual counter update that's made, so that a test file can
observe/match those lines somehow).

Understood, and I should have read that thread first: Adam's original
patch had a test for this, and you and Fujii had already dropped it.

I would like to work on the framework for v20, if nobody else is. Some
facts that make it look tractable:

- Every write to st_progress_param goes through backend_progress.c
(pgstat_progress_update_param, _incr_param, _parallel_incr_param,
_update_multi_param, plus start/end_command). There are 163 calls in
23 files, and none of them writes the array directly, so one hook
there sees every update.

- There is precedent for the switch in the DEVELOPER_OPTIONS trace_*
settings (trace_locks, trace_notify, trace_sort, ...).

- As far as I can see, the only test that checks progress values for
their own sake is COPY's, from a trigger that reads
pg_stat_progress_copy during the insert. That needs user code running
inside the command, so it cannot reach VACUUM, ANALYZE, CREATE INDEX or
REPACK. Two recovery TAP tests poll pg_stat_progress_basebackup and
pg_stat_progress_vacuum, but only to know when to act.

Before writing anything, three questions, so that I build what you have
in mind:

1. A runtime developer setting (say trace_progress, like trace_notify)
or something compiled in only for debug builds (like LOCK_DEBUG
around trace_locks)?

2. Should tests read the lines from the server log in TAP tests, or
from the client with client_min_messages in the regression suite?
Counters such as blocks scanned vary between runs, so I assume a test
would match phases and selected counters rather than every line.

3. One line per call, or only when a value actually changes?

The first users would be VACUUM and REPACK, including the two
index_rebuild_count cases from this week.

Regards,
Manu

Hello,

On 2026-Sep-16, Manuel Reyes Bravo wrote:

I would like to work on the framework for v20, if nobody else is.

Sounds good.

Some facts that make it look tractable:

- Every write to st_progress_param goes through backend_progress.c
(pgstat_progress_update_param, _incr_param, _parallel_incr_param,
_update_multi_param, plus start/end_command). There are 163 calls in
23 files, and none of them writes the array directly, so one hook
there sees every update.

Yep.

- There is precedent for the switch in the DEVELOPER_OPTIONS trace_*
settings (trace_locks, trace_notify, trace_sort, ...).

I think those are all pretty archaic, so I wouldn't necessarily base a
design on them.

Before writing anything, three questions, so that I build what you have
in mind:

1. A runtime developer setting (say trace_progress, like trace_notify)
or something compiled in only for debug builds (like LOCK_DEBUG
around trace_locks)?

I think a compile option is enough. We'll want a buildfarm animal that
runs with that option set, but things set up in such a way that the
(limited amount of) debug code is compiled out for regular developer
builds, so that this doesn't cause "meson test" to be any slower.

2. Should tests read the lines from the server log in TAP tests, or
from the client with client_min_messages in the regression suite?
Counters such as blocks scanned vary between runs, so I assume a test
would match phases and selected counters rather than every line.

No opinion on this. Maybe a good frame would be a TAP test that runs
REPACK/COPY/etc and then reads the debug output to see if the order of
phase switching is from A to B to C, and that block numbers in column
such-and-such are monotonically increasing within one phase, and that
they get back to 0 when changing to phase X, etc.

3. One line per call, or only when a value actually changes?

I think emitting a line when nothing has changed would be pointless
noise.

The first users would be VACUUM and REPACK, including the two
index_rebuild_count cases from this week.

Sure, as long as it's not restricted to only cover them.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/

On Wed, Sep 16, 2026 at 04:30:16PM +0200, Alvaro Herrera wrote:

I don't like this idea, because it adds no systematic mechanism to test
the progress-report feature as a whole. I don't see why REPACK should
be the place to start testing this. Also, injection points seem the
wrong tool for the job, even if you can achieve testing an increment of
a single progress counter within an existing test.

My feeling regarding v6-0003 echoes with yours. Using injection
points does not feel completely right to capture a dynamic state. We
could do something like a tracing facility perhaps? Run a set of SQL
commands, then check how much the progress has evolved over the full
sequence? To me, progress coverage should not just check one point in
time of the progress, but a succession of expected numbers. And this
does not have to involve concurrent activity.

Saying that, v16-0003 is only touching the module injection_points
with no footprint to the core backend. The exit callback is an
interesting thing to see, perhaps there could be a use for it. I am
not convinced by the oversized part to generate a forced NOTICE,
meaning duplication with the other existing callbacks.

As I said in a reply to Fujii in the thread for the patch you replied to
in pgsql-committers, I think we need to come up with a test framework
specific to observing progress report counters. (Maybe, and I'm just
braindumping here, have them in debug mode print out a line for each
individual counter update that's made, so that a test file can
observe/match those lines somehow). That's more work upfront, but it
can allow us systematically test all the counters in a coherent way.

Yeah. I've just had my eyes on a test two hours ago, related to the
VACUUM progress view, that had the idea to plug into a TAP test an
injection point wait and a psql background session to check the
progress data. That feels bloated to me, just to check *one* state of
the progress report.
--
Michael

Hi Álvaro, Michael,

Álvaro Herrera <alvherre@kurilemu.de> wrote:

Maybe a good frame would be a TAP test that runs
REPACK/COPY/etc and then reads the debug output to see if the order of
phase switching is from A to B to C, and that block numbers in column
such-and-such are monotonically increasing within one phase, and that
they get back to 0 when changing to phase X, etc.

Michael Paquier <michael@paquier.xyz> wrote:

To me, progress coverage should not just check one point in
time of the progress, but a succession of expected numbers. And this
does not have to involve concurrent activity.

Here is a first version. 0005 is the framework; 0001-0004 fix four
bugs it found the first time it ran over the regression suite.

0005 adds PROGRESS_DEBUG, a compile-time option in pg_config_manual.h.
With it, backend_progress.c logs every change to a backend's progress
state at LOG_SERVER_ONLY, and only changes:

progress start: VACUUM relid=16384
progress update: VACUUM relid=16384 0:1->2 8:0->2
progress end: VACUUM relid=16384

One pgstat_progress_update_multi_param() call is one line, since readers
see those values change together. Without the option the code is
compiled out: backend_progress.c compiles to the same machine code as on
master (compared with __LINE__ and __FILE__ fixed, 265 instructions).

A new module, src/test/modules/test_progress, reads those lines back.
Its ProgressCheck.pm replays each backend's trace and checks rules that
hold for every command: each change starts from the last value logged,
counters don't decrease or only go back to 0, done counters stay within
their totals, phases take defined values, and a command only writes its
own parameters. The test then runs COPY, CREATE INDEX [CONCURRENTLY], a
parallel GIN build, ANALYZE, VACUUM (with truncation, in several index
cycles, and parallel), REPACK (sorting, through an index, and
CONCURRENTLY) and base backups, and checks the succession of phases and
of index_rebuild_count, and some final values. ProgressCheck.pm also
describes every macro of commands/progress.h, and the test checks that
against the header on every build, so a new parameter can't be added
without saying what it is. Without PROGRESS_DEBUG that is all the test
does: it doesn't start a server, so it costs nothing in other builds.

Running make check with PROGRESS_DEBUG and ProgressCheck over the
postmaster log (1.34 million changes from about 3,600 commands) found:

0001 blocks_done of an index build's heap scan was reported as
blocks_total on the first block, then went back to 1.
heapam_scan_get_blocks_done() took the wrap-around branch when the
current block is the start block. Since ab0dfc961b6.

0002 When one command builds several indexes without progress for each
(REPACK, CREATE INDEX on a partitioned table), each build started
from the previous build's tuples_done, which could be above the new
tuples_total. index_build() only reset the AM counters when
reporting progress, since caec9d9fadf, while the AMs report them
anyway. It now resets them in every build, but still sets the phase
only when asked, so it keeps what 4b445479f9e fixed.

0003 num_dead_item_ids and dead_tuple_bytes are documented as collected
since the last index vacuum cycle, but dead_items_reset() didn't
report the reset, so the view kept the previous cycle's values
until the next page with dead items. num_dead_tuples behaved the
same way before 667e65aac35.

0004 A parallel GIN build ended with tuples_done one above
tuples_total: the leader counted the last key's tuples again when
flushing them. Since 8492feb98f6.

With the four fixes, the same run has no violations. Removing any one
of them from the series makes the test fail, only in the checks for that
fix.

I also checked the framework against the two bugs that 4b445479f9e and
0765b48874a fixed last week, by reverting each of them on top of the
series. Without 4b445479f9e the test fails in every REPACK case,
because index_rebuild_count starts at 2 and then goes back to 1. Without
0765b48874a, REPACK (CONCURRENTLY) fails because index_rebuild_count is
never reported. With both reverted, each is still caught: the final
count happens to be right in that case, which is why the test checks the
succession rather than the last value.

Some things the trace shows that I did not treat as bugs, and on which
I'd like your opinion:

- Parameters written while no command is active. Index AMs report
their subphase and counts whether or not anyone asked.
reindex_relation() sets REPACK's index_rebuild_count for its other
callers too (TRUNCATE, REINDEX TABLE, and through finish_heap_swap()
ALTER TABLE and REFRESH MATERIALIZED VIEW, for which
finish_heap_swap() also sets REPACK's phase). And CREATE TABLE ...
PARTITION OF and ATTACH PARTITION report a whole CREATE INDEX for
the partition's index, phase and partitions_done included, without a
command. Nothing reads the parameters then, so ProgressCheck only
counts these writes. Should those callers stop writing, instead?

- REINDEX CONCURRENTLY starts CREATE INDEX again for each index, with
that index's table, which may be the TOAST table. ProgressCheck
accepts a command restarting itself, and flags only a different
command starting inside another one (which never happened).

Open questions:

1. The option is a plain #define (CPPFLAGS=-DPROGRESS_DEBUG with
configure, -Dc_args=-DPROGRESS_DEBUG with meson), and the test finds
it in pg_config --cppflags or --cflags, or in pg_config_manual.h.
Would you rather have a configure/meson option, like injection
points?

2. The trace is large: make check writes a 182 MB log with the option
against 3 MB without, and the tests took 2.6% longer in one run. 91% of the
lines are tuples_done in index builds, which is updated once per
tuple. That seems acceptable for an opt-in build, but runs of +1 of
the same counter could be folded into one line if you think the size
matters for a buildfarm animal.

3. ProgressCheck.pm lives in the module for now. If it's useful
elsewhere, it could go in src/test/perl.

With PROGRESS_DEBUG, check-world passes. The trace doesn't reach clients
(LOG_SERVER_ONLY), and standalone backends don't emit it, since they log
to their caller's stderr: initdb's test checks that it prints nothing
there. Without the option, check-world passes as on master.

The bugs fixed by 0001 and 0003 are in all supported branches, the one
fixed by 0004 in 18 and later, and the one fixed by 0002 only in 19. I
can post the fixes separately if that is easier.

Regards,
Manu

Hi,

While reviewing the REPACK progress patch with this framework, I noticed
something the framework could not see, and the attached patch teaches it
to.

A progress phase lives in three places, and nothing keeps them in sync:

src/include/commands/progress.h the value
src/backend/catalog/system_views.sql the text the user sees
doc/src/sgml/monitoring.sgml the row in the phase table

The framework already derived the values from progress.h. It now also
reads the other two and compares them with what the commands actually
report: a phase value the view or the table does not know about fails
test, and a phase first reached out of the documented order is reported.

The second case is not hypothetical. Running VACUUM, ANALYZE, CLUSTER,
CREATE INDEX [CONCURRENTLY], REINDEX [CONCURRENTLY], REPACK and REPACK
(CONCURRENTLY) through it, nine of the ten come out consistent, and one
does not:

REPACK (CONCURRENTLY):
enabling logical decoding -> initializing logical decoding ->
seq scanning heap -> rebuilding index -> catch-up ->
swapping relation files -> performing final cleanup

REPACK went from "rebuilding index" to "catch-up", but the
documentation lists "catch-up" before "rebuilding index"

So the row order of the REPACK phase table is not the order the phases
happen in. build_new_index() sets REBUILD_INDEX and the concurrent path
reaches it before catch-up. This is pre-existing, not something the
in-flight REPACK progress patches introduce, but it is exactly the kind
of thing someone reads that table to find out.

The obvious trap here is a command that loops. VACUUM goes back to
"scanning heap" for every round of index vacuuming, which is legitimate
and documented, and a naive order check would flag it. Only the first
arrival at a phase is considered, so a return to a phase already seen is
not reported. The test covers that: with maintenance_work_mem at the
minimum it gets a real two-round VACUUM

scanning heap -> vacuuming indexes -> vacuuming heap ->
scanning heap -> vacuuming indexes -> vacuuming heap ->
cleaning up indexes -> truncating heap -> performing final cleanup

and reports nothing for it.

The out-of-order case is reported rather than failed, because it is a
documentation bug and I did not want a test that fails on master until
that table is reordered. If you would rather have it fail, that is a
one-line change and then the REPACK table needs fixing first.

The two paths come from the Makefile, as PROGRESS_H already did,
so a phase added in all three places keeps the check quiet, and a phase
added in two of them does not.

Regards,
Manu

My fault: I sent the new patch on its own, and the CFBot takes only the
attachments of the last message, so it tried to apply a patch that adds
files under src/test/modules/test_progress/ to a tree that does not have
that directory yet. Hence "needs rebase" on the entry.

Here is the whole series, rebased on master (07c73f45063). Nothing
changed in 0001-0005; 0006 is the documentation cross-check I posted an
hour ago.

Rebasing it did turn up something worth mentioning, since it is the sort
of thing this series exists to catch. With only the last three patches
applied on current master, the trace checker fails:

CREATE INDEX 1 violation
parallel GIN build 1
CREATE INDEX CONCURRENTLY 1
REPACK 5
REPACK (CONCURRENTLY) 3

all of them resets that do not reset:

PROGRESS_SCAN_BLOCKS_DONE went from 14 to 1, not to 0
PROGRESS_CREATEIDX_TUPLES_DONE went from 800 to 1, not to 0

Those are the two index-build bugs that 0001 and 0002 fix, so this is
not a regression, it is the checker doing its job on a tree that is
missing the fixes. I am noting it because it is a decent demonstration
of what the series buys: drop two fixes from a five-patch series and the
test tells you exactly which counters stopped being reset, with the log
line that proves it.

With the six applied: make check passes, and test_progress passes while
still reporting the one thing it is meant to report, the REPACK phase
table listing "catch-up" before "rebuilding index" when execution does
the opposite.

Regards,
Manu

The CFBot still could not apply the series, and this time it was the
mail, not the tree: the attachments of my last message went out as
text/plain without base64, and on the way the first line of each was
escaped from "From <sha1>" to ">From <sha1>", which git am does not
recognize ("Patch format detection failed" in the CFBot log). They
are base64-encoded this time.

v3, rebased on master (374522aa63a), changes two things.

0005 now runs every command that reports progress, which Álvaro asked
for. New since v2:

- REINDEX TABLE and REINDEX INDEX CONCURRENTLY
- CREATE INDEX on a partitioned table (partitions_total/done)
- ANALYZE of a partitioned table with extended statistics
- COPY FROM with WHERE and ON_ERROR ignore (tuples_excluded/skipped)
- VACUUM FULL
- disabling and enabling data checksums online, launcher and workers

Data checksums needed one addition to the rules: their counters start
at -1, which the view shows as NULL, so a counter may now go to and
come back from a command's NULL value.

The new cases earn their keep on the old bugs, too. With 0001-0004
reverted, the checker used to fail 10 checks; it now fails 14, the new
ones in REINDEX TABLE, REINDEX CONCURRENTLY, CREATE INDEX on a
partitioned table and VACUUM FULL, all of them the same unreset
counters.

0006 changes in three ways:

- It no longer compares the order of the phase tables in
monitoring.sgml with the order of execution. Sami made the point in
the REPACK phases thread [1]/messages/by-id/CAN12+YKo-vjvdPQts6QnHoB3ET5A2or137oN_FGMviQdtYLN6Q@mail.gmail.com that the documented order is not a
guarantee, and 001 already checks the actual succession of phases.
What remains is the drift check: every phase a command reports must
be named by the view and listed in the documentation.
- That check used to run only when it found a problem, so a clean run
passed without testing anything; it is now one test per command.
- 002_doc_phases.pl only ran when PROGRESS_DEBUG_BUILD=1 was set in the
environment, and nothing sets it, so on a server compiled with
-DPROGRESS_DEBUG -- the buildfarm animal Álvaro suggested -- it would
have been skipped every time. It now detects the build the same way
001_progress.pl does; that check moved into ProgressCheck.pm as
compiled_with_progress_debug().

0001-0004 are unchanged apart from the rebase.

Tested with meson:

- with -DPROGRESS_DEBUG: 001 passes 80 subtests (53 in v2), and 002
passes 7;
- 0005 alone passes the same 80;
- 002 fails, naming the phase, when a row of a phase table is renamed;
- without PROGRESS_DEBUG: 001 only checks progress.h against its
description, and 002 skips, saying why;
- the main regression suite passes, with no compiler warnings.

Regards,
Manu

[1]: /messages/by-id/CAN12+YKo-vjvdPQts6QnHoB3ET5A2or137oN_FGMviQdtYLN6Q@mail.gmail.com
/messages/by-id/CAN12+YKo-vjvdPQts6QnHoB3ET5A2or137oN_FGMviQdtYLN6Q@mail.gmail.com

v4 attached, rebased over master.

While rebasing, the completeness check in 0006 flagged that commit
1378aa13430 ("Report per-index vacuum progress in pg_stat_progress_vacuum")
added PROGRESS_VACUUM_CURRENT_INDEX_RELID without it being described in the
spec that the test reads from progress.h. v4 classifies it -- it is the
OID of the index a worker is currently processing, so a free value.
Catching that kind of drift, a new counter that nothing describes or
tests, is what the check is for.

Nothing else changed since v3: the four bug fixes in 0001-0004 and the
PROGRESS_DEBUG trace in 0005 are the same.

Regards,
Manu

On Mon, Oct 05, 2026 at 10:29:09PM -0300, Manu wrote:

While rebasing, the completeness check in 0006 flagged that commit
1378aa13430 ("Report per-index vacuum progress in pg_stat_progress_vacuum")
added PROGRESS_VACUUM_CURRENT_INDEX_RELID without it being described in the
spec that the test reads from progress.h. v4 classifies it -- it is the
OID of the index a worker is currently processing, so a free value.
Catching that kind of drift, a new counter that nothing describes or
tests, is what the check is for.

While posting such things, you may want to create a new thread instead
of replying to an existing thread and changing the subject of the
previous thread to what you want to send: this breaks thread flows.

Nothing else changed since v3: the four bug fixes in 0001-0004 and the
PROGRESS_DEBUG trace in 0005 are the same.

Ah, yeah, v4-0001 and v4-0004 look like mistakes. While we're on it,
there is an extra gap in _gin_process_worker_data(): in the case of
GinBufferShouldTrim(), we should surely increment bs_numtuples after
tuplesort_putgintuple().

v4-0002 looks wrong to me; you are manipulating the report resets
even under !progress, and you surely should not do that.

I don't see the point of v4-0003 as well: on the next pass the
counters will be updated anyway, or we should see a reset of the
counters shortly enough.

All these feel cosmetic to me, even if from what I can see they could
lead to sporadic blips. If I follow that correctly, heap goes a short
time at 100% of blocks done vs total, while gin could go over 100% of
done vs total. Annoying, but not critical in any way.
--
Michael