From 80cdf4f3407d34bdcfc862a8b1150ab53524d706 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Sat, 3 Oct 2026 00:53:18 +0300 Subject: [PATCH v3 1/1] Don't call vacuum_delay_point() while holding locks It's a bad idea to sleep while holding locks, because you might block another process that wants to acquire the same lock. So avoid calling vacuum_delay_point() while holding locks. There were a few places that did that, which I found by adding an "Assert(INTERRUPTS_CAN_BE_PROCESSED())" into vacuum_delay_point() and running the regression tests. I didn't include that Assert in this commit, because there might be more places that do that that are not covered by the regression tests, including extensions. But I did add a runtime check that bails out of vacuum_delay_point() quickly if interrupts cannot be processed at the time. That prevents the sleep that might block others, and it also avoids potentially calling ProcessConfigFile() while in a critical section, if vacuum_delay_point() was ever called in one. In ginInsertCleanup(), there was already another vacuum_delay_point() call later in the loop, while not holding any locks, so just remove the other call that was made while holding the lock. In hashbucketcleanup(), move the vacuum_delay_point() call up the stack to its caller. The call in hashbucketcleanup() was not able to handle interrupts because it held a lock, and because there were no vacuum_delay_point() or CHECK_FOR_INTERRUPTS() calls in the caller's loop, the whole hash index vacuuming phase was uninterruptible by pending shutdown or query cancel. Now it can be interrupted between buckets. Unfortunately, hashbucketcleanup() has to process all the bucket's pages in one go without pausing; fixing that would require changing how hashbucketcleanup()'s lock chaining works. In acquire_sample_rows(), move the vacuum_delay_point() call in the loop to between pages, to a time where we're not holding the buffer lock. Reported-by: Sergei Kornilov Author: Mostafa Reviewed-by: Kirill Reshke Discussion: https://postgr.es/m/19628-c2b17d358181a1ea@postgresql.org --- src/backend/access/gin/ginfast.c | 6 +++--- src/backend/access/hash/hash.c | 5 +++-- src/backend/commands/analyze.c | 3 +-- src/backend/commands/vacuum.c | 8 ++++++++ 4 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/backend/access/gin/ginfast.c b/src/backend/access/gin/ginfast.c index 46fc60115a8..3779b43fa1a 100644 --- a/src/backend/access/gin/ginfast.c +++ b/src/backend/access/gin/ginfast.c @@ -895,8 +895,6 @@ ginInsertCleanup(GinState *ginstate, bool must_empty_list, */ processPendingPage(&accum, &datums, page, FirstOffsetNumber); - vacuum_delay_point(false); - /* * Is it time to flush memory to disk? Flush if we are at the end of * the pending list, or if we have a full row and memory is getting @@ -1002,10 +1000,12 @@ ginInsertCleanup(GinState *ginstate, bool must_empty_list, UnlockReleaseBuffer(buffer); } + /* call vacuum_delay_point while not holding any buffer lock */ + vacuum_delay_point(false); + /* * Read next page in pending list */ - vacuum_delay_point(false); buffer = ReadBuffer(index, blkno); LockBuffer(buffer, GIN_SHARE); page = BufferGetPage(buffer); diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c index b2e34d2d45e..63846c10840 100644 --- a/src/backend/access/hash/hash.c +++ b/src/backend/access/hash/hash.c @@ -562,6 +562,9 @@ bucket_loop: Page page; bool split_cleanup = false; + /* call vacuum_delay_point while not holding any buffer lock */ + vacuum_delay_point(false); + /* Get address of bucket's start page */ bucket_blkno = BUCKET_TO_BLKNO(cachedmetap, cur_bucket); @@ -799,8 +802,6 @@ hashbucketcleanup(Relation rel, Bucket cur_bucket, Buffer bucket_buf, bool retain_pin = false; bool clear_dead_marking = false; - vacuum_delay_point(false); - page = BufferGetPage(buf); opaque = HashPageGetOpaque(page); diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c index d0498b14da1..518d7526255 100644 --- a/src/backend/commands/analyze.c +++ b/src/backend/commands/analyze.c @@ -1339,8 +1339,6 @@ acquire_sample_rows(Relation onerel, int elevel, /* Outer loop over blocks to sample */ while (table_scan_analyze_next_block(scan, stream)) { - vacuum_delay_point(true); - while (table_scan_analyze_next_tuple(scan, &liverows, &deadrows, slot)) { /* @@ -1388,6 +1386,7 @@ acquire_sample_rows(Relation onerel, int elevel, pgstat_progress_update_param(PROGRESS_ANALYZE_BLOCKS_DONE, ++blksdone); + vacuum_delay_point(true); } read_stream_end(stream); diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index d8c2f33c615..a257dd8d21e 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -2482,6 +2482,14 @@ vacuum_delay_point(bool is_analyze) { double msec = 0; + /* + * If we're holding locks or holding interrupts for some other reason, + * don't sleep, because we don't want to hold locks any longer than + * necessary. + */ + if (!INTERRUPTS_CAN_BE_PROCESSED()) + return; + /* Always check for interrupts */ CHECK_FOR_INTERRUPTS(); -- 2.47.3