From c9324e22409e064202872dfa1c559f539094e531 Mon Sep 17 00:00:00 2001 From: Shihao Date: Thu, 24 Sep 2026 23:46:58 -0400 Subject: [PATCH v3 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 | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index 759be53d6b8..03fb4d5f0b9 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -1129,6 +1129,20 @@ rebuild_relation(Relation OldHeap, Relation index, bool verbose, */ BecomeLockGroupLeader(); + /* + * Lock the TOAST relation before the worker starts. The worker only + * decodes the changes of the TOAST relation stored under the + * relfilenumber it sees when it starts. If the TOAST relation got + * rewritten after that (VACUUM FULL can be run on it directly), the + * TOAST chunks of concurrent changes would not be decoded, and a + * changed TOASTed value would be taken for an unchanged one when + * applying the changes. copy_table_data() locks the TOAST relation + * too, but that's too late for this purpose. + */ + if (OidIsValid(OldHeap->rd_rel->reltoastrelid)) + LockRelationOid(OldHeap->rd_rel->reltoastrelid, + ShareUpdateExclusiveLock); + /* * Start the worker that decodes data changes applied while we're * copying the table contents. @@ -1136,10 +1150,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.37.1 (Apple Git-137.1)