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