shihao zhong <[email protected]> wrote: > Subject: REPACK (CONCURRENTLY): do not block the table while waiting for the > final lock
[ I assume this is not meant for v19, is it? ] > > REPACK (CONCURRENTLY) requests AccessExclusiveLock at the end to swap > the files. While it is queued for that lock behind a long running > transaction, every new query on the table queues behind it. On master I > held AccessShareLock in one session for 15 seconds, and a single row > SELECT that arrived while REPACK was waiting took 14.7 seconds. The > usual defense, lock_timeout, makes it worse here. With lock_timeout = 3s > the REPACK fails after all the copying is done. > > The attached patch makes REPACK stop queueing for that lock. It checks > whether the lock is available. While it is not, it applies the changes > that arrived meanwhile and checks again every 50 ms, the way > lazy_truncate_heap() does. With the patch the same SELECT took 0.9 > seconds, and REPACK finished once the long transaction ended. > lock_timeout limits how long it keeps trying, so its meaning does not > change. > > The cost is on the REPACK side. A conditional request only succeeds when > nobody holds a lock at that moment, so on a busy table it can take a > while. With 16 pgbench clients doing single row SELECTs on the table > (about 135k tps), REPACK needed 1 to 10 seconds to get the lock, against > 0.5 seconds when queueing. After hours of copying I think that is fine. > > There is a variant with a lock manager change that queues for > deadlock_timeout at a time and then leaves the queue. Waiting for a limited time would make more sense, but why exactly deadlock_timeout should control that? Attached is my proposal to restrict the wait time, w/o hacking the lock manager. Note that it deliberately does not teach REPACK to give up. Unlike (lazy) VACUUM, all the work is rolled back if REPACK ends with ERROR. -- Antonin Houska Web: https://www.cybertec-postgresql.com
>From da282accfdd2891b0c6689d94f002061879d5b35 Mon Sep 17 00:00:00 2001 From: Antonin Houska <[email protected]> Date: Tue, 6 Oct 2026 20:06:15 +0200 Subject: [PATCH] Limit the time for REPACK (CONCURRENTLY) to stay in the lock queue. While the backend executing REPACK (CONCURRENTLY) is waiting for the lock for the final processing (AccessExclusiveLock), backends that joined the wait queue later must wait as well, even if they are requesting much weaker lock. This patch applies a timeout to the REPACK's lock acquisition. Once the timeout has fired, the backend executing REPACK is removed from the queue and requests the lock again. Thus the other backends are not stuck in the queue anymore. --- src/backend/commands/repack.c | 72 ++++++++++++++++++++++++++++++++++- 1 file changed, 71 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/repack.c b/src/backend/commands/repack.c index 899005609c2..49516d0a637 100644 --- a/src/backend/commands/repack.c +++ b/src/backend/commands/repack.c @@ -212,6 +212,8 @@ static void rebuild_relation_finish_concurrent(Relation NewHeap, Relation OldHea Oid identIdx, TransactionId frozenXid, MultiXactId cutoffMulti); +static void lock_relation_with_timeout(Oid relid, int timeout, + ChangeContext *chgcxt); static List *build_new_indexes(Relation NewHeap, Relation OldHeap, List *OldIndexes); static void copy_index_constraints(Relation old_index, Oid new_index_id, Oid new_heap_id); @@ -3472,8 +3474,12 @@ rebuild_relation_finish_concurrent(Relation NewHeap, Relation OldHeap, /* * Acquire AccessExclusiveLock on the table, its TOAST relation (if there * is one), all its indexes, so that we can swap the files. + * + * TODO Tune the timeout - the longer we wait, the more concurrent changes + * we need to process while holding the lock. Does it deserve a new GUC + * parameter? */ - LockRelationOid(old_table_oid, AccessExclusiveLock); + lock_relation_with_timeout(old_table_oid, 3000, &chgcxt); /* * Lock all indexes now, not only the clustering one: all indexes need to @@ -3591,6 +3597,70 @@ rebuild_relation_finish_concurrent(Relation NewHeap, Relation OldHeap, relpersistence); } +/* + * Lock relation with AccessExclusiveLock, but never wait longer than + * 'timeout' milliseconds. + */ +static void +lock_relation_with_timeout(Oid relid, int timeout, ChangeContext *chgcxt) +{ + int LockTimeout_save; + MemoryContext edata_context, oldcxt; + XLogRecPtr end_of_wal; + + edata_context = AllocSetContextCreate(TopTransactionContext, + "RepackLockError", + ALLOCSET_DEFAULT_SIZES); + oldcxt = CurrentMemoryContext; + while (true) + { + bool acquired = false; + + LockTimeout_save = LockTimeout; + LockTimeout = timeout; + + PG_TRY(); + { + LockRelationOid(relid, AccessExclusiveLock); + + acquired = true; + } + PG_CATCH(); + { + ErrorData *edata; + + LockTimeout = LockTimeout_save; + + /* Save error info in caller's context */ + MemoryContextSwitchTo(edata_context); + edata = CopyErrorData(); + FlushErrorState(); + MemoryContextSwitchTo(oldcxt); + + /* + * LOCK_NOT_AVAILABLE is what we expect on timeout, anything else + * is worth re-throwing. + */ + if (edata->sqlerrcode != ERRCODE_LOCK_NOT_AVAILABLE) + ReThrowError(edata); + MemoryContextReset(edata_context); + } + PG_END_TRY(); + LockTimeout = LockTimeout_save; + + if (acquired) + goto out; + + /* Process the data changes that appeared during the waiting. */ + XLogFlush(GetXLogInsertEndRecPtr()); + end_of_wal = GetFlushRecPtr(NULL); + process_concurrent_changes(end_of_wal, chgcxt, false); + } + +out: + MemoryContextDelete(edata_context); +} + /* * Build indexes on NewHeap according to those on OldHeap. * -- 2.52.0
