Antonin Houska <[email protected]> wrote:
> > 3. 0008: assertion failure in compute_new_xmax_infomask()
>
> I don't know at the moment when the XID could get assigned. I need to
> do some investigation.
I measured it, so you don't have to. gdb attached to the backend
running REPACK, breakpoint on AssignTransactionId(), backtrace on every
hit (script attached). In a whole REPACK (CONCURRENTLY) run there is
exactly one hit, and it is this one:
#0 AssignTransactionId (xact.c:644)
#1 GetCurrentTransactionId (xact.c:461)
#2 LogAccessExclusiveLockPrepare (standby.c:1465)
#3 LockAcquireExtended (lock.c:1008) lockmode=8
#4 LockRelationOid (lmgr.c:115) relid of the new heap
#5 heap_create_with_catalog (heap.c:1294) "pg_temp_16384"
#6 make_new_heap (repack.c:1709)
#7 make_new_heap_for_repack (repack.c:1389)
#8 rebuild_relation (repack.c:1230)
So it is not the replay that assigns the XID, and it is not a race: it
is REPACK itself, creating the transient heap, long before any change
is applied. heap_create_with_catalog() locks the new relation with
AccessExclusiveLock, and LockAcquireExtended() does
if (lockmode >= AccessExclusiveLock &&
locktag->locktag_type == LOCKTAG_RELATION &&
!RecoveryInProgress() &&
XLogStandbyInfoActive())
{
LogAccessExclusiveLockPrepare();
log_lock = true;
}
and LogAccessExclusiveLockPrepare() is a bare
GetCurrentTransactionId(), so that the standby can release the lock at
commit (its comment explains why).
The two thresholds line up exactly: XLogStandbyInfoActive() is
wal_level >= WAL_LEVEL_REPLICA, and repack.c:1012 already refuses to
run with anything below WAL_LEVEL_REPLICA. So the branch is taken on
every REPACK that is allowed to start at all, and the backend has a
top XID from the moment it creates the transient heap.
That makes the Assert unsatisfiable from that side: by the time the
replay calls heap_update() for a change whose xmax belongs to another
transaction, GetTopTransactionIdIfAny() is necessarily valid. It is
not that the XID is assigned too early by accident - there is nowhere
later to move it, short of creating the new heap in a separate
transaction. Whether the right answer is to relax the Assert for this
caller or to change where the XID comes from is your call; I did not
want to guess at it, but the measurement seemed worth having before
you spend time looking.
> > 4. Progress reporting
> ...
> Do you mean that we should add variants of WRITE_NEW_HEAP and
> REBUILD_INDEX specifically for the auxiliary table?
Not necessarily - I think there are two separate things in there, and
only the second one is a design question.
The first is a plain reporting bug: build_new_index() sets the phase
for indexes that are not the table's own - the identity index of the
empty new heap and the indexes of the auxiliary table. That alone is
what makes "rebuilding index" appear before "seq scanning heap", and
twice with USING INDEX. Not setting the phase for those internal
builds fixes the order without touching the catalog or the docs.
The second is what the auxiliary table's work should be reported as,
and there I would rather not add new values. Today, with v03 and
USING INDEX, SORT_TUPLES and WRITE_NEW_HEAP are never reported at all,
while the "REPACK Phases" table in monitoring.sgml still says
"REPACK is currently sorting tuples" and "REPACK is currently writing
the new heap". A user watching pg_stat_progress_repack on v19 and on
v20 would see two documented phases disappear.
> With the auxiliary table, sorting IMO hapens in two phases: 1) build
> the clustering index and 2) scan the index and insert the output into
> the new heap. As long as each phase is reported on its own, I don't
> see room for SORT_TUPLES.
Your two phases map onto the two existing values, I think: (1) is
where the tuplesort actually runs, so that is SORT_TUPLES, and (2) is
WRITE_NEW_HEAP. That is the room for SORT_TUPLES - the sort is the
index build. It keeps the documented set of phases intact and needs
no catversion bump.
If you do prefer new values for the auxiliary table, that is fine too,
but then monitoring.sgml and system_views.sql have to move with it. I
have a test module for exactly that check (it cross-reads progress.h,
system_views.sql and monitoring.sgml and compares the documented
phases against the ones a real run reports) in [1]; I'm happy to run
it against your next version either way, before it becomes someone
else's bug report.
One more thing to keep an eye on: Sami Imseih is changing the same
phases in [2] (v2-0001 docs for v19, v2-0002 phases for v20). Worth
syncing so the two don't collide.
[1] https://commitfest.postgresql.org/patch/7331/
[2] https://commitfest.postgresql.org/patch/7330/
Regards,
Manu
#!/usr/bin/env bash
# #6957 v03-0008: WHO assigns the XID to the REPACK backend before the replay?
#
# The Assert in compute_new_xmax_infomask() (heapam.c) requires that, if the
# xmax about to be written is not ours, we must not have a top XID:
#
# Assert(TransactionIdIsCurrentTransactionId(add_to_xmax) ||
# !TransactionIdIsValid(GetTopTransactionIdIfAny()))
#
# and it fails during the replay that runs after AccessExclusiveLock is taken.
# Antonin said (2026-09-23) he does not know when that XID gets assigned. This
# measures it instead of reasoning about it: gdb attaches TO THE REPACK BACKEND
# and dumps a backtrace on every AssignTransactionId(), plus the one at the
# TRAP. The last backtrace before the TRAP is the one that assigned the XID
# that breaks the Assert.
#
# catch_xid.sh [build] [repack_snapshot_after]
set -u
N=${1:-p6957a}
AFTER=${2:-50}
BIN=$HOME/pgprog/i-$N/bin
HERE=$(cd "$(dirname "$0")" && pwd)
PGD=$(mktemp -d /tmp/claude-1000/catchxid.XXXX)
PORT=54967
OUT=$HERE/cazar_xid.out
export PGPORT=$PORT PGHOST=/tmp PGDATABASE=postgres
"$BIN/initdb" -D "$PGD" -A trust --no-sync > /dev/null
cat >> "$PGD/postgresql.conf" <<EOF
port = $PORT
unix_socket_directories = '/tmp'
wal_level = logical
max_replication_slots = 10
log_line_prefix = '%m [%p] '
logging_collector = off
restart_after_crash = on
EOF
"$BIN/pg_ctl" -D "$PGD" -l "$PGD/server.log" -w start > /dev/null
P="$BIN/psql -X -q"
$P -c "CREATE TABLE t (id int PRIMARY KEY, v text)" \
-c "INSERT INTO t SELECT g, repeat('x', 200) FROM generate_series(1, 30000)
g"
# The session that runs REPACK: through a fifo, so we have its PID beforehand.
FIFO=$(mktemp -u); mkfifo "$FIFO"
# -At (unaligned, no headers) is required: with the table format, \g also
# writes the "(1 row)" line and the PID comes out with a 1 glued to it.
"$BIN/psql" -X -q -At < "$FIFO" > "$PGD/repack.out" 2>&1 &
exec 3> "$FIFO"
echo "SELECT pg_backend_pid() \\g $PGD/pid" >&3
for _ in $(seq 40); do [ -s "$PGD/pid" ] && break; sleep 0.2; done
PID=$(tr -dc '0-9' < "$PGD/pid")
kill -0 "$PID" 2>/dev/null || { echo "invalid PID: '$PID'"; exit 1; }
echo "== REPACK backend: $PID"
cat > "$PGD/catch.gdb" <<'EOF'
set pagination off
set confirm off
set print frame-arguments all
# Postgres uses SIGUSR1 for IPC all the time: without this, gdb stops at the
# first signal, the trailing `continue` is consumed there, and not a single
# AssignTransactionId is captured (seen: 0 captures and one backtrace sitting
# in epoll_wait).
handle SIGUSR1 nostop noprint pass
handle SIGUSR2 nostop noprint pass
handle SIGALRM nostop noprint pass
handle SIGPIPE nostop noprint pass
break AssignTransactionId
commands
silent
printf "\n=== AssignTransactionId ===\n"
bt 22
continue
end
continue
printf "\n=== THE BACKEND STOPPED (TRAP/signal) ===\n"
bt 30
EOF
gdb -p "$PID" -batch -x "$PGD/catch.gdb" > "$OUT" 2>&1 &
GDB=$!
sleep 3 # let gdb finish attaching before sending anything
# Concurrent load: with no UPDATEs to replay there is no replay to break.
( for i in $(seq 1 300); do
$P -c "UPDATE t SET v = 'upd' WHERE id = $i * 7" 2> /dev/null
done ) &
LOAD=$!
[ "$AFTER" = 0 ] || echo "SET repack_snapshot_after = $AFTER;" >&3
echo "REPACK (CONCURRENTLY) t;" >&3
echo "SELECT 'done' \\g $PGD/done" >&3
for _ in $(seq 180); do [ -s "$PGD/done" ] && break; kill -0 "$PID" 2>/dev/null
|| break; sleep 1; done
wait "$LOAD" 2>/dev/null
exec 3>&- ; rm -f "$FIFO"
wait "$GDB" 2>/dev/null
echo "== TRAPs in the log: $(grep -c '^TRAP' "$PGD/server.log")"
grep -m1 '^TRAP' "$PGD/server.log" | cut -c1-150
echo "== XID assignments captured: $(grep -c '=== AssignTransactionId ==='
"$OUT")"
cp "$PGD/server.log" "$HERE/cazar_xid.server.log"
cp "$PGD/repack.out" "$HERE/cazar_xid.repack.out" 2>/dev/null
"$BIN/pg_ctl" -D "$PGD" -m immediate -w stop > /dev/null 2>&1
rm -rf "$PGD"
echo "== backtraces in $OUT"
