agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedREPACK (CONCURRENTLY) can silently lose updates when the toast table is rewritten
17+ messages / 7 participants
[nested] [flat]
* REPACK (CONCURRENTLY) can silently lose updates when the toast table is rewritten
@ 2026-09-23 00:23 Thom Brown <thom@linux.com>
0 siblings, 2 replies; 17+ messages in thread
From: Thom Brown @ 2026-09-23 00:23 UTC (permalink / raw)
To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Hi,
Whilst stress-testing REPACK (CONCURRENTLY) I managed to get it to silently
throw away committed updates to a TOASTed column. There's no error, and both
verify_heapam() and bt_index_check() seem to think everything is fine.
I had claude analyse what's going on and:
-------
It happens at the very start of REPACK. The decoding worker grabs the
toast relation's relfilenode in repack_setup_logical_decoding() and immediately
lets go of the lock, but the backend doesn't lock the toast table until
copy_table_data(). In between sits get_initial_snapshot(), which waits for all
running XIDs to finish, so an open transaction widens the gap nicely. During
that gap you can rewrite the toast table by name (VACUUM FULL / REPACK /
CLUSTER of it) and give it a fresh relfilenode - the parent is only held with
ShareUpdateExclusiveLock, so that's allowed.
-------
Reproducible steps:
-- session A: hold a transaction open so REPACK blocks building its snapshot
BEGIN;
SELECT pg_current_xact_id();
-- session B: set up, arm the one stock injection point, then REPACK (blocks)
CREATE TABLE test (id int PRIMARY KEY, big text);
ALTER TABLE test ALTER COLUMN big SET STORAGE EXTERNAL;
INSERT INTO test SELECT g, repeat('old', 3000) FROM generate_series(1,3) g;
SELECT injection_points_attach('repack-concurrently-before-lock', 'wait');
REPACK (CONCURRENTLY) test;
-- session C: rewrite the toast table in the gap, then let A commit
VACUUM FULL pg_toast.pg_toast_<oid-of-test>;
-- session A:
COMMIT;
-- session C: update the toasted rows
UPDATE test SET big = repeat('NEW', 4000) WHERE id IN (1,2,3);
SELECT id, left(big,9) AS val, length(big) FROM test ORDER BY id;
id | val | length
----+-----------+--------
1 | NEWNEWNEW | 12000
2 | NEWNEWNEW | 12000
3 | NEWNEWNEW | 12000
-- session C: let REPACK finish, then look again
SELECT injection_points_wakeup('repack-concurrently-before-lock');
SELECT id, left(big,9) AS val, length(big) FROM test ORDER BY id;
id | val | length
----+-----------+--------
1 | oldoldold | 9000
2 | oldoldold | 9000
3 | oldoldold | 9000
SELECT * FROM verify_heapam('t', check_toast => true); -- 0 rows
SELECT bt_index_check('t_pkey', heapallindexed => true); -- passes
So three committed updates have quietly reverted to their old values.
Regards
Thom
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: REPACK (CONCURRENTLY) can silently lose updates when the toast table is rewritten
@ 2026-09-23 00:42 Manu <manuelreyesbravo@gmail.com>
parent: Thom Brown <thom@linux.com>
1 sibling, 1 reply; 17+ messages in thread
From: Manu @ 2026-09-23 00:42 UTC (permalink / raw)
To: Thom Brown <thom@linux.com>; +Cc: pgsql-hackers@lists.postgresql.org
Hi Thom,
Reproduced here on master (07c73f45063), Linux, following your recipe
exactly: the three rows read NEWNEWNEW/12000 before REPACK finishes and
oldoldold/9000 after it, no error anywhere.
One thing worth adding, because it changes how this reads: the
injection point is not needed.
It makes the race deterministic, but the window your analysis describes
- between repack_setup_logical_decoding() taking the toast relfilenode
and copy_table_data() locking the toast table - is wide enough on its
own, because get_initial_snapshot() sits in it waiting for running
XIDs. An ordinary open transaction is enough to hold it open. Without
any injection point:
session A BEGIN; SELECT pg_current_xact_id(); (sleeps 3s, commits)
session B REPACK (CONCURRENTLY) test; -- waits for A
session C VACUUM FULL pg_toast.pg_toast_<oid>; -- inside the gap
session C UPDATE test SET big = ... ; -- right after A commits
5 of 5 runs lost the update.
Script attached; it builds and drops the table on each iteration so the
result is not an artifact of one particular relfilenode.
So this does not need a debug build or a test-only feature to happen.
It needs a long-running transaction, a REPACK (CONCURRENTLY), and
someone rewriting that table's toast relation in the meantime - which
is an odd thing to do by hand, but it is allowed, the parent is only
held with ShareUpdateExclusiveLock as you say, and "run VACUUM FULL on
the biggest toast tables" is the kind of thing maintenance scripts do.
I have not looked for a fix yet. From your description the obvious
question is whether repack_setup_logical_decoding() should hold the
toast lock until copy_table_data() takes it, or whether the relfilenode
should be re-checked after the snapshot is built; the second sounds
cheaper but I have not read enough of that path to have an opinion
worth posting.
Regards,
Manu
#!/usr/bin/env bash
# ?Hace falta el injection point, o la carrera se gana sola?
#
# Con el punto de inyeccion el bug sale siempre, porque REPACK queda detenido
# justo antes de tomar el lock. Sin el, hay que meter el UPDATE en la ventana
# que va desde que la transaccion vieja commitea hasta que REPACK copia la
# tabla. Aca se intenta N veces, con el UPDATE disparado inmediatamente
# despues del COMMIT, sin esperas.
set -u
B=/home/manu/pgprog/i-serie
D=/home/manu/pgprog/data_carrera
P=55592
N=${N:-5}
"$B/bin/pg_ctl" -D "$D" -m immediate -w stop >/dev/null 2>&1
rm -rf "$D"
"$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
EOF
"$B/bin/pg_ctl" -D "$D" -o "-p $P" -l /home/manu/pgprog/carrera.log -w start >/dev/null 2>&1
q() { "$B/bin/psql" -p $P -U postgres -qtAX -c "$1" 2>&1; }
ganadas=0
for i in $(seq 1 $N); do
q "DROP TABLE IF EXISTS test" >/dev/null
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'")
# A: transaccion abierta, se cierra sola a los 3s
( "$B/bin/psql" -p $P -U postgres -qtAX \
-c "BEGIN" -c "SELECT pg_current_xact_id()" -c "SELECT pg_sleep(3)" -c "COMMIT" >/dev/null 2>&1 ) &
sleep 0.5
# B: REPACK, que se va a quedar esperando a que A termine
( q "REPACK (CONCURRENTLY) test" >/dev/null 2>&1 ) &
# C: reescribir la toast mientras REPACK espera
sleep 1
q "VACUUM FULL $TOAST" >/dev/null 2>&1
# esperar a que A commitee y disparar el UPDATE lo antes posible
wait %1 2>/dev/null
q "UPDATE test SET big = repeat('NEW', 4000) WHERE id IN (1,2,3)" >/dev/null 2>&1
wait 2>/dev/null
res=$(q "SELECT DISTINCT left(big,9) FROM test")
if [ "$res" = "oldoldold" ]; then
ganadas=$((ganadas+1)); echo " intento $i: UPDATE PERDIDO"
else
echo " intento $i: los updates sobrevivieron ($res)"
fi
done
"$B/bin/pg_ctl" -D "$D" -m immediate -w stop >/dev/null 2>&1
echo
echo "sin injection point: $ganadas de $N intentos perdieron el update"
Attachments:
[text/plain] nocfbot-repack-toast-race.sh.txt (2.3K, ../../179012413951.1850281.5077495683381671561@gmail.com/2-nocfbot-repack-toast-race.sh.txt)
download | inline:
#!/usr/bin/env bash
# ?Hace falta el injection point, o la carrera se gana sola?
#
# Con el punto de inyeccion el bug sale siempre, porque REPACK queda detenido
# justo antes de tomar el lock. Sin el, hay que meter el UPDATE en la ventana
# que va desde que la transaccion vieja commitea hasta que REPACK copia la
# tabla. Aca se intenta N veces, con el UPDATE disparado inmediatamente
# despues del COMMIT, sin esperas.
set -u
B=/home/manu/pgprog/i-serie
D=/home/manu/pgprog/data_carrera
P=55592
N=${N:-5}
"$B/bin/pg_ctl" -D "$D" -m immediate -w stop >/dev/null 2>&1
rm -rf "$D"
"$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
EOF
"$B/bin/pg_ctl" -D "$D" -o "-p $P" -l /home/manu/pgprog/carrera.log -w start >/dev/null 2>&1
q() { "$B/bin/psql" -p $P -U postgres -qtAX -c "$1" 2>&1; }
ganadas=0
for i in $(seq 1 $N); do
q "DROP TABLE IF EXISTS test" >/dev/null
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'")
# A: transaccion abierta, se cierra sola a los 3s
( "$B/bin/psql" -p $P -U postgres -qtAX \
-c "BEGIN" -c "SELECT pg_current_xact_id()" -c "SELECT pg_sleep(3)" -c "COMMIT" >/dev/null 2>&1 ) &
sleep 0.5
# B: REPACK, que se va a quedar esperando a que A termine
( q "REPACK (CONCURRENTLY) test" >/dev/null 2>&1 ) &
# C: reescribir la toast mientras REPACK espera
sleep 1
q "VACUUM FULL $TOAST" >/dev/null 2>&1
# esperar a que A commitee y disparar el UPDATE lo antes posible
wait %1 2>/dev/null
q "UPDATE test SET big = repeat('NEW', 4000) WHERE id IN (1,2,3)" >/dev/null 2>&1
wait 2>/dev/null
res=$(q "SELECT DISTINCT left(big,9) FROM test")
if [ "$res" = "oldoldold" ]; then
ganadas=$((ganadas+1)); echo " intento $i: UPDATE PERDIDO"
else
echo " intento $i: los updates sobrevivieron ($res)"
fi
done
"$B/bin/pg_ctl" -D "$D" -m immediate -w stop >/dev/null 2>&1
echo
echo "sin injection point: $ganadas de $N intentos perdieron el update"
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: REPACK (CONCURRENTLY) can silently lose updates when the toast table is rewritten
@ 2026-09-23 05:09 shihao zhong <zhong950419@gmail.com>
parent: Manu <manuelreyesbravo@gmail.com>
0 siblings, 2 replies; 17+ messages in thread
From: shihao zhong @ 2026-09-23 05:09 UTC (permalink / raw)
To: Manu <manuelreyesbravo@gmail.com>; +Cc: Thom Brown <thom@linux.com>; pgsql-hackers@lists.postgresql.org
> or whether the relfilenode should be re-checked after the snapshot is
built
Holding the toast lock from the start deadlocks. A session that asks for
AccessExclusiveLock gets an XID before it waits, and the decoding worker
waits for all XIDs while it sets up.
So the attached patch re-checks instead. Once the worker is set up it no
longer waits for anyone, so the backend locks the toast table there and
compares its relfilenode with the one the worker uses. If they differ, it
starts a new worker. Nothing has been copied yet, so REPACK just carries on.
0002 adds a test to repack_toast.spec that fails without 0001.
optional.
Thanks,
Shihao
Attachments:
[application/octet-stream] v1-0001-Fix-REPACK-CONCURRENTLY-losing-updates-after-a-TO.patch (6.5K, ../../CAGRkXqRYLtBRaMzdH+e7PMO-BRaWPPo37gvOx3C=jQ1uP4Cx7w@mail.gmail.com/3-v1-0001-Fix-REPACK-CONCURRENTLY-losing-updates-after-a-TO.patch)
download | inline diff:
From 7156c0b4376975367d32e4984712dd4043092468 Mon Sep 17 00:00:00 2001
From: Shihao <zhong950419@gmail.com>
Date: Wed, 23 Sep 2026 00:37:52 -0400
Subject: [PATCH v1 1/2] Fix REPACK (CONCURRENTLY) losing updates after a TOAST
rewrite
The decoding worker of REPACK (CONCURRENTLY) remembers the relfilenumber
of the TOAST relation when it starts, and only decodes the TOAST changes
stored under it. The backend did not lock the TOAST relation until it
started to copy the data. In between, the worker waits for running
transactions to finish, so the gap can be long.
If the TOAST relation was rewritten in that gap, for example by VACUUM
FULL run on it directly, the TOAST chunks of concurrent updates were
filtered out. An updated value then reached the apply phase as a plain
on-disk TOAST pointer, which the apply code takes as a sign that the
column did not change. So it kept the old value, and the committed update
was lost with no error.
Fix by locking the TOAST relation as soon as the worker has finished its
setup, and checking that the TOAST relation still has the relfilenumber
the worker uses. If it does not, start over with a new worker. No data
has been copied at that point, so REPACK just goes on.
We cannot take the lock before starting the worker. A transaction waiting
for that lock has an XID, and the worker waits for it to finish, so that
would be a deadlock. Once its setup is done, the worker no longer waits
for other transactions.
Backpatch to v19, where REPACK (CONCURRENTLY) was introduced.
Reported-by: Thom Brown <thom@linux.com>
Discussion: https://postgr.es/m/CAA-aLv5MF6BLL+BWvix2Yw+CBardtH43AofPReQunhDZPNBtuA@mail.gmail.com
Backpatch-through: 19
---
src/backend/commands/repack.c | 68 +++++++++++++++++++++++++-
src/backend/commands/repack_worker.c | 1 +
src/include/commands/repack_internal.h | 7 +++
3 files changed, 75 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 759be53d6b8..28f60b9933c 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -221,6 +221,7 @@ static void wait_for_repack_decoding_worker(void);
static void stop_repack_decoding_worker(void);
static void stop_repack_decoding_worker_cb(int code, Datum arg);
static Snapshot get_initial_snapshot(DecodingWorker *worker);
+static bool toast_rewritten_since_worker_start(Oid toastrelid);
static void ProcessRepackMessage(StringInfo msg);
static const char *RepackCommandAsString(RepackCommand cmd);
@@ -1141,7 +1142,43 @@ rebuild_relation(Relation OldHeap, Relation index, bool verbose,
* clustering index) and checking again if it's still eligible for
* REPACK CONCURRENTLY.
*/
- start_repack_decoding_worker(tableOid);
+ for (;;)
+ {
+ Oid toastrelid = OldHeap->rd_rel->reltoastrelid;
+
+ start_repack_decoding_worker(tableOid);
+
+ /*
+ * The worker decodes the TOAST chunks of concurrent changes by
+ * the relfilenumber the TOAST relation had when the worker
+ * started, but we don't hold a lock on the TOAST relation yet, so
+ * it could have been rewritten since then (VACUUM FULL can be run
+ * on it directly). If that happened, the TOAST chunks would not
+ * be decoded, and a changed TOASTed value would be taken for an
+ * unchanged one when applying the changes.
+ *
+ * So lock the TOAST relation now and check. If it has been
+ * rewritten, start over with a new worker. We haven't copied any
+ * data yet, so nothing else is lost.
+ *
+ * We can't lock the TOAST relation before starting the worker:
+ * the worker waits for all transactions with XID to finish, and a
+ * transaction waiting for our lock would then cause a deadlock.
+ * Now that the worker has finished its setup, it no longer waits
+ * for other transactions.
+ */
+ if (!OidIsValid(toastrelid))
+ break;
+ LockRelationOid(toastrelid, ShareUpdateExclusiveLock);
+ if (!toast_rewritten_since_worker_start(toastrelid))
+ break;
+
+ ereport(DEBUG1,
+ errmsg_internal("TOAST relation of \"%s\" was rewritten, restarting REPACK decoding worker",
+ RelationGetRelationName(OldHeap)));
+ UnlockRelationOid(toastrelid, ShareUpdateExclusiveLock);
+ stop_repack_decoding_worker();
+ }
/*
* Wait until the worker has the initial snapshot and retrieve it.
@@ -4036,6 +4073,35 @@ get_initial_snapshot(DecodingWorker *worker)
return snapshot;
}
+/*
+ * Has the given TOAST relation been rewritten since the decoding worker
+ * started?
+ *
+ * The worker only decodes the changes of the TOAST relation stored under the
+ * relfilenumber it saw when starting. The caller must hold a lock on the
+ * TOAST relation that prevents it from being rewritten.
+ */
+static bool
+toast_rewritten_since_worker_start(Oid toastrelid)
+{
+ DecodingWorkerShared *shared;
+ RelFileLocator worker_locator;
+ Relation toastrel;
+ bool result;
+
+ shared = (DecodingWorkerShared *) dsm_segment_address(decoding_worker->seg);
+ SpinLockAcquire(&shared->mutex);
+ Assert(shared->initialized);
+ worker_locator = shared->toast_locator;
+ SpinLockRelease(&shared->mutex);
+
+ toastrel = table_open(toastrelid, NoLock);
+ result = !RelFileLocatorEquals(toastrel->rd_locator, worker_locator);
+ table_close(toastrel, NoLock);
+
+ return result;
+}
+
/*
* Generate worker's file name into 'fname', which must be of size MAXPGPATH.
* If relations of the same 'relid' happen to be processed at the same time,
diff --git a/src/backend/commands/repack_worker.c b/src/backend/commands/repack_worker.c
index 690863c6411..6df672c2ca7 100644
--- a/src/backend/commands/repack_worker.c
+++ b/src/backend/commands/repack_worker.c
@@ -143,6 +143,7 @@ RepackWorkerMain(Datum main_arg)
/* Announce that we're ready. */
SpinLockAcquire(&shared->mutex);
+ shared->toast_locator = repacked_rel_toast_locator;
shared->initialized = true;
SpinLockRelease(&shared->mutex);
ConditionVariableSignal(&shared->cv);
diff --git a/src/include/commands/repack_internal.h b/src/include/commands/repack_internal.h
index ec6e31d77f2..b4a4b9908f3 100644
--- a/src/include/commands/repack_internal.h
+++ b/src/include/commands/repack_internal.h
@@ -102,6 +102,13 @@ typedef struct DecodingWorkerShared
/* Relation from which data changes to decode. */
Oid relid;
+ /*
+ * Locator of the TOAST relation whose changes the worker decodes, set
+ * together with 'initialized'. The relNumber is InvalidRelFileNumber if
+ * the relation has no TOAST relation.
+ */
+ RelFileLocator toast_locator;
+
/* CV the backend waits on */
ConditionVariable cv;
--
2.37.1 (Apple Git-137.1)
[application/octet-stream] v1-0002-Test-TOAST-rewrite-during-REPACK-CONCURRENTLY-sta.patch (6.6K, ../../CAGRkXqRYLtBRaMzdH+e7PMO-BRaWPPo37gvOx3C=jQ1uP4Cx7w@mail.gmail.com/4-v1-0002-Test-TOAST-rewrite-during-REPACK-CONCURRENTLY-sta.patch)
download | inline diff:
From 3a9164457aa15c7fed459533652e35da0c40ba0c Mon Sep 17 00:00:00 2001
From: Shihao <zhong950419@gmail.com>
Date: Wed, 23 Sep 2026 00:37:52 -0400
Subject: [PATCH v1 2/2] Test TOAST rewrite during REPACK (CONCURRENTLY)
startup
Add a permutation to repack_toast.spec that rewrites the TOAST relation
while the decoding worker waits for a running transaction. REPACK has to
start the worker again. Without the fix, the concurrent updates of
TOASTed columns are lost.
Discussion: https://postgr.es/m/CAA-aLv5MF6BLL+BWvix2Yw+CBardtH43AofPReQunhDZPNBtuA@mail.gmail.com
---
.../expected/repack_toast.out | 148 +++++++++++++++++-
.../injection_points/specs/repack_toast.spec | 41 +++++
2 files changed, 188 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..ef5c13f8969 100644
--- a/src/test/modules/injection_points/expected/repack_toast.out
+++ b/src/test/modules/injection_points/expected/repack_toast.out
@@ -1,4 +1,4 @@
-Parsed test spec with 2 sessions
+Parsed test spec with 3 sessions
starting permutation: s1_wait_before_lock s2_updates s2_check s2_wakeup_before_lock s1_check
injection_points_attach
@@ -124,3 +124,149 @@ injection_points_detach
(1 row)
+
+starting permutation: s2_begin s1_wait_before_lock s3_rewrite_toast s2_commit s2_updates s2_check s2_wakeup_before_lock s1_check
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step s2_begin:
+ BEGIN;
+ SELECT pg_current_xact_id() IS NOT NULL AS has_xid;
+
+has_xid
+-------
+t
+(1 row)
+
+step s1_wait_before_lock:
+ REPACK (CONCURRENTLY) repack_toast;
+ <waiting ...>
+step s3_rewrite_toast:
+ DO $$
+ BEGIN
+ EXECUTE format('REPACK %s',
+ (SELECT reltoastrelid::regclass FROM pg_class
+ WHERE relname = 'repack_toast'));
+ END;
+ $$;
+
+step s2_commit:
+ COMMIT;
+
+step s2_updates:
+ DELETE FROM repack_toast WHERE i=1;
+ INSERT INTO repack_toast(i, j, k) VALUES (1, gen_external(), gen_compressible(1));
+
+ -- existing toast data unchanged. (This covers the case where we
+ -- adjust the toast pointer.)
+ UPDATE repack_toast SET i=i+300 where i % 10 = 2 RETURNING OLD.i, NEW.i;
+
+ -- "j" is here an external indirect, written to the file separately.
+ UPDATE repack_toast SET j=gen_external() where i % 10 = 3 RETURNING OLD.i, NEW.i;
+
+ -- the updated value of "j" is compressed.
+ UPDATE repack_toast SET j=gen_compressible(1), k=k||'' where i % 10 = 4 RETURNING i;
+
+ -- the updated value of "j" is compressed externally.
+ UPDATE repack_toast SET j=gen_compressible_external(2) where i % 10 = 5 RETURNING i;
+
+ -- the updated value of "j" stays inline.
+ UPDATE repack_toast SET j=gen_inline(), k=repeat(k,5) where i % 10 = 6 RETURNING i;
+
+ -- updated value of "j" is a short varlena; "k" is written separately.
+ UPDATE repack_toast SET j=gen_short(), k=gen_external() where i % 10 = 7 RETURNING i;
+
+ i| i
+--+---
+ 2|302
+12|312
+(2 rows)
+
+ i| i
+--+--
+ 3| 3
+13|13
+(2 rows)
+
+ i
+--
+ 4
+14
+(2 rows)
+
+ i
+--
+ 5
+15
+(2 rows)
+
+ i
+--
+ 6
+16
+(2 rows)
+
+ i
+--
+ 7
+17
+(2 rows)
+
+step s2_check:
+ INSERT INTO relfilenodes(node)
+ SELECT c2.relfilenode
+ FROM pg_class c1 JOIN pg_class c2 ON c2.oid = c1.oid OR c2.oid = c1.reltoastrelid
+ WHERE c1.relname='repack_toast';
+
+ INSERT INTO data_s2(i, j, j_toast, k, k_toast)
+ SELECT i, j, COALESCE(pg_column_toast_chunk_id(j), 0) AS j_toast,
+ k, COALESCE(pg_column_toast_chunk_id(k), 0) AS k_toast
+ FROM repack_toast;
+
+step s2_wakeup_before_lock:
+ SELECT injection_points_wakeup('repack-concurrently-before-lock');
+
+injection_points_wakeup
+-----------------------
+
+(1 row)
+
+step s1_wait_before_lock: <... completed>
+step s1_check:
+ INSERT INTO relfilenodes(node)
+ SELECT c2.relfilenode
+ FROM pg_class c1 JOIN pg_class c2 ON c2.oid = c1.oid OR c2.oid = c1.reltoastrelid
+ WHERE c1.relname='repack_toast';
+
+ SELECT count(DISTINCT node) FROM relfilenodes;
+
+ INSERT INTO data_s1(i, j, j_toast, k, k_toast)
+ SELECT i,
+ j, COALESCE(pg_column_toast_chunk_id(j), 0) AS j_toast,
+ k, COALESCE(pg_column_toast_chunk_id(k), 0) AS k_toast
+ FROM repack_toast;
+
+ -- this should be empty
+ SELECT d1.i, substring(d1.j FOR 12) AS d1_j, substring(d1.k FOR 12) AS d1_k,
+ d2.i, substring(d2.j FOR 12) AS d2_j, substring(d2.k FOR 12) AS d2_k,
+ d1.j_toast as d1_j_tst, d2.j_toast as d2_j_tst,
+ d1.k_toast as d1_k_tst, d2.k_toast AS d2_k_tst
+ FROM data_s1 d1 FULL JOIN data_s2 d2 USING (i, j, k)
+ WHERE d1.i ISNULL OR d2.i ISNULL;
+
+count
+-----
+ 4
+(1 row)
+
+i|d1_j|d1_k|i|d2_j|d2_k|d1_j_tst|d2_j_tst|d1_k_tst|d2_k_tst
+-+----+----+-+----+----+--------+--------+--------+--------
+(0 rows)
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
diff --git a/src/test/modules/injection_points/specs/repack_toast.spec b/src/test/modules/injection_points/specs/repack_toast.spec
index cc8f034d016..0288c054f53 100644
--- a/src/test/modules/injection_points/specs/repack_toast.spec
+++ b/src/test/modules/injection_points/specs/repack_toast.spec
@@ -125,6 +125,18 @@ teardown
session s2
+# Keep a transaction with XID open, so that the decoding worker has to wait
+# before it can build the initial snapshot.
+step s2_begin
+{
+ BEGIN;
+ SELECT pg_current_xact_id() IS NOT NULL AS has_xid;
+}
+step s2_commit
+{
+ COMMIT;
+}
+
# Test different kinds of toast data changes.
step s2_updates
{
@@ -170,6 +182,23 @@ step s2_wakeup_before_lock
SELECT injection_points_wakeup('repack-concurrently-before-lock');
}
+# Rewrite the TOAST relation. The decoding worker only decodes the changes
+# of the TOAST relation stored under the relfilenumber it saw when starting,
+# so REPACK must notice if the TOAST relation got rewritten before REPACK
+# locked it, and start the worker again. Otherwise the TOAST chunks of the
+# concurrent changes are not decoded, and the changes are lost.
+session s3
+step s3_rewrite_toast
+{
+ DO $$
+ BEGIN
+ EXECUTE format('REPACK %s',
+ (SELECT reltoastrelid::regclass FROM pg_class
+ WHERE relname = 'repack_toast'));
+ END;
+ $$;
+}
+
# Test if data changes introduced while one session is performing REPACK
# CONCURRENTLY find their way into the table.
permutation
@@ -178,3 +207,15 @@ permutation
s2_check
s2_wakeup_before_lock
s1_check
+
+# Same, but rewrite the TOAST relation while the decoding worker waits for s2
+# to commit.
+permutation
+ s2_begin
+ s1_wait_before_lock
+ s3_rewrite_toast
+ s2_commit
+ s2_updates
+ s2_check
+ s2_wakeup_before_lock
+ s1_check
--
2.37.1 (Apple Git-137.1)
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: REPACK (CONCURRENTLY) can silently lose updates when the toast table is rewritten
@ 2026-09-23 08:26 Thom Brown <thom@linux.com>
parent: shihao zhong <zhong950419@gmail.com>
1 sibling, 1 reply; 17+ messages in thread
From: Thom Brown @ 2026-09-23 08:26 UTC (permalink / raw)
To: shihao zhong <zhong950419@gmail.com>; +Cc: Manu <manuelreyesbravo@gmail.com>; pgsql-hackers@lists.postgresql.org
On Wed, 23 Sept 2026 at 06:10, shihao zhong <zhong950419@gmail.com> wrote:
>
> > or whether the relfilenode should be re-checked after the snapshot is built
>
> Holding the toast lock from the start deadlocks. A session that asks for
> AccessExclusiveLock gets an XID before it waits, and the decoding worker
> waits for all XIDs while it sets up.
>
> So the attached patch re-checks instead. Once the worker is set up it no
> longer waits for anyone, so the backend locks the toast table there and
> compares its relfilenode with the one the worker uses. If they differ, it
> starts a new worker. Nothing has been copied yet, so REPACK just carries on.
>
> 0002 adds a test to repack_toast.spec that fails without 0001.
Thanks guys.
I've tested your patches, and I can't reproduce the issue with them
applied. The test on its own fails successfully.
I do have a question relating to this:
+ UnlockRelationOid(toastrelid, ShareUpdateExclusiveLock);
+ stop_repack_decoding_worker();
Is there any opportunity for another rewrite to sneak in between these two?
Thom
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: REPACK (CONCURRENTLY) can silently lose updates when the toast table is rewritten
@ 2026-09-23 12:08 shihao zhong <zhong950419@gmail.com>
parent: Thom Brown <thom@linux.com>
0 siblings, 1 reply; 17+ messages in thread
From: shihao zhong @ 2026-09-23 12:08 UTC (permalink / raw)
To: Thom Brown <thom@linux.com>; +Cc: Manu <manuelreyesbravo@gmail.com>; pgsql-hackers@lists.postgresql.org
> + UnlockRelationOid(toastrelid, ShareUpdateExclusiveLock);
> + stop_repack_decoding_worker();
>
> 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.
The unlock has to come before starting the new worker anyway, or we
are back to the deadlock.
Thanks,
Shihao
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: REPACK (CONCURRENTLY) can silently lose updates when the toast table is rewritten
@ 2026-09-23 14:16 Manu <manuelreyesbravo@gmail.com>
parent: shihao zhong <zhong950419@gmail.com>
0 siblings, 0 replies; 17+ messages in thread
From: Manu @ 2026-09-23 14:16 UTC (permalink / raw)
To: shihao zhong <zhong950419@gmail.com>; +Cc: Thom Brown <thom@linux.com>; Antonin Houska <ah@cybertec.at>; pgsql-hackers@lists.postgresql.org
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
Attachments:
[text/plain] nocfbot-repack-toast-hammer.sh.txt (4.0K, ../../179017301330.2624303.9601990576272593585@gmail.com/2-nocfbot-repack-toast-hammer.sh.txt)
download | inline:
#!/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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: REPACK (CONCURRENTLY) can silently lose updates when the toast table is rewritten
@ 2026-09-23 15:06 Melanie Plageman <melanieplageman@gmail.com>
parent: Thom Brown <thom@linux.com>
1 sibling, 1 reply; 17+ messages in thread
From: Melanie Plageman @ 2026-09-23 15:06 UTC (permalink / raw)
To: Thom Brown <thom@linux.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Tue, Sep 22, 2026 at 8:23 PM Thom Brown <thom@linux.com> wrote:
>
> Whilst stress-testing REPACK (CONCURRENTLY) I managed to get it to silently
> throw away committed updates to a TOASTed column. There's no error, and both
> verify_heapam() and bt_index_check() seem to think everything is fine.
Should this be added as an open item? [1]
- Melanie
[1] https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: REPACK (CONCURRENTLY) can silently lose updates when the toast table is rewritten
@ 2026-09-23 16:22 Antonin Houska <ah@cybertec.at>
parent: shihao zhong <zhong950419@gmail.com>
1 sibling, 2 replies; 17+ messages in thread
From: Antonin Houska @ 2026-09-23 16:22 UTC (permalink / raw)
To: shihao zhong <zhong950419@gmail.com>; +Cc: Manu <manuelreyesbravo@gmail.com>; Thom Brown <thom@linux.com>; pgsql-hackers@lists.postgresql.org
shihao zhong <zhong950419@gmail.com> wrote:
> > or whether the relfilenode should be re-checked after the snapshot is built
>
> Holding the toast lock from the start deadlocks. A session that asks for
> AccessExclusiveLock gets an XID before it waits, and the decoding worker
> waits for all XIDs while it sets up.
The same (supposedly low) deadlock risk already exists for the main table, see
this comment in rebuild_relation():
/*
* Start the worker that decodes data changes applied while we're
* copying the table contents.
*
* Note that the worker has to wait for all transactions with XID
* already assigned to finish. If some of those transactions is
* waiting for a lock conflicting with ShareUpdateExclusiveLock on our
* table (e.g. it runs CREATE INDEX), we can end up in a deadlock.
* Not sure this risk is worth unlocking/locking the table (and its
* clustering index) and checking again if it's still eligible for
* REPACK CONCURRENTLY.
*/
start_repack_decoding_worker(tableOid);
I'm not sure if locking the TOAST relation earlier would make the situation
worse.
The reason TOAST relation is not locked until copy_table_data() does so is
that CLUSTER / VACUUM FULL in v18 did it this way (not sure what the reason
for such design was). I haven't changed that for REPACK exactly because I
failed to envision this stale relfilenode issue.
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: REPACK (CONCURRENTLY) can silently lose updates when the toast table is rewritten
@ 2026-09-23 17:09 Thom Brown <thom@linux.com>
parent: Melanie Plageman <melanieplageman@gmail.com>
0 siblings, 0 replies; 17+ messages in thread
From: Thom Brown @ 2026-09-23 17:09 UTC (permalink / raw)
To: Melanie Plageman <melanieplageman@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
On Wed, 23 Sept 2026 at 16:07, Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> On Tue, Sep 22, 2026 at 8:23 PM Thom Brown <thom@linux.com> wrote:
> >
> > Whilst stress-testing REPACK (CONCURRENTLY) I managed to get it to silently
> > throw away committed updates to a TOASTed column. There's no error, and both
> > verify_heapam() and bt_index_check() seem to think everything is fine.
>
> Should this be added as an open item? [1]
Yes. I've now added it.
Thanks.
Thom
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: REPACK (CONCURRENTLY) can silently lose updates when the toast table is rewritten
@ 2026-09-23 17:18 Thom Brown <thom@linux.com>
parent: Antonin Houska <ah@cybertec.at>
1 sibling, 1 reply; 17+ messages in thread
From: Thom Brown @ 2026-09-23 17:18 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: shihao zhong <zhong950419@gmail.com>; Manu <manuelreyesbravo@gmail.com>; pgsql-hackers@lists.postgresql.org
On Wed, 23 Sept 2026 at 17:22, Antonin Houska <ah@cybertec.at> wrote:
>
> shihao zhong <zhong950419@gmail.com> wrote:
>
> > > or whether the relfilenode should be re-checked after the snapshot is built
> >
> > Holding the toast lock from the start deadlocks. A session that asks for
> > AccessExclusiveLock gets an XID before it waits, and the decoding worker
> > waits for all XIDs while it sets up.
>
> The same (supposedly low) deadlock risk already exists for the main table, see
> this comment in rebuild_relation():
>
> /*
> * Start the worker that decodes data changes applied while we're
> * copying the table contents.
> *
> * Note that the worker has to wait for all transactions with XID
> * already assigned to finish. If some of those transactions is
> * waiting for a lock conflicting with ShareUpdateExclusiveLock on our
> * table (e.g. it runs CREATE INDEX), we can end up in a deadlock.
> * Not sure this risk is worth unlocking/locking the table (and its
> * clustering index) and checking again if it's still eligible for
> * REPACK CONCURRENTLY.
> */
> start_repack_decoding_worker(tableOid);
>
> I'm not sure if locking the TOAST relation earlier would make the situation
> worse.
>
> The reason TOAST relation is not locked until copy_table_data() does so is
> that CLUSTER / VACUUM FULL in v18 did it this way (not sure what the reason
> for such design was). I haven't changed that for REPACK exactly because I
> failed to envision this stale relfilenode issue.
I gave that a try, and it does. It just swaps the lost update for a deadlock.
If you lock the toast up front and something rewrites it at the same
time (which is the thing that triggers this in the first place, e.g. a
REPACK of the toast table), REPACK falls over:
Session 1:
BEGIN;
INSERT INTO test VALUES (999999, 'x');
Session 2:
REPACK (CONCURRENTLY) test;
Session 1:
CREATE INDEX ON test (big);
ERROR: deadlock detected
DETAIL: Process 214534 waits for ShareLock on transaction 1774005;
blocked by process 214579.
Process 214579 waits for AccessExclusiveLock on relation 3672470 of
database 5; blocked by process 214534.
CONTEXT: REPACK decoding worker
The rewrite already has an XID by the time it waits, and the worker
waits for that XID whilst it sets up, so the two just sit on each
other. It doesn't matter which lock we take either because anything
that would stop the rewrite conflicts with it.
Thom
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: REPACK (CONCURRENTLY) can silently lose updates when the toast table is rewritten
@ 2026-09-23 18:27 Masahiko Sawada <sawada.mshk@gmail.com>
parent: Antonin Houska <ah@cybertec.at>
1 sibling, 2 replies; 17+ messages in thread
From: Masahiko Sawada @ 2026-09-23 18:27 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: shihao zhong <zhong950419@gmail.com>; Manu <manuelreyesbravo@gmail.com>; Thom Brown <thom@linux.com>; pgsql-hackers@lists.postgresql.org
On Wed, Sep 23, 2026 at 9:23 AM Antonin Houska <ah@cybertec.at> wrote:
>
> shihao zhong <zhong950419@gmail.com> wrote:
>
> > > or whether the relfilenode should be re-checked after the snapshot is built
> >
> > Holding the toast lock from the start deadlocks. A session that asks for
> > AccessExclusiveLock gets an XID before it waits, and the decoding worker
> > waits for all XIDs while it sets up.
>
> The same (supposedly low) deadlock risk already exists for the main table, see
> this comment in rebuild_relation():
>
> /*
> * Start the worker that decodes data changes applied while we're
> * copying the table contents.
> *
> * Note that the worker has to wait for all transactions with XID
> * already assigned to finish. If some of those transactions is
> * waiting for a lock conflicting with ShareUpdateExclusiveLock on our
> * table (e.g. it runs CREATE INDEX), we can end up in a deadlock.
> * Not sure this risk is worth unlocking/locking the table (and its
> * clustering index) and checking again if it's still eligible for
> * REPACK CONCURRENTLY.
> */
> start_repack_decoding_worker(tableOid);
>
> I'm not sure if locking the TOAST relation earlier would make the situation
> worse.
Agreed.
So I think the simplest fix would be to acquire a lock on the TOAST
table before starting the repack worker. It would make the case in
question fail with a deadlock, instead of silently losing updates.
The proposed patch also fixes the problem, but I'm concerned that it
repeatedly starts and stops the repack worker without any limit. I
think we could error out if we detect a concurrent rewrite, so that
users can re-run REPACK CONCURRENTLY. This check could also be done on
the repack worker side: after getting the relfilelocator of the TOAST
table and initializing the logical decoding, the repack worker
rechecks the relfilelocator. If they don't match, it raises an error.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: REPACK (CONCURRENTLY) can silently lose updates when the toast table is rewritten
@ 2026-09-24 01:45 shihao zhong <zhong950419@gmail.com>
parent: Masahiko Sawada <sawada.mshk@gmail.com>
1 sibling, 1 reply; 17+ messages in thread
From: shihao zhong @ 2026-09-24 01:45 UTC (permalink / raw)
To: Masahiko Sawada <sawada.mshk@gmail.com>; +Cc: Antonin Houska <ah@cybertec.at>; Manu <manuelreyesbravo@gmail.com>; Thom Brown <thom@linux.com>; pgsql-hackers@lists.postgresql.org
Hi Masahiko,
Thanks for reviewing it.
> I think we could error out if we detect a concurrent rewrite, so that
> users can re-run REPACK CONCURRENTLY. This check could also be done on
> the repack worker side
Agreed. v2 attached. REPACK now fails if the TOAST table was rewritten,
and the user can run it again.
The check stays in the backend, under the lock, though. If the worker
checks, a rewrite can still come after that check and before
copy_table_data() locks the TOAST table, and the update is lost the same
way. The backend takes the lock right after the worker is set up and
keeps it. Locking first would also work, but then the same race ends in
a deadlock instead of a clear error.
With the loop gone, the window Thom asked about is gone too. 0002 is the
test and is optional.
Shihao
On Wed, Sep 23, 2026 at 2:27 PM Masahiko Sawada <sawada.mshk@gmail.com>
wrote:
> On Wed, Sep 23, 2026 at 9:23 AM Antonin Houska <ah@cybertec.at> wrote:
> >
> > shihao zhong <zhong950419@gmail.com> wrote:
> >
> > > > or whether the relfilenode should be re-checked after the snapshot
> is built
> > >
> > > Holding the toast lock from the start deadlocks. A session that asks
> for
> > > AccessExclusiveLock gets an XID before it waits, and the decoding
> worker
> > > waits for all XIDs while it sets up.
> >
> > The same (supposedly low) deadlock risk already exists for the main
> table, see
> > this comment in rebuild_relation():
> >
> > /*
> > * Start the worker that decodes data changes applied while we're
> > * copying the table contents.
> > *
> > * Note that the worker has to wait for all transactions with XID
> > * already assigned to finish. If some of those transactions is
> > * waiting for a lock conflicting with ShareUpdateExclusiveLock on
> our
> > * table (e.g. it runs CREATE INDEX), we can end up in a deadlock.
> > * Not sure this risk is worth unlocking/locking the table (and its
> > * clustering index) and checking again if it's still eligible for
> > * REPACK CONCURRENTLY.
> > */
> > start_repack_decoding_worker(tableOid);
> >
> > I'm not sure if locking the TOAST relation earlier would make the
> situation
> > worse.
>
> Agreed.
>
> So I think the simplest fix would be to acquire a lock on the TOAST
> table before starting the repack worker. It would make the case in
> question fail with a deadlock, instead of silently losing updates.
>
> The proposed patch also fixes the problem, but I'm concerned that it
> repeatedly starts and stops the repack worker without any limit. I
> think we could error out if we detect a concurrent rewrite, so that
> users can re-run REPACK CONCURRENTLY. This check could also be done on
> the repack worker side: after getting the relfilelocator of the TOAST
> table and initializing the logical decoding, the repack worker
> rechecks the relfilelocator. If they don't match, it raises an error.
>
> Regards,
>
> --
> Masahiko Sawada
> Amazon Web Services: https://aws.amazon.com
>
Attachments:
[application/octet-stream] v2-0001-Fix-REPACK-CONCURRENTLY-losing-updates-after-a-TO.patch (6.4K, ../../CAGRkXqR19T5MMoL=0-tuiYSO82uGfNUdGceSqoDZ9_Fvd6NtUw@mail.gmail.com/3-v2-0001-Fix-REPACK-CONCURRENTLY-losing-updates-after-a-TO.patch)
download | inline diff:
From c8c4feb560f46591a1e942362fd5ab34276d692a Mon Sep 17 00:00:00 2001
From: Shihao <zhong950419@gmail.com>
Date: Wed, 23 Sep 2026 21:03:13 -0400
Subject: [PATCH v2 1/2] Fix REPACK (CONCURRENTLY) losing updates after a TOAST
rewrite
The decoding worker of REPACK (CONCURRENTLY) remembers the relfilenumber
of the TOAST relation when it starts, and only decodes the TOAST changes
stored under it. The backend did not lock the TOAST relation until it
started to copy the data. In between, the worker waits for running
transactions to finish, so the gap can be long.
If the TOAST relation was rewritten in that gap, for example by VACUUM
FULL run on it directly, the TOAST chunks of concurrent updates were
filtered out. An updated value then reached the apply phase as a plain
on-disk TOAST pointer, which the apply code takes as a sign that the
column did not change. So it kept the old value, and the committed update
was lost with no error.
Fix by locking the TOAST relation as soon as the worker has finished its
setup, and checking that the TOAST relation still has the relfilenumber
the worker uses. If it does not, REPACK fails, and the user can run it
again. The lock is held till the end of the transaction, so no rewrite
can come after the check.
We could take the lock before starting the worker instead, but a
transaction waiting for that lock has an XID, and the worker waits for it
to finish. That turns the lost update into a deadlock. Once its setup is
done, the worker no longer waits for other transactions.
Backpatch to v19, where REPACK (CONCURRENTLY) was introduced.
Reported-by: Thom Brown <thom@linux.com>
Discussion: https://postgr.es/m/CAA-aLv5MF6BLL+BWvix2Yw+CBardtH43AofPReQunhDZPNBtuA@mail.gmail.com
Backpatch-through: 19
---
src/backend/commands/repack.c | 59 ++++++++++++++++++++++++++
src/backend/commands/repack_worker.c | 1 +
src/include/commands/repack_internal.h | 7 +++
3 files changed, 67 insertions(+)
diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c
index 759be53d6b8..815655343a0 100644
--- a/src/backend/commands/repack.c
+++ b/src/backend/commands/repack.c
@@ -221,6 +221,7 @@ static void wait_for_repack_decoding_worker(void);
static void stop_repack_decoding_worker(void);
static void stop_repack_decoding_worker_cb(int code, Datum arg);
static Snapshot get_initial_snapshot(DecodingWorker *worker);
+static void check_toast_not_rewritten(Relation OldHeap);
static void ProcessRepackMessage(StringInfo msg);
static const char *RepackCommandAsString(RepackCommand cmd);
@@ -1143,6 +1144,32 @@ rebuild_relation(Relation OldHeap, Relation index, bool verbose,
*/
start_repack_decoding_worker(tableOid);
+ /*
+ * The worker decodes the TOAST chunks of concurrent changes by the
+ * relfilenumber the TOAST relation had when the worker started, but
+ * we don't hold a lock on the TOAST relation yet, so it could have
+ * been rewritten since then (VACUUM FULL can be run on it directly).
+ * If that happened, the TOAST chunks would not be decoded, and a
+ * changed TOASTed value would be taken for an unchanged one when
+ * applying the changes.
+ *
+ * So lock the TOAST relation now and check. The lock is held till
+ * the end of the transaction, so the TOAST relation cannot be
+ * rewritten after the check.
+ *
+ * We can't lock the TOAST relation before starting the worker: the
+ * worker waits for all transactions with XID to finish, and a
+ * transaction waiting for our lock would then cause a deadlock. Now
+ * that the worker has finished its setup, it no longer waits for
+ * other transactions.
+ */
+ if (OidIsValid(OldHeap->rd_rel->reltoastrelid))
+ {
+ LockRelationOid(OldHeap->rd_rel->reltoastrelid,
+ ShareUpdateExclusiveLock);
+ check_toast_not_rewritten(OldHeap);
+ }
+
/*
* Wait until the worker has the initial snapshot and retrieve it.
*/
@@ -4036,6 +4063,38 @@ get_initial_snapshot(DecodingWorker *worker)
return snapshot;
}
+/*
+ * Check that the TOAST relation of OldHeap still has the relfilenumber that
+ * the decoding worker saw when it started, and fail if it does not.
+ *
+ * The worker only decodes the changes of the TOAST relation stored under that
+ * relfilenumber. The caller must hold a lock on the TOAST relation that
+ * prevents it from being rewritten.
+ */
+static void
+check_toast_not_rewritten(Relation OldHeap)
+{
+ DecodingWorkerShared *shared;
+ RelFileLocator worker_locator;
+ Relation toastrel;
+
+ shared = (DecodingWorkerShared *) dsm_segment_address(decoding_worker->seg);
+ SpinLockAcquire(&shared->mutex);
+ Assert(shared->initialized);
+ worker_locator = shared->toast_locator;
+ SpinLockRelease(&shared->mutex);
+
+ toastrel = table_open(OldHeap->rd_rel->reltoastrelid, NoLock);
+ if (!RelFileLocatorEquals(toastrel->rd_locator, worker_locator))
+ ereport(ERROR,
+ errcode(ERRCODE_T_R_SERIALIZATION_FAILURE),
+ errmsg("could not execute %s on relation \"%s\"",
+ "REPACK (CONCURRENTLY)", RelationGetRelationName(OldHeap)),
+ errdetail("The TOAST relation was rewritten concurrently."),
+ errhint("The transaction might succeed if retried."));
+ table_close(toastrel, NoLock);
+}
+
/*
* Generate worker's file name into 'fname', which must be of size MAXPGPATH.
* If relations of the same 'relid' happen to be processed at the same time,
diff --git a/src/backend/commands/repack_worker.c b/src/backend/commands/repack_worker.c
index 690863c6411..6df672c2ca7 100644
--- a/src/backend/commands/repack_worker.c
+++ b/src/backend/commands/repack_worker.c
@@ -143,6 +143,7 @@ RepackWorkerMain(Datum main_arg)
/* Announce that we're ready. */
SpinLockAcquire(&shared->mutex);
+ shared->toast_locator = repacked_rel_toast_locator;
shared->initialized = true;
SpinLockRelease(&shared->mutex);
ConditionVariableSignal(&shared->cv);
diff --git a/src/include/commands/repack_internal.h b/src/include/commands/repack_internal.h
index ec6e31d77f2..b4a4b9908f3 100644
--- a/src/include/commands/repack_internal.h
+++ b/src/include/commands/repack_internal.h
@@ -102,6 +102,13 @@ typedef struct DecodingWorkerShared
/* Relation from which data changes to decode. */
Oid relid;
+ /*
+ * Locator of the TOAST relation whose changes the worker decodes, set
+ * together with 'initialized'. The relNumber is InvalidRelFileNumber if
+ * the relation has no TOAST relation.
+ */
+ RelFileLocator toast_locator;
+
/* CV the backend waits on */
ConditionVariable cv;
--
2.37.1 (Apple Git-137.1)
[application/octet-stream] v2-0002-Test-TOAST-rewrite-during-REPACK-CONCURRENTLY-sta.patch (7.1K, ../../CAGRkXqR19T5MMoL=0-tuiYSO82uGfNUdGceSqoDZ9_Fvd6NtUw@mail.gmail.com/4-v2-0002-Test-TOAST-rewrite-during-REPACK-CONCURRENTLY-sta.patch)
download | inline diff:
From cd941178a647d9026b8c5739364305675733398e Mon Sep 17 00:00:00 2001
From: Shihao <zhong950419@gmail.com>
Date: Wed, 23 Sep 2026 21:03:13 -0400
Subject: [PATCH v2 2/2] Test TOAST rewrite during REPACK (CONCURRENTLY)
startup
Add a permutation to repack_toast.spec that rewrites the TOAST relation
while the decoding worker waits for a running transaction. REPACK has to
fail and leave the table alone. Without the fix, it succeeds and the
concurrent updates of TOASTed columns are lost.
Discussion: https://postgr.es/m/CAA-aLv5MF6BLL+BWvix2Yw+CBardtH43AofPReQunhDZPNBtuA@mail.gmail.com
---
.../expected/repack_toast.out | 151 +++++++++++++++++-
.../injection_points/specs/repack_toast.spec | 49 ++++++
2 files changed, 199 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..81634339745 100644
--- a/src/test/modules/injection_points/expected/repack_toast.out
+++ b/src/test/modules/injection_points/expected/repack_toast.out
@@ -1,4 +1,4 @@
-Parsed test spec with 2 sessions
+Parsed test spec with 3 sessions
starting permutation: s1_wait_before_lock s2_updates s2_check s2_wakeup_before_lock s1_check
injection_points_attach
@@ -124,3 +124,152 @@ injection_points_detach
(1 row)
+
+starting permutation: s2_begin s1_wait_before_lock s3_rewrite_toast s2_commit s2_updates s2_check s2_wakeup_before_lock_if_waiting s1_check
+injection_points_attach
+-----------------------
+
+(1 row)
+
+step s2_begin:
+ BEGIN;
+ SELECT pg_current_xact_id() IS NOT NULL AS has_xid;
+
+has_xid
+-------
+t
+(1 row)
+
+step s1_wait_before_lock:
+ REPACK (CONCURRENTLY) repack_toast;
+ <waiting ...>
+step s3_rewrite_toast:
+ DO $$
+ BEGIN
+ EXECUTE format('REPACK %s',
+ (SELECT reltoastrelid::regclass FROM pg_class
+ WHERE relname = 'repack_toast'));
+ END;
+ $$;
+
+step s2_commit:
+ COMMIT;
+
+step s1_wait_before_lock: <... completed>
+ERROR: could not execute REPACK (CONCURRENTLY) on relation "repack_toast"
+step s2_updates:
+ DELETE FROM repack_toast WHERE i=1;
+ INSERT INTO repack_toast(i, j, k) VALUES (1, gen_external(), gen_compressible(1));
+
+ -- existing toast data unchanged. (This covers the case where we
+ -- adjust the toast pointer.)
+ UPDATE repack_toast SET i=i+300 where i % 10 = 2 RETURNING OLD.i, NEW.i;
+
+ -- "j" is here an external indirect, written to the file separately.
+ UPDATE repack_toast SET j=gen_external() where i % 10 = 3 RETURNING OLD.i, NEW.i;
+
+ -- the updated value of "j" is compressed.
+ UPDATE repack_toast SET j=gen_compressible(1), k=k||'' where i % 10 = 4 RETURNING i;
+
+ -- the updated value of "j" is compressed externally.
+ UPDATE repack_toast SET j=gen_compressible_external(2) where i % 10 = 5 RETURNING i;
+
+ -- the updated value of "j" stays inline.
+ UPDATE repack_toast SET j=gen_inline(), k=repeat(k,5) where i % 10 = 6 RETURNING i;
+
+ -- updated value of "j" is a short varlena; "k" is written separately.
+ UPDATE repack_toast SET j=gen_short(), k=gen_external() where i % 10 = 7 RETURNING i;
+
+ i| i
+--+---
+ 2|302
+12|312
+(2 rows)
+
+ i| i
+--+--
+ 3| 3
+13|13
+(2 rows)
+
+ i
+--
+ 4
+14
+(2 rows)
+
+ i
+--
+ 5
+15
+(2 rows)
+
+ i
+--
+ 6
+16
+(2 rows)
+
+ i
+--
+ 7
+17
+(2 rows)
+
+step s2_check:
+ INSERT INTO relfilenodes(node)
+ SELECT c2.relfilenode
+ FROM pg_class c1 JOIN pg_class c2 ON c2.oid = c1.oid OR c2.oid = c1.reltoastrelid
+ WHERE c1.relname='repack_toast';
+
+ INSERT INTO data_s2(i, j, j_toast, k, k_toast)
+ SELECT i, j, COALESCE(pg_column_toast_chunk_id(j), 0) AS j_toast,
+ k, COALESCE(pg_column_toast_chunk_id(k), 0) AS k_toast
+ FROM repack_toast;
+
+step s2_wakeup_before_lock_if_waiting:
+ SELECT injection_points_wakeup('repack-concurrently-before-lock')
+ FROM pg_stat_activity
+ WHERE wait_event_type = 'InjectionPoint' AND
+ wait_event = 'repack-concurrently-before-lock';
+
+injection_points_wakeup
+-----------------------
+(0 rows)
+
+step s1_check:
+ INSERT INTO relfilenodes(node)
+ SELECT c2.relfilenode
+ FROM pg_class c1 JOIN pg_class c2 ON c2.oid = c1.oid OR c2.oid = c1.reltoastrelid
+ WHERE c1.relname='repack_toast';
+
+ SELECT count(DISTINCT node) FROM relfilenodes;
+
+ INSERT INTO data_s1(i, j, j_toast, k, k_toast)
+ SELECT i,
+ j, COALESCE(pg_column_toast_chunk_id(j), 0) AS j_toast,
+ k, COALESCE(pg_column_toast_chunk_id(k), 0) AS k_toast
+ FROM repack_toast;
+
+ -- this should be empty
+ SELECT d1.i, substring(d1.j FOR 12) AS d1_j, substring(d1.k FOR 12) AS d1_k,
+ d2.i, substring(d2.j FOR 12) AS d2_j, substring(d2.k FOR 12) AS d2_k,
+ d1.j_toast as d1_j_tst, d2.j_toast as d2_j_tst,
+ d1.k_toast as d1_k_tst, d2.k_toast AS d2_k_tst
+ FROM data_s1 d1 FULL JOIN data_s2 d2 USING (i, j, k)
+ WHERE d1.i ISNULL OR d2.i ISNULL;
+
+count
+-----
+ 2
+(1 row)
+
+i|d1_j|d1_k|i|d2_j|d2_k|d1_j_tst|d2_j_tst|d1_k_tst|d2_k_tst
+-+----+----+-+----+----+--------+--------+--------+--------
+(0 rows)
+
+injection_points_detach
+-----------------------
+
+(1 row)
+
diff --git a/src/test/modules/injection_points/specs/repack_toast.spec b/src/test/modules/injection_points/specs/repack_toast.spec
index cc8f034d016..a5cf148d97d 100644
--- a/src/test/modules/injection_points/specs/repack_toast.spec
+++ b/src/test/modules/injection_points/specs/repack_toast.spec
@@ -125,6 +125,18 @@ teardown
session s2
+# Keep a transaction with XID open, so that the decoding worker has to wait
+# before it can build the initial snapshot.
+step s2_begin
+{
+ BEGIN;
+ SELECT pg_current_xact_id() IS NOT NULL AS has_xid;
+}
+step s2_commit
+{
+ COMMIT;
+}
+
# Test different kinds of toast data changes.
step s2_updates
{
@@ -169,6 +181,31 @@ step s2_wakeup_before_lock
{
SELECT injection_points_wakeup('repack-concurrently-before-lock');
}
+# Like above, but only if REPACK got that far.
+step s2_wakeup_before_lock_if_waiting
+{
+ SELECT injection_points_wakeup('repack-concurrently-before-lock')
+ FROM pg_stat_activity
+ WHERE wait_event_type = 'InjectionPoint' AND
+ wait_event = 'repack-concurrently-before-lock';
+}
+
+# Rewrite the TOAST relation. The decoding worker only decodes the changes
+# of the TOAST relation stored under the relfilenumber it saw when starting,
+# so REPACK must fail if the TOAST relation got rewritten before REPACK locked
+# it. Otherwise the TOAST chunks of the concurrent changes are not decoded,
+# and the changes are lost.
+session s3
+step s3_rewrite_toast
+{
+ DO $$
+ BEGIN
+ EXECUTE format('REPACK %s',
+ (SELECT reltoastrelid::regclass FROM pg_class
+ WHERE relname = 'repack_toast'));
+ END;
+ $$;
+}
# Test if data changes introduced while one session is performing REPACK
# CONCURRENTLY find their way into the table.
@@ -178,3 +215,15 @@ permutation
s2_check
s2_wakeup_before_lock
s1_check
+
+# Same, but rewrite the TOAST relation while the decoding worker waits for s2
+# to commit. REPACK must fail and leave the table alone.
+permutation
+ s2_begin
+ s1_wait_before_lock
+ s3_rewrite_toast
+ s2_commit
+ s2_updates
+ s2_check
+ s2_wakeup_before_lock_if_waiting
+ s1_check
--
2.37.1 (Apple Git-137.1)
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: REPACK (CONCURRENTLY) can silently lose updates when the toast table is rewritten
@ 2026-09-24 03:08 Manu <manuelreyesbravo@gmail.com>
parent: shihao zhong <zhong950419@gmail.com>
0 siblings, 0 replies; 17+ messages in thread
From: Manu @ 2026-09-24 03:08 UTC (permalink / raw)
To: shihao zhong <zhong950419@gmail.com>; +Cc: Masahiko Sawada <sawada.mshk@gmail.com>; Antonin Houska <ah@cybertec.at>; Thom Brown <thom@linux.com>; pgsql-hackers@lists.postgresql.org
Hi Shihao,
> Agreed. v2 attached. REPACK now fails if the TOAST table was
> rewritten, and the user can run it again.
I ran the same checks as for v1 against v2, on cff329240ba with
--enable-cassert --enable-injection-points, next to a control build
with only v2-0002. Both patches apply cleanly and build with no
warnings.
1. The race without an injection point (VACUUM FULL of the TOAST
relation while the worker waits, UPDATE right after), 5 runs each:
control: REPACK succeeds, update lost in 5 of 5
v2: REPACK fails in 5 of 5, update kept in 5 of 5
ERROR: could not execute REPACK (CONCURRENTLY) on relation "test"
DETAIL: The TOAST relation was rewritten concurrently.
HINT: The transaction might succeed if retried.
2. VACUUM FULL of the TOAST relation in a loop for 20 s, over the
whole startup: v2 fails after 3.6 s with the same error, and the
update is kept. With v1 the same run took about 20 s and 166 worker
restarts, so the unbounded wait I mentioned for v1 is gone.
3. No rewrite at all: REPACK succeeds in 3 of 3, 2.5-2.6 s, the same
as the control.
4. Thom's deadlock case, where a transaction that already has an XID
locks the TOAST relation while the worker waits for it:
REINDEX TABLE of the TOAST relation (lock, no rewrite)
v2: REPACK succeeds, no deadlock, update kept
CLUSTER of the TOAST relation (lock and rewrite)
control: REPACK succeeds, update lost
v2: REPACK fails with the error above, update kept
So taking the lock after the worker's setup does what the commit
message says: no deadlock, and a clear error when the rewrite does
happen.
5. Tests: repack_toast fails on the control and passes with v2. With
v2 all injection_points tests pass (4 regress, 14 isolation), and so
do make check (239) and src/test/isolation (133).
The script is attached (.txt, so the cfbot keeps testing your
patches).
Regards,
Manu
#!/usr/bin/env bash
# v2 of the fix: REPACK (CONCURRENTLY) now errors out if the TOAST relation
# was rewritten while the decoding worker was starting, instead of retrying.
# Every case runs against a build and reports REPACK's own outcome (ok or
# its error), how long it took, and the final value of the updated rows.
#
# race Thom's case without an injection point: an open transaction
# keeps the worker waiting, VACUUM FULL rewrites the TOAST
# relation meanwhile, an UPDATE of the TOASTed column commits
# right after the transaction ends. N attempts.
# hammer VACUUM FULL of the TOAST relation in a loop for SECS seconds,
# over the whole startup of REPACK (the v1 retry loop spun here).
# none the same, with no rewrite at all: the normal path.
# xidlock a transaction that already has an XID locks the TOAST
# relation (REINDEX of it) while the worker waits for it: taking
# the TOAST lock before starting the worker deadlocks here.
# xidrewrite the same, but the transaction rewrites the TOAST relation
# (CLUSTER of it) before committing.
#
# v2_check.sh <install dir> [case ...]
set -u
B=$1; shift
CASES=${*:-race hammer none xidlock xidrewrite}
N=${N:-5}
SECS=${SECS:-20}
D=${D:-$HOME/pgprog/data_v2check}
P=${P:-55711}
LOG=$HOME/pgprog/v2check.log
OUT=$HOME/pgprog/v2check-repack.out
"$B/bin/pg_ctl" -D "$D" -m immediate -w stop >/dev/null 2>&1
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
deadlock_timeout = 1s
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; }
fresh() { # a new table with three TOASTed rows; sets TOAST
q "DROP TABLE IF EXISTS test" >/dev/null
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'")
}
start_repack() { # in the background; sets REPACK and T0
T0=$(date +%s.%N)
( q "REPACK (CONCURRENTLY) test" > "$OUT" 2>&1; date +%s.%N > "$OUT.end" ) &
REPACK=$!
}
report() { # label
local limit=$((SECS + 60)) waited=0
while kill -0 $REPACK 2>/dev/null && [ $waited -lt $limit ]; do sleep 1; waited=$((waited+1)); done
wait 2>/dev/null
local secs=$(echo "$(cat "$OUT.end" 2>/dev/null || date +%s.%N) - $T0" | bc)
local repack=$(grep -m1 -E 'ERROR|FATAL' "$OUT" | sed 's/^.*\(ERROR\|FATAL\): *//')
local value=$(q "SELECT string_agg(DISTINCT left(big, 9), ',') FROM test")
printf ' %-12s REPACK %-50s %5.1fs value %s\n' "$1" "${repack:-ok}" "$secs" "$value"
[ -z "$repack" ] || grep -E '^(DETAIL|HINT):' "$OUT" | sed 's/^/ /'
}
open_xact() { # seconds: a transaction with an XID, closed after the sleep
( "$B/bin/psql" -p $P -U postgres -qtAX \
-c "BEGIN" -c "SELECT pg_current_xact_id()" -c "SELECT pg_sleep($1)" -c "COMMIT" >/dev/null 2>&1 ) &
XACT=$!
}
update_new() {
q "UPDATE test SET big = repeat('NEW', 4000) WHERE id IN (1,2,3)" >/dev/null
}
echo "== build: $B ($("$B/bin/postgres" --version))"
for c in $CASES; do
case $c in
race)
for i in $(seq 1 $N); do
fresh; open_xact 3; sleep 0.5
start_repack; sleep 1
q "VACUUM FULL $TOAST" >/dev/null
wait $XACT; update_new
report "race $i"
done ;;
hammer)
fresh; open_xact 4; sleep 0.5
( until_t=$((SECONDS + SECS))
while [ $SECONDS -lt $until_t ]; do q "VACUUM FULL $TOAST" >/dev/null; done ) &
HAMMER=$!
start_repack
wait $XACT; update_new
report "hammer ${SECS}s"
wait $HAMMER 2>/dev/null ;;
none)
for i in $(seq 1 3); do
fresh; open_xact 3; sleep 0.5
start_repack
wait $XACT; update_new
report "none $i"
done ;;
xidlock|xidrewrite)
fresh
# LOCK TABLE is refused on a TOAST relation, so use commands that
# lock it for real: REINDEX takes ShareLock on it without a rewrite,
# CLUSTER rewrites it (new relfilenumber).
if [ $c = xidlock ]; then
stmt="REINDEX TABLE $TOAST"
else
idx=$(q "SELECT c.relname FROM pg_index i JOIN pg_class c ON c.oid = i.indexrelid WHERE i.indrelid = '$TOAST'::regclass")
stmt="CLUSTER $TOAST USING $idx"
fi
( "$B/bin/psql" -p $P -U postgres -qtAX \
-c "BEGIN" -c "INSERT INTO test VALUES (100, 'x')" -c "SELECT pg_sleep(1.5)" \
-c "$stmt" -c "SELECT pg_sleep(1)" -c "COMMIT" > "$OUT.s1" 2>&1 ) &
S1=$!
sleep 0.5; start_repack
wait $S1
s1=$(grep -m1 -E 'ERROR' "$OUT.s1" | sed 's/^.*ERROR: *//')
update_new
report "$c"
echo " session 1 ($stmt): ${s1:-ok}" ;;
esac
done
grep -E 'deadlock detected' "$LOG" | head -3 | sed 's/^/ log: /'
"$B/bin/pg_ctl" -D "$D" -m immediate -w stop >/dev/null 2>&1
Attachments:
[text/plain] nocfbot-repack-toast-v2-check.sh.txt (4.9K, ../../179021928483.3690793.10906772966940835615@gmail.com/2-nocfbot-repack-toast-v2-check.sh.txt)
download | inline:
#!/usr/bin/env bash
# v2 of the fix: REPACK (CONCURRENTLY) now errors out if the TOAST relation
# was rewritten while the decoding worker was starting, instead of retrying.
# Every case runs against a build and reports REPACK's own outcome (ok or
# its error), how long it took, and the final value of the updated rows.
#
# race Thom's case without an injection point: an open transaction
# keeps the worker waiting, VACUUM FULL rewrites the TOAST
# relation meanwhile, an UPDATE of the TOASTed column commits
# right after the transaction ends. N attempts.
# hammer VACUUM FULL of the TOAST relation in a loop for SECS seconds,
# over the whole startup of REPACK (the v1 retry loop spun here).
# none the same, with no rewrite at all: the normal path.
# xidlock a transaction that already has an XID locks the TOAST
# relation (REINDEX of it) while the worker waits for it: taking
# the TOAST lock before starting the worker deadlocks here.
# xidrewrite the same, but the transaction rewrites the TOAST relation
# (CLUSTER of it) before committing.
#
# v2_check.sh <install dir> [case ...]
set -u
B=$1; shift
CASES=${*:-race hammer none xidlock xidrewrite}
N=${N:-5}
SECS=${SECS:-20}
D=${D:-$HOME/pgprog/data_v2check}
P=${P:-55711}
LOG=$HOME/pgprog/v2check.log
OUT=$HOME/pgprog/v2check-repack.out
"$B/bin/pg_ctl" -D "$D" -m immediate -w stop >/dev/null 2>&1
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
deadlock_timeout = 1s
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; }
fresh() { # a new table with three TOASTed rows; sets TOAST
q "DROP TABLE IF EXISTS test" >/dev/null
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'")
}
start_repack() { # in the background; sets REPACK and T0
T0=$(date +%s.%N)
( q "REPACK (CONCURRENTLY) test" > "$OUT" 2>&1; date +%s.%N > "$OUT.end" ) &
REPACK=$!
}
report() { # label
local limit=$((SECS + 60)) waited=0
while kill -0 $REPACK 2>/dev/null && [ $waited -lt $limit ]; do sleep 1; waited=$((waited+1)); done
wait 2>/dev/null
local secs=$(echo "$(cat "$OUT.end" 2>/dev/null || date +%s.%N) - $T0" | bc)
local repack=$(grep -m1 -E 'ERROR|FATAL' "$OUT" | sed 's/^.*\(ERROR\|FATAL\): *//')
local value=$(q "SELECT string_agg(DISTINCT left(big, 9), ',') FROM test")
printf ' %-12s REPACK %-50s %5.1fs value %s\n' "$1" "${repack:-ok}" "$secs" "$value"
[ -z "$repack" ] || grep -E '^(DETAIL|HINT):' "$OUT" | sed 's/^/ /'
}
open_xact() { # seconds: a transaction with an XID, closed after the sleep
( "$B/bin/psql" -p $P -U postgres -qtAX \
-c "BEGIN" -c "SELECT pg_current_xact_id()" -c "SELECT pg_sleep($1)" -c "COMMIT" >/dev/null 2>&1 ) &
XACT=$!
}
update_new() {
q "UPDATE test SET big = repeat('NEW', 4000) WHERE id IN (1,2,3)" >/dev/null
}
echo "== build: $B ($("$B/bin/postgres" --version))"
for c in $CASES; do
case $c in
race)
for i in $(seq 1 $N); do
fresh; open_xact 3; sleep 0.5
start_repack; sleep 1
q "VACUUM FULL $TOAST" >/dev/null
wait $XACT; update_new
report "race $i"
done ;;
hammer)
fresh; open_xact 4; sleep 0.5
( until_t=$((SECONDS + SECS))
while [ $SECONDS -lt $until_t ]; do q "VACUUM FULL $TOAST" >/dev/null; done ) &
HAMMER=$!
start_repack
wait $XACT; update_new
report "hammer ${SECS}s"
wait $HAMMER 2>/dev/null ;;
none)
for i in $(seq 1 3); do
fresh; open_xact 3; sleep 0.5
start_repack
wait $XACT; update_new
report "none $i"
done ;;
xidlock|xidrewrite)
fresh
# LOCK TABLE is refused on a TOAST relation, so use commands that
# lock it for real: REINDEX takes ShareLock on it without a rewrite,
# CLUSTER rewrites it (new relfilenumber).
if [ $c = xidlock ]; then
stmt="REINDEX TABLE $TOAST"
else
idx=$(q "SELECT c.relname FROM pg_index i JOIN pg_class c ON c.oid = i.indexrelid WHERE i.indrelid = '$TOAST'::regclass")
stmt="CLUSTER $TOAST USING $idx"
fi
( "$B/bin/psql" -p $P -U postgres -qtAX \
-c "BEGIN" -c "INSERT INTO test VALUES (100, 'x')" -c "SELECT pg_sleep(1.5)" \
-c "$stmt" -c "SELECT pg_sleep(1)" -c "COMMIT" > "$OUT.s1" 2>&1 ) &
S1=$!
sleep 0.5; start_repack
wait $S1
s1=$(grep -m1 -E 'ERROR' "$OUT.s1" | sed 's/^.*ERROR: *//')
update_new
report "$c"
echo " session 1 ($stmt): ${s1:-ok}" ;;
esac
done
grep -E 'deadlock detected' "$LOG" | head -3 | sed 's/^/ log: /'
"$B/bin/pg_ctl" -D "$D" -m immediate -w stop >/dev/null 2>&1
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: REPACK (CONCURRENTLY) can silently lose updates when the toast table is rewritten
@ 2026-09-24 03:51 Robert Treat <rob@xzilla.net>
parent: Masahiko Sawada <sawada.mshk@gmail.com>
1 sibling, 1 reply; 17+ messages in thread
From: Robert Treat @ 2026-09-24 03:51 UTC (permalink / raw)
To: Masahiko Sawada <sawada.mshk@gmail.com>; +Cc: Antonin Houska <ah@cybertec.at>; shihao zhong <zhong950419@gmail.com>; Manu <manuelreyesbravo@gmail.com>; Thom Brown <thom@linux.com>; pgsql-hackers@lists.postgresql.org
On Wed, Sep 23, 2026 at 2:28 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Sep 23, 2026 at 9:23 AM Antonin Houska <ah@cybertec.at> wrote:
> >
> > shihao zhong <zhong950419@gmail.com> wrote:
> >
> > > > or whether the relfilenode should be re-checked after the snapshot is built
> > >
> > > Holding the toast lock from the start deadlocks. A session that asks for
> > > AccessExclusiveLock gets an XID before it waits, and the decoding worker
> > > waits for all XIDs while it sets up.
> >
> > The same (supposedly low) deadlock risk already exists for the main table, see
> > this comment in rebuild_relation():
> >
> > /*
> > * Start the worker that decodes data changes applied while we're
> > * copying the table contents.
> > *
> > * Note that the worker has to wait for all transactions with XID
> > * already assigned to finish. If some of those transactions is
> > * waiting for a lock conflicting with ShareUpdateExclusiveLock on our
> > * table (e.g. it runs CREATE INDEX), we can end up in a deadlock.
> > * Not sure this risk is worth unlocking/locking the table (and its
> > * clustering index) and checking again if it's still eligible for
> > * REPACK CONCURRENTLY.
> > */
> > start_repack_decoding_worker(tableOid);
> >
> > I'm not sure if locking the TOAST relation earlier would make the situation
> > worse.
>
> Agreed.
>
> So I think the simplest fix would be to acquire a lock on the TOAST
> table before starting the repack worker. It would make the case in
> question fail with a deadlock, instead of silently losing updates.
>
> The proposed patch also fixes the problem, but I'm concerned that it
> repeatedly starts and stops the repack worker without any limit. I
> think we could error out if we detect a concurrent rewrite, so that
> users can re-run REPACK CONCURRENTLY. This check could also be done on
> the repack worker side: after getting the relfilelocator of the TOAST
> table and initializing the logical decoding, the repack worker
> rechecks the relfilelocator. If they don't match, it raises an error.
>
It feels a little off to me that if I am trying to REPACKCC, and
someone (maybe even myself, but certainly not Postgres) comes along
and runs a command the conflicts with my existing REPACKCC, that my
REPACKCC is canceled rather than having the other command either wait
or error out. That's a little more complicated a fix, with likely
heavier and/or longer held locks, but feels like it would be less
surprising for users.
Robert Treat
https://xzilla.net
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: REPACK (CONCURRENTLY) can silently lose updates when the toast table is rewritten
@ 2026-09-24 07:57 Antonin Houska <ah@cybertec.at>
parent: Thom Brown <thom@linux.com>
0 siblings, 1 reply; 17+ messages in thread
From: Antonin Houska @ 2026-09-24 07:57 UTC (permalink / raw)
To: Thom Brown <thom@linux.com>; +Cc: shihao zhong <zhong950419@gmail.com>; Manu <manuelreyesbravo@gmail.com>; pgsql-hackers@lists.postgresql.org
Thom Brown <thom@linux.com> wrote:
> On Wed, 23 Sept 2026 at 17:22, Antonin Houska <ah@cybertec.at> wrote:
> >
> > shihao zhong <zhong950419@gmail.com> wrote:
> >
> > > > or whether the relfilenode should be re-checked after the snapshot is built
> > >
> > > Holding the toast lock from the start deadlocks. A session that asks for
> > > AccessExclusiveLock gets an XID before it waits, and the decoding worker
> > > waits for all XIDs while it sets up.
> >
> > The same (supposedly low) deadlock risk already exists for the main table, see
> > this comment in rebuild_relation():
> >
> > /*
> > * Start the worker that decodes data changes applied while we're
> > * copying the table contents.
> > *
> > * Note that the worker has to wait for all transactions with XID
> > * already assigned to finish. If some of those transactions is
> > * waiting for a lock conflicting with ShareUpdateExclusiveLock on our
> > * table (e.g. it runs CREATE INDEX), we can end up in a deadlock.
> > * Not sure this risk is worth unlocking/locking the table (and its
> > * clustering index) and checking again if it's still eligible for
> > * REPACK CONCURRENTLY.
> > */
> > start_repack_decoding_worker(tableOid);
> >
> > I'm not sure if locking the TOAST relation earlier would make the situation
> > worse.
> >
> > The reason TOAST relation is not locked until copy_table_data() does so is
> > that CLUSTER / VACUUM FULL in v18 did it this way (not sure what the reason
> > for such design was). I haven't changed that for REPACK exactly because I
> > failed to envision this stale relfilenode issue.
>
> I gave that a try, and it does. It just swaps the lost update for a deadlock.
>
> If you lock the toast up front and something rewrites it at the same
> time (which is the thing that triggers this in the first place, e.g. a
> REPACK of the toast table), REPACK falls over:
>
> Session 1:
> BEGIN;
> INSERT INTO test VALUES (999999, 'x');
>
> Session 2:
> REPACK (CONCURRENTLY) test;
>
> Session 1:
> CREATE INDEX ON test (big);
>
> ERROR: deadlock detected
> DETAIL: Process 214534 waits for ShareLock on transaction 1774005;
> blocked by process 214579.
> Process 214579 waits for AccessExclusiveLock on relation 3672470 of
> database 5; blocked by process 214534.
> CONTEXT: REPACK decoding worker
>
> The rewrite already has an XID by the time it waits, and the worker
> waits for that XID whilst it sets up, so the two just sit on each
> other. It doesn't matter which lock we take either because anything
> that would stop the rewrite conflicts with it.
IMO this example does not exactly demonstrate the problem described in the
comment above: if REPACK (CONCURRENTLY) waits for AccessExclusiveLock, it's
going to perform the relation swap, so the worker should already be gone.
On the other hand, the message
"Process ... waits for ShareLock on transaction ..."
is what the deadlock detector would report for the decoding worker. However,
where would the request for AccessExclusiveLock come from in that case? CREATE
INDEX only uses it to lock the new index relation, however that cannot be
locked by other backends until the transaction has committed (because it's not
visible before commit).
What exactly have you changed in the code?
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: REPACK (CONCURRENTLY) can silently lose updates when the toast table is rewritten
@ 2026-09-24 08:32 Thom Brown <thom@linux.com>
parent: Antonin Houska <ah@cybertec.at>
0 siblings, 0 replies; 17+ messages in thread
From: Thom Brown @ 2026-09-24 08:32 UTC (permalink / raw)
To: Antonin Houska <ah@cybertec.at>; +Cc: shihao zhong <zhong950419@gmail.com>; Manu <manuelreyesbravo@gmail.com>; pgsql-hackers@lists.postgresql.org
On Thu, 24 Sept 2026 at 08:57, Antonin Houska <ah@cybertec.at> wrote:
>
> Thom Brown <thom@linux.com> wrote:
>
> > On Wed, 23 Sept 2026 at 17:22, Antonin Houska <ah@cybertec.at> wrote:
> > >
> > > shihao zhong <zhong950419@gmail.com> wrote:
> > >
> > > > > or whether the relfilenode should be re-checked after the snapshot is built
> > > >
> > > > Holding the toast lock from the start deadlocks. A session that asks for
> > > > AccessExclusiveLock gets an XID before it waits, and the decoding worker
> > > > waits for all XIDs while it sets up.
> > >
> > > The same (supposedly low) deadlock risk already exists for the main table, see
> > > this comment in rebuild_relation():
> > >
> > > /*
> > > * Start the worker that decodes data changes applied while we're
> > > * copying the table contents.
> > > *
> > > * Note that the worker has to wait for all transactions with XID
> > > * already assigned to finish. If some of those transactions is
> > > * waiting for a lock conflicting with ShareUpdateExclusiveLock on our
> > > * table (e.g. it runs CREATE INDEX), we can end up in a deadlock.
> > > * Not sure this risk is worth unlocking/locking the table (and its
> > > * clustering index) and checking again if it's still eligible for
> > > * REPACK CONCURRENTLY.
> > > */
> > > start_repack_decoding_worker(tableOid);
> > >
> > > I'm not sure if locking the TOAST relation earlier would make the situation
> > > worse.
> > >
> > > The reason TOAST relation is not locked until copy_table_data() does so is
> > > that CLUSTER / VACUUM FULL in v18 did it this way (not sure what the reason
> > > for such design was). I haven't changed that for REPACK exactly because I
> > > failed to envision this stale relfilenode issue.
> >
> > I gave that a try, and it does. It just swaps the lost update for a deadlock.
> >
> > If you lock the toast up front and something rewrites it at the same
> > time (which is the thing that triggers this in the first place, e.g. a
> > REPACK of the toast table), REPACK falls over:
> >
> > Session 1:
> > BEGIN;
> > INSERT INTO test VALUES (999999, 'x');
> >
> > Session 2:
> > REPACK (CONCURRENTLY) test;
> >
> > Session 1:
> > CREATE INDEX ON test (big);
> >
> > ERROR: deadlock detected
> > DETAIL: Process 214534 waits for ShareLock on transaction 1774005;
> > blocked by process 214579.
> > Process 214579 waits for AccessExclusiveLock on relation 3672470 of
> > database 5; blocked by process 214534.
> > CONTEXT: REPACK decoding worker
> >
> > The rewrite already has an XID by the time it waits, and the worker
> > waits for that XID whilst it sets up, so the two just sit on each
> > other. It doesn't matter which lock we take either because anything
> > that would stop the rewrite conflicts with it.
>
> IMO this example does not exactly demonstrate the problem described in the
> comment above: if REPACK (CONCURRENTLY) waits for AccessExclusiveLock, it's
> going to perform the relation swap, so the worker should already be gone.
>
> On the other hand, the message
>
> "Process ... waits for ShareLock on transaction ..."
>
> is what the deadlock detector would report for the decoding worker. However,
> where would the request for AccessExclusiveLock come from in that case? CREATE
> INDEX only uses it to lock the new index relation, however that cannot be
> locked by other backends until the transaction has committed (because it's not
> visible before commit).
>
> What exactly have you changed in the code?
Apologies, I seem to have incorrectly paired tests with different
results during my copy and pasting.
I'll see if I can untangle it later today.
Thom
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: REPACK (CONCURRENTLY) can silently lose updates when the toast table is rewritten
@ 2026-09-24 08:41 Antonin Houska <ah@cybertec.at>
parent: Robert Treat <rob@xzilla.net>
0 siblings, 0 replies; 17+ messages in thread
From: Antonin Houska @ 2026-09-24 08:41 UTC (permalink / raw)
To: Robert Treat <rob@xzilla.net>; +Cc: Masahiko Sawada <sawada.mshk@gmail.com>; shihao zhong <zhong950419@gmail.com>; Manu <manuelreyesbravo@gmail.com>; Thom Brown <thom@linux.com>; pgsql-hackers@lists.postgresql.org
Robert Treat <rob@xzilla.net> wrote:
> On Wed, Sep 23, 2026 at 2:28 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Wed, Sep 23, 2026 at 9:23 AM Antonin Houska <ah@cybertec.at> wrote:
> > >
> > > shihao zhong <zhong950419@gmail.com> wrote:
> > >
> > > > > or whether the relfilenode should be re-checked after the snapshot is built
> > > >
> > > > Holding the toast lock from the start deadlocks. A session that asks for
> > > > AccessExclusiveLock gets an XID before it waits, and the decoding worker
> > > > waits for all XIDs while it sets up.
> > >
> > > The same (supposedly low) deadlock risk already exists for the main table, see
> > > this comment in rebuild_relation():
> > >
> > > /*
> > > * Start the worker that decodes data changes applied while we're
> > > * copying the table contents.
> > > *
> > > * Note that the worker has to wait for all transactions with XID
> > > * already assigned to finish. If some of those transactions is
> > > * waiting for a lock conflicting with ShareUpdateExclusiveLock on our
> > > * table (e.g. it runs CREATE INDEX), we can end up in a deadlock.
> > > * Not sure this risk is worth unlocking/locking the table (and its
> > > * clustering index) and checking again if it's still eligible for
> > > * REPACK CONCURRENTLY.
> > > */
> > > start_repack_decoding_worker(tableOid);
> > >
> > > I'm not sure if locking the TOAST relation earlier would make the situation
> > > worse.
> >
> > Agreed.
> >
> > So I think the simplest fix would be to acquire a lock on the TOAST
> > table before starting the repack worker. It would make the case in
> > question fail with a deadlock, instead of silently losing updates.
> >
> > The proposed patch also fixes the problem, but I'm concerned that it
> > repeatedly starts and stops the repack worker without any limit. I
> > think we could error out if we detect a concurrent rewrite, so that
> > users can re-run REPACK CONCURRENTLY. This check could also be done on
> > the repack worker side: after getting the relfilelocator of the TOAST
> > table and initializing the logical decoding, the repack worker
> > rechecks the relfilelocator. If they don't match, it raises an error.
> >
>
> It feels a little off to me that if I am trying to REPACKCC, and
> someone (maybe even myself, but certainly not Postgres) comes along
> and runs a command the conflicts with my existing REPACKCC, that my
> REPACKCC is canceled rather than having the other command either wait
> or error out.
pg_squeeze gives up as soon as it notices a "disrupting" catalog
change. Although I haven't heard complaints about this behavior (it's probably
not common to run conflicting DDL commands during maintenance window), I admit
it's not the ideal approach.
For REPACK (CONCURRENTLY), we decided to not give up voluntarily. Even if
REPACK ends up in a deadlock, it still has some chance to win. The direction
we took here is to adjust the deadlock detector (in future versions) so that
REPACK always wins. Raising ERROR on REPACK's side in case of specific
conflict would be against that strategy.
(What I said does not mean that I'm in favor of restarting the decoding worker
either. I still prefer locking the TOAST relation early, as I noted elsewhere
in the thread.)
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
^ permalink raw reply [nested|flat] 17+ messages in thread
end of thread, other threads:[~2026-09-24 08:41 UTC | newest]
Thread overview: 17+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-09-23 00:23 REPACK (CONCURRENTLY) can silently lose updates when the toast table is rewritten Thom Brown <thom@linux.com>
2026-09-23 00:42 ` Manu <manuelreyesbravo@gmail.com>
2026-09-23 05:09 ` shihao zhong <zhong950419@gmail.com>
2026-09-23 08:26 ` Thom Brown <thom@linux.com>
2026-09-23 12:08 ` shihao zhong <zhong950419@gmail.com>
2026-09-23 14:16 ` Manu <manuelreyesbravo@gmail.com>
2026-09-23 16:22 ` Antonin Houska <ah@cybertec.at>
2026-09-23 17:18 ` Thom Brown <thom@linux.com>
2026-09-24 07:57 ` Antonin Houska <ah@cybertec.at>
2026-09-24 08:32 ` Thom Brown <thom@linux.com>
2026-09-23 18:27 ` Masahiko Sawada <sawada.mshk@gmail.com>
2026-09-24 01:45 ` shihao zhong <zhong950419@gmail.com>
2026-09-24 03:08 ` Manu <manuelreyesbravo@gmail.com>
2026-09-24 03:51 ` Robert Treat <rob@xzilla.net>
2026-09-24 08:41 ` Antonin Houska <ah@cybertec.at>
2026-09-23 15:06 ` Melanie Plageman <melanieplageman@gmail.com>
2026-09-23 17:09 ` Thom Brown <thom@linux.com>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox