From 85b2bcff30c776d8487fe686e83594a034af4da6 Mon Sep 17 00:00:00 2001 From: Shihao Date: Thu, 24 Sep 2026 23:46:58 -0400 Subject: [PATCH v4 2/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 before the worker starts, like the table itself. A rewrite that comes during the startup now waits for REPACK. The rewrite has an XID by then, so if the worker still waits for running transactions, the two can deadlock, and the deadlock detector cancels one of them. DDL on the table itself has the same risk already. Backpatch to v19, where REPACK (CONCURRENTLY) was introduced. Reported-by: Thom Brown Discussion: https://postgr.es/m/CAA-aLv5MF6BLL+BWvix2Yw+CBardtH43AofPReQunhDZPNBtuA@mail.gmail.com --- src/backend/commands/repack.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index b748426930a..386797915be 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -536,6 +536,15 @@ cluster_rel(RepackCommand cmd, Relation OldHeap, Oid indexOid, if (concurrent) check_concurrent_repack_requirements(OldHeap, &ident_idx); + /* + * In concurrent mode, also lock the toast table. Otherwise it would be + * possible for the toast relfilenode to change (e.g. because VACUUM FULL + * is run on it), and then logical decoding would fail to detect any + * concurrent changes there. + */ + if (concurrent && OidIsValid(OldHeap->rd_rel->reltoastrelid)) + LockRelationOid(OldHeap->rd_rel->reltoastrelid, lmode); + /* * Also check the state of indexes; this can abort the command for REPACK. * Historically this hasn't affected CLUSTER or VACUUM FULL, so don't do @@ -1136,6 +1145,11 @@ rebuild_relation(Relation OldHeap, Relation index, bool verbose, */ BecomeLockGroupLeader(); + /* If there is a toast table, it must have been locked already */ + Assert(!OidIsValid(OldHeap->rd_rel->reltoastrelid) || + CheckRelationOidLockedByMe(OldHeap->rd_rel->reltoastrelid, + lmode, false)); + /* * Start the worker that decodes data changes applied while we're * copying the table contents. @@ -1143,10 +1157,10 @@ rebuild_relation(Relation OldHeap, Relation index, bool verbose, * 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. + * table or its TOAST relation (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); -- 2.47.3