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
From 9b0eab2442647960a3bf94e5b9c2a82d17042f11 Mon Sep 17 00:00:00 2001 From: Manu <[email protected]> Date: Wed, 16 Sep 2026 06:48:55 -0300 Subject: [PATCH] Test index_rebuild_count of REPACK (CONCURRENTLY) Commits 4b445479f9e and 0765b48874a fixed index_rebuild_count in pg_stat_progress_repack: creating the TOAST index of the new heap no longer writes the CREATE INDEX phase into the same progress slot, and the concurrent path now counts the indexes it builds. Neither added a test. repack_toast.spec already stops REPACK (CONCURRENTLY) at repack-concurrently-before-lock, right after build_new_indexes() and with a TOAST table on the new heap. Read the progress view from the other session there: with the table's single index rebuilt, the count must be 1. Reverting 4b445479f9e makes it 3, and reverting 0765b48874a makes it 0. --- .../modules/injection_points/expected/repack_toast.out | 10 +++++++++- .../modules/injection_points/specs/repack_toast.spec | 9 +++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/test/modules/injection_points/expected/repack_toast.out b/src/test/modules/injection_points/expected/repack_toast.out index 95e7b19893e..ee8cf1dfa4b 100644 --- a/src/test/modules/injection_points/expected/repack_toast.out +++ b/src/test/modules/injection_points/expected/repack_toast.out @@ -1,6 +1,6 @@ Parsed test spec with 2 sessions -starting permutation: s1_wait_before_lock s2_updates s2_check s2_wakeup_before_lock s1_check +starting permutation: s1_wait_before_lock s2_progress s2_updates s2_check s2_wakeup_before_lock s1_check injection_points_attach ----------------------- @@ -9,6 +9,14 @@ injection_points_attach step s1_wait_before_lock: REPACK (CONCURRENTLY) repack_toast; <waiting ...> +step s2_progress: + SELECT phase, index_rebuild_count FROM pg_stat_progress_repack; + +phase |index_rebuild_count +----------------+------------------- +rebuilding index| 1 +(1 row) + step s2_updates: DELETE FROM repack_toast WHERE i=1; INSERT INTO repack_toast(i, j, k) VALUES (1, gen_external(), gen_compressible(1)); diff --git a/src/test/modules/injection_points/specs/repack_toast.spec b/src/test/modules/injection_points/specs/repack_toast.spec index cc8f034d016..03f5f9086c7 100644 --- a/src/test/modules/injection_points/specs/repack_toast.spec +++ b/src/test/modules/injection_points/specs/repack_toast.spec @@ -125,6 +125,14 @@ teardown session s2 +# By now the one index of repack_toast has been built on the new heap, so +# index_rebuild_count must be 1: the index built must be counted, and the +# TOAST index of the new heap must not leave anything behind. +step s2_progress +{ + SELECT phase, index_rebuild_count FROM pg_stat_progress_repack; +} + # Test different kinds of toast data changes. step s2_updates { @@ -174,6 +182,7 @@ step s2_wakeup_before_lock # CONCURRENTLY find their way into the table. permutation s1_wait_before_lock + s2_progress s2_updates s2_check s2_wakeup_before_lock -- 2.55.0
"""Watch pg_stat_progress_repack live while VACUUM FULL / CLUSTER / REPACK run.
usage: progress_live.py <port> <label>
Creates a table with a TOAST table and three indexes, then for each command
polls the progress view from a second connection every few milliseconds and
prints every change of (phase, index_rebuild_count) with its time.
"""
import sys
import threading
import time
import psycopg
PORT, LABEL = sys.argv[1], sys.argv[2]
POLL = float(sys.argv[3]) if len(sys.argv) > 3 else 0.005
DSN = f"host=/tmp port={PORT} user=postgres dbname=postgres"
ROWS = 1_500_000
COMMANDS = [
"VACUUM FULL progress_t",
"CLUSTER progress_t USING progress_t_pkey",
"REPACK progress_t",
"REPACK (CONCURRENTLY) progress_t",
]
def setup():
with psycopg.connect(DSN, autocommit=True) as c:
c.execute("DROP TABLE IF EXISTS progress_t")
c.execute("CREATE TABLE progress_t (id int PRIMARY KEY, a int, b text, t text)")
c.execute(f"""
INSERT INTO progress_t
SELECT g, g % 9973, md5(g::text),
CASE WHEN g % 500 = 0
THEN (SELECT string_agg(md5((g * x)::text), '')
FROM generate_series(1, 400) x)
END
FROM generate_series(1, {ROWS}) g""")
c.execute("CREATE INDEX progress_t_a ON progress_t (a)")
c.execute("CREATE INDEX progress_t_b ON progress_t (b)")
c.execute("ANALYZE progress_t")
n_idx = c.execute("SELECT count(*) FROM pg_index WHERE indrelid = 'progress_t'::regclass").fetchone()[0]
toast = c.execute("""SELECT t.relname, pg_size_pretty(pg_relation_size(t.oid))
FROM pg_class c JOIN pg_class t ON t.oid = c.reltoastrelid
WHERE c.relname = 'progress_t'""").fetchone()
ver = c.execute("SELECT version()").fetchone()[0].split(" on ")[0]
print(f"### {LABEL}: {ver}")
print(f"table progress_t: {ROWS} rows, {n_idx} indexes, TOAST {toast[0]} = {toast[1]}\n")
return n_idx
def watch(command):
done = threading.Event()
error = []
def run():
try:
with psycopg.connect(DSN, autocommit=True) as c:
c.execute(command)
except Exception as e: # reported below, not swallowed
error.append(e)
finally:
done.set()
samples, last = 0, None
timeline = []
with psycopg.connect(DSN, autocommit=True) as mon:
worker = threading.Thread(target=run)
t0 = time.monotonic()
worker.start()
while not done.is_set():
row = mon.execute(
"SELECT phase, index_rebuild_count FROM pg_stat_progress_repack "
"WHERE relid = 'progress_t'::regclass").fetchone()
samples += 1
if row is not None and row != last:
timeline.append((int((time.monotonic() - t0) * 1000), row[0], row[1]))
last = row
time.sleep(POLL)
worker.join()
elapsed = int((time.monotonic() - t0) * 1000)
print(f"-- {command} ({elapsed} ms, {samples} samples)")
if error:
print(f" ERROR: {error[0]}")
for ms, phase, count in timeline:
print(f" {ms:>6} ms {phase:<28} index_rebuild_count = {count}")
print()
n_idx = setup()
for cmd in COMMANDS:
watch(cmd)
print(f"(the table has {n_idx} indexes: a correct count never goes above {n_idx})")
progress_live.sh
Description: application/shellscript
