From 3356a88cb6c1138ef03463c4ff0c2a22546eb958 Mon Sep 17 00:00:00 2001 From: Shihao Date: Fri, 25 Sep 2026 10:05:22 -0400 Subject: [PATCH v4 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 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 | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index b748426930a..7f22d4cbf3e 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -659,6 +659,18 @@ cluster_rel(RepackCommand cmd, Relation OldHeap, Oid indexOid, OldHeap->rd_rel->relkind == RELKIND_MATVIEW || OldHeap->rd_rel->relkind == RELKIND_TOASTVALUE); + /* + * Lock the TOAST relation too. In the concurrent case, the decoding + * worker only decodes TOAST changes stored under the relfilenumber it + * sees at startup, so the relation must not be rewritten from then on. + * AccessShareLock is enough for that. A stronger lock could deadlock + * with the transactions the worker waits for, so rebuild_relation() + * upgrades it after the worker has started. + */ + if (OidIsValid(OldHeap->rd_rel->reltoastrelid)) + LockRelationOid(OldHeap->rd_rel->reltoastrelid, + concurrent ? AccessShareLock : lmode); + /* * All predicate locks on the tuples or pages are about to be made * invalid, because we move tuples around. Promote them to relation @@ -1156,6 +1168,11 @@ rebuild_relation(Relation OldHeap, Relation index, bool verbose, snapshot = get_initial_snapshot(decoding_worker); PushActiveSnapshot(snapshot); + + /* Now that the worker is done waiting, upgrade the TOAST lock. */ + if (OidIsValid(OldHeap->rd_rel->reltoastrelid)) + LockRelationOid(OldHeap->rd_rel->reltoastrelid, + ShareUpdateExclusiveLock); } /* for CLUSTER or REPACK USING INDEX, mark the index as the one to use */ @@ -1402,9 +1419,6 @@ copy_table_data(Relation NewHeap, Relation OldHeap, Relation OldIndex, PGRUsage ru0; char *nspname; bool concurrent = snapshot != NULL; - LOCKMODE lmode; - - lmode = RepackLockLevel(concurrent); pg_rusage_init(&ru0); @@ -1420,7 +1434,7 @@ copy_table_data(Relation NewHeap, Relation OldHeap, Relation OldIndex, Assert(newTupDesc->natts == oldTupDesc->natts); /* - * If the OldHeap has a toast table, get lock on the toast table to keep + * If the OldHeap has a toast table, callers must have locked it to keep * it from being vacuumed. This is needed because autovacuum processes * toast tables independently of their main tables, with no lock on the * latter. If an autovacuum were to start on the toast table after we @@ -1428,12 +1442,10 @@ copy_table_data(Relation NewHeap, Relation OldHeap, Relation OldIndex, * possibly remove as DEAD toast tuples belonging to main tuples we think * are only RECENTLY_DEAD. Then we'd fail while trying to copy those * tuples. - * - * We don't need to open the toast relation here, just lock it. The lock - * will be held till end of transaction. */ - if (OldHeap->rd_rel->reltoastrelid) - LockRelationOid(OldHeap->rd_rel->reltoastrelid, lmode); + Assert(!OidIsValid(OldHeap->rd_rel->reltoastrelid) || + CheckRelationOidLockedByMe(OldHeap->rd_rel->reltoastrelid, + RepackLockLevel(concurrent), false)); /* * If both tables have TOAST tables, perform toast swap by content. It is -- 2.37.1 (Apple Git-137.1)