Hi,
(I first sent this as a reply on pgsql-committers, by mistake and
without the patch. Sorry for the noise; this is the right list, and
the patch is attached.)
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.
repack_toast.spec already has what is needed: it stops REPACK
(CONCURRENTLY) at repack-concurrently-before-lock, which comes right
after build_new_indexes(), on a table that has a TOAST table and one
index. The attached patch adds one step there, in the other session:
SELECT phase, index_rebuild_count FROM pg_stat_progress_repack;
To check that it catches both problems, I ran the spec on master
(0765b48874a) and with each fix reverted, rebuilding only the touched
file each time:
master, both fixes rebuilding index | 1
4b445479f9e reverted rebuilding index | 3
0765b48874a reverted rebuilding index | 0
git show 4b445479f9e -- src/backend/catalog/toasting.c | git apply -R
git show 0765b48874a -- src/backend/commands/repack.c | git apply -R
make -C src/test/modules/injection_points check \
ISOLATION=repack_toast REGRESS= TAP_TESTS=
On master the whole injection_points module passes with it (4 regress
and 14 isolation tests), built with --enable-cassert and
--enable-injection-points. The patch also applies to REL_19_STABLE,
where both fixes were backpatched, but I have only checked that it
applies there, not run it.
The non-concurrent path has no injection point in the middle, so the
test covers only REPACK (CONCURRENTLY). I watched the other commands
live instead, polling pg_stat_progress_repack from a second connection
without pause on a table with 1.5M rows, a 39 MB TOAST table and three
indexes. Each line is a change seen in the view: time in ms, phase,
and index_rebuild_count after the arrow.
19beta2, VACUUM FULL:
3 ms, initializing -> 2
3 ms, seq scanning heap -> 2
241 ms, rebuilding index -> 2
364 ms, rebuilding index -> 1
521 ms, rebuilding index -> 2
1898 ms, performing final cleanup -> 3
master, VACUUM FULL:
3 ms, seq scanning heap -> 0
765 ms, rebuilding index -> 0
962 ms, rebuilding index -> 1
1191 ms, rebuilding index -> 2
2589 ms, performing final cleanup -> 3
19beta2, REPACK (CONCURRENTLY):
5 ms, initializing -> 2
342 ms, rebuilding index -> 2
2042 ms, catch-up -> 2
2044 ms, performing final cleanup -> 2
master, REPACK (CONCURRENTLY):
5 ms, seq scanning heap -> 0
1027 ms, rebuilding index -> 0
1180 ms, rebuilding index -> 1
1416 ms, rebuilding index -> 2
2735 ms, rebuilding index -> 3
2736 ms, catch-up -> 3
On 19beta2 the count jumps to 2 while the new heap is created, goes
backwards to 1 when the first index is rebuilt, and in concurrent mode
stays at 2 to the end. On master CLUSTER and REPACK look exactly like
VACUUM FULL above, so the non-concurrent path is right as well. (The
scripts are attached too; the master build has --enable-cassert, which
is why it is slower.)
Regards,
Manu
