Hi Shihao,
I tested v1-0001 and v1-0002 on master (cff329240ba), two builds from
the same commit with --enable-cassert --enable-injection-points: one
with only 0002 applied (control) and one with both. 0001 builds with
no new warnings.
1. The race
The script I posted earlier wins the race without an injection point,
so it is a fair before/after check. Same script, same parameters,
against both builds:
master, 0002 only: 5 of 5 runs lost the update
master + 0001: 0 of 5 runs lost the update
2. The test
Thom already reported that the test fails without the fix; this only
adds that it fails for the right reason and nothing else moved:
control: repack_toast FAILS (results differ from expected)
0001: repack_toast passes, 499 ms
The other tests of the injection_points module pass in both builds, so
0002 fails only for the reason it is meant to.
3. The window Thom asked about
> > Is there any opportunity for another rewrite to sneak in between
> > these two?
>
> Yes, but it doesn't matter. The old worker is thrown away and nothing
> has been copied yet. The new worker reads the relfilenode itself when
> it starts, so a rewrite before that is simply what it sees. A rewrite
> after that is caught by the next check, which is made under the lock
> again.
That is the claim I could put under load instead of taking it on
trust. A loop of VACUUM FULL on the toast relation runs for the whole
startup of REPACK, so it lands inside that window many times over,
while an open transaction keeps the worker waiting during setup and an
UPDATE of the TOASTed columns commits right after. With
log_min_messages=debug1 the patch's own DEBUG1 counts the restarts:
hammer 0s: REPACK ended in ~4s, 0 restarts, value correct
hammer 8s: REPACK ended in ~4s, 33 restarts, value correct
hammer 20s: REPACK ended in ~20s, 166 restarts, value correct
hammer 40s: REPACK ended in ~40s, 342 restarts, value correct
master, 20s: REPACK ended in ~5s, n/a, UPDATE LOST
So it holds up under continuous pressure: the value is never wrong, no
deadlock, and the run with no rewrites at all costs nothing (4s, zero
restarts), so the retry loop does not show up on the normal path.
4. One thing worth deciding, not a correctness issue
The numbers above also say that REPACK can be held up for as long as
the rewriting lasts. It is not a hard livelock - with the 8s hammer
it got through in 4s - but with the 20s and the 40s one it finished
only about when the hammer stopped, at roughly 8 restarts per second.
Whether it gets through is a matter of winning the window; the loop
has no cap and no backoff, and each turn starts a worker that waits
for all running transactions again.
I would not call this a bug: correct-but-waiting beats fast-and-wrong,
and a VACUUM FULL loop on a toast relation is not a real workload. But
it is unbounded, and the caller gets no hint of why nothing is
happening. Since 0001 already has the DEBUG1, would it be worth
raising it, or capping the retries and erroring out after N? Your
call - I mention it because the measurement was there.
Script attached (.txt, so the cfbot keeps testing your patches).
Regards,
Manu
#!/usr/bin/env bash
# v1-0001 under a hammer: what happens if the TOAST relation is rewritten
# over and over?
#
# Thom asked (2026-09-23) whether another rewrite can sneak in between
# UnlockRelationOid() and stop_repack_decoding_worker(). Shihao answered that
# it does not matter, because the old worker is discarded and nothing has been
# copied yet. This does not argue with that: it measures it. A loop of
# VACUUM FULL on the TOAST relation runs for the whole startup of REPACK, so
# it lands inside that window many times over.
#
# Three things are measured:
# 1. whether the result is still correct (the UPDATE is not lost),
# 2. how many times the worker was restarted (the DEBUG1 the patch adds),
# 3. whether REPACK finishes, and how fast: the loop in the patch has no
# retry cap, so it matters whether it converges or spins.
#
# hammer.sh [build] [hammer_seconds]
set -u
B=${1:-/home/manu/pgtoast-i-fix}
SECS=${2:-20}
D=/home/manu/pgprog/data_hammer
P=55703
LOG=/home/manu/pgprog/hammer.log
REPACK_OUT=/home/manu/pgprog/hammer-repack.out
"$B/bin/pg_ctl" -D "$D" -m immediate -w stop >/dev/null 2>&1
# The log lives OUTSIDE $D and pg_ctl -l appends: without this, the restart
# count below carries over the previous run (it happened: 446 restarts
# reported on a build that does not even have that message).
rm -rf "$D" "$LOG"
"$B/bin/initdb" -D "$D" -U postgres --no-sync -A trust >/dev/null 2>&1
cat >> "$D/postgresql.conf" <<'EOF'
wal_level = logical
max_replication_slots = 10
max_wal_senders = 10
log_min_messages = debug1
log_line_prefix = '%m [%p] '
EOF
"$B/bin/pg_ctl" -D "$D" -o "-p $P" -l "$LOG" -w start >/dev/null 2>&1
q() { "$B/bin/psql" -p $P -U postgres -qtAX -c "$1" 2>&1; }
q "CREATE TABLE test (id int PRIMARY KEY, big text)" >/dev/null
q "ALTER TABLE test ALTER COLUMN big SET STORAGE EXTERNAL" >/dev/null
q "INSERT INTO test SELECT g, repeat('old', 3000) FROM generate_series(1,3) g"
>/dev/null
TOAST=$(q "SELECT 'pg_toast.' || c2.relname FROM pg_class c1 JOIN pg_class c2
ON c2.oid = c1.reltoastrelid WHERE c1.relname='test'")
echo "== toast: $TOAST build: $B"
# A: an open transaction, so the worker has someone to wait for during setup
( "$B/bin/psql" -p $P -U postgres -qtAX \
-c "BEGIN" -c "SELECT pg_current_xact_id()" -c "SELECT pg_sleep(4)" -c
"COMMIT" >/dev/null 2>&1 ) &
sleep 0.5
# C: the hammer, rewriting the TOAST relation without pause
( until_t=$((SECONDS + SECS))
while [ $SECONDS -lt $until_t ]; do "$B/bin/psql" -p $P -U postgres -qtAX -c
"VACUUM FULL $TOAST" >/dev/null 2>&1; done ) &
HAMMER=$!
# B: the REPACK that has to survive the hammer
t0=$SECONDS
( q "REPACK (CONCURRENTLY) test" > "$REPACK_OUT" 2>&1 ) &
REPACK=$!
wait %1 2>/dev/null # let A commit
q "UPDATE test SET big = repeat('NEW', 4000) WHERE id IN (1,2,3)" >/dev/null
2>&1
# Bound the wait on REPACK: if it never returns, that is exactly the data point
waited=0
while kill -0 $REPACK 2>/dev/null && [ $waited -lt $((SECS + 60)) ]; do sleep
1; waited=$((waited+1)); done
if kill -0 $REPACK 2>/dev/null; then
echo "== REPACK DID NOT FINISH after ${waited}s (the hammer ran for
${SECS}s)"; finished=no
else
echo "== REPACK finished in ~$((SECONDS - t0))s"; finished=yes
fi
wait $HAMMER 2>/dev/null
wait 2>/dev/null
value=$(q "SELECT DISTINCT left(big,9) FROM test")
restarts=$(grep -c 'restarting REPACK decoding worker' "$LOG")
echo "== worker restarts: $restarts"
echo "== REPACK output: $(head -2 "$REPACK_OUT" | tr '\n' ' ')"
echo "== final value: $value (expected NEWNEWNEW)"
# Reading the control run (a build WITHOUT the patch): there, 0 restarts is
# normal (the message does not exist) and a correct value proves nothing,
# because this scenario is not a reliable reproducer of the lost update --
# that is what the earlier race script is for, and it wins 5 times out of 5.
# Here the control only tells us how long a REPACK takes when it ignores the
# rewrites.
[ "$finished" = yes ] && [ "$value" = "NEWNEWNEW" ] \
&& echo "== result correct and REPACK finished" \
|| echo "== CHECK: see $LOG"
"$B/bin/pg_ctl" -D "$D" -m immediate -w stop >/dev/null 2>&1
