From c8c4feb560f46591a1e942362fd5ab34276d692a Mon Sep 17 00:00:00 2001 From: Shihao 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 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)