From e6736779f45ceddbccbee156e80b9e91155ba111 Mon Sep 17 00:00:00 2001 From: Hou Zhijie Date: Wed, 21 Jun 2023 11:16:43 +0800 Subject: [PATCH] disallow reloading config when holding the page lock There is an existing rule that we don't acquire any other heavyweight lock while holding the page lock except for relation extension. The reason for the existing rule for page lock and relation extension locks was to not allow them to participate in group locking which will allow other parallel operations like a parallel vacuum where multiple workers can work on the same index, or parallel inserts, parallel copy, etc. However, commit 7d71d3dd08 (in PG16) began reloading the config file after acquiring the page lock during autovacuum. The problem arose when, after acquiring the page lock (a heavyweight lock), we attempted to access the catalog cache while reloading the config file. This action attempted to acquire a lock on the catalog relation, breaking the existing rule and resulting in an assertion failure. To address this problem, we disallow the reloading of the config file while holding the page lock. --- contrib/bloom/blvacuum.c | 4 ++-- contrib/file_fdw/file_fdw.c | 2 +- src/backend/access/gin/ginfast.c | 14 +++++++++++--- src/backend/access/gin/ginvacuum.c | 6 +++--- src/backend/access/gist/gistvacuum.c | 2 +- src/backend/access/hash/hash.c | 2 +- src/backend/access/heap/vacuumlazy.c | 6 +++--- src/backend/access/nbtree/nbtree.c | 2 +- src/backend/access/spgist/spgvacuum.c | 4 ++-- src/backend/commands/analyze.c | 10 +++++----- src/backend/commands/vacuum.c | 5 +++-- src/backend/tsearch/ts_typanalyze.c | 2 +- src/backend/utils/adt/array_typanalyze.c | 2 +- src/backend/utils/adt/rangetypes_typanalyze.c | 2 +- src/include/commands/vacuum.h | 2 +- 15 files changed, 37 insertions(+), 28 deletions(-) diff --git a/contrib/bloom/blvacuum.c b/contrib/bloom/blvacuum.c index 2340d49e00..7e633b2ced 100644 --- a/contrib/bloom/blvacuum.c +++ b/contrib/bloom/blvacuum.c @@ -61,7 +61,7 @@ blbulkdelete(IndexVacuumInfo *info, IndexBulkDeleteResult *stats, *itupPtr, *itupEnd; - vacuum_delay_point(); + vacuum_delay_point(true); buffer = ReadBufferExtended(index, MAIN_FORKNUM, blkno, RBM_NORMAL, info->strategy); @@ -191,7 +191,7 @@ blvacuumcleanup(IndexVacuumInfo *info, IndexBulkDeleteResult *stats) Buffer buffer; Page page; - vacuum_delay_point(); + vacuum_delay_point(true); buffer = ReadBufferExtended(index, MAIN_FORKNUM, blkno, RBM_NORMAL, info->strategy); diff --git a/contrib/file_fdw/file_fdw.c b/contrib/file_fdw/file_fdw.c index 9e330b9934..7216566739 100644 --- a/contrib/file_fdw/file_fdw.c +++ b/contrib/file_fdw/file_fdw.c @@ -1176,7 +1176,7 @@ file_acquire_sample_rows(Relation onerel, int elevel, for (;;) { /* Check for user-requested abort or sleep */ - vacuum_delay_point(); + vacuum_delay_point(true); /* Fetch next row */ MemoryContextReset(tupcontext); diff --git a/src/backend/access/gin/ginfast.c b/src/backend/access/gin/ginfast.c index ca7d770d86..029a1a5635 100644 --- a/src/backend/access/gin/ginfast.c +++ b/src/backend/access/gin/ginfast.c @@ -886,7 +886,15 @@ ginInsertCleanup(GinState *ginstate, bool full_clean, */ processPendingPage(&accum, &datums, page, FirstOffsetNumber); - vacuum_delay_point(); + /* + * It is not safe to reload the config file while holding the page lock + * as this may attempt to acquire a lock on the catalog relation, + * leading to a violation of the rule that we cannot acquire any other + * heavyweight lock while holding the page lock, except for relation + * extension locks. Please refer to the comments atop IsPageLockHeld + * for further details. + */ + vacuum_delay_point(false); /* * Is it time to flush memory to disk? Flush if we are at the end of @@ -923,7 +931,7 @@ ginInsertCleanup(GinState *ginstate, bool full_clean, { ginEntryInsert(ginstate, attnum, key, category, list, nlist, NULL); - vacuum_delay_point(); + vacuum_delay_point(false); } /* @@ -996,7 +1004,7 @@ ginInsertCleanup(GinState *ginstate, bool full_clean, /* * Read next page in pending list */ - vacuum_delay_point(); + vacuum_delay_point(false); buffer = ReadBuffer(index, blkno); LockBuffer(buffer, GIN_SHARE); page = BufferGetPage(buffer); diff --git a/src/backend/access/gin/ginvacuum.c b/src/backend/access/gin/ginvacuum.c index e5d310d836..1fdc143c6a 100644 --- a/src/backend/access/gin/ginvacuum.c +++ b/src/backend/access/gin/ginvacuum.c @@ -663,12 +663,12 @@ ginbulkdelete(IndexVacuumInfo *info, IndexBulkDeleteResult *stats, UnlockReleaseBuffer(buffer); } - vacuum_delay_point(); + vacuum_delay_point(true); for (i = 0; i < nRoot; i++) { ginVacuumPostingTree(&gvs, rootOfPostingTree[i]); - vacuum_delay_point(); + vacuum_delay_point(true); } if (blkno == InvalidBlockNumber) /* rightmost page */ @@ -749,7 +749,7 @@ ginvacuumcleanup(IndexVacuumInfo *info, IndexBulkDeleteResult *stats) Buffer buffer; Page page; - vacuum_delay_point(); + vacuum_delay_point(true); buffer = ReadBufferExtended(index, MAIN_FORKNUM, blkno, RBM_NORMAL, info->strategy); diff --git a/src/backend/access/gist/gistvacuum.c b/src/backend/access/gist/gistvacuum.c index 3f60d3274d..6fe7d0d629 100644 --- a/src/backend/access/gist/gistvacuum.c +++ b/src/backend/access/gist/gistvacuum.c @@ -283,7 +283,7 @@ restart: recurse_to = InvalidBlockNumber; /* call vacuum_delay_point while not holding any buffer lock */ - vacuum_delay_point(); + vacuum_delay_point(true); buffer = ReadBufferExtended(rel, MAIN_FORKNUM, blkno, RBM_NORMAL, info->strategy); diff --git a/src/backend/access/hash/hash.c b/src/backend/access/hash/hash.c index fc5d97f606..d4b19749f8 100644 --- a/src/backend/access/hash/hash.c +++ b/src/backend/access/hash/hash.c @@ -714,7 +714,7 @@ hashbucketcleanup(Relation rel, Bucket cur_bucket, Buffer bucket_buf, bool retain_pin = false; bool clear_dead_marking = false; - vacuum_delay_point(); + vacuum_delay_point(true); page = BufferGetPage(buf); opaque = HashPageGetOpaque(page); diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 4eb953f904..bf1a46b974 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -889,7 +889,7 @@ lazy_scan_heap(LVRelState *vacrel) update_vacuum_error_info(vacrel, NULL, VACUUM_ERRCB_PHASE_SCAN_HEAP, blkno, InvalidOffsetNumber); - vacuum_delay_point(); + vacuum_delay_point(true); /* * Regularly check if wraparound failsafe should trigger. @@ -1353,7 +1353,7 @@ lazy_scan_skip(LVRelState *vacrel, Buffer *vmbuffer, BlockNumber next_block, skipsallvis = true; } - vacuum_delay_point(); + vacuum_delay_point(true); next_unskippable_block++; nskippable_blocks++; } @@ -2438,7 +2438,7 @@ lazy_vacuum_heap_rel(LVRelState *vacrel) Page page; Size freespace; - vacuum_delay_point(); + vacuum_delay_point(true); blkno = ItemPointerGetBlockNumber(&vacrel->dead_items->items[index]); vacrel->blkno = blkno; diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c index 4553aaee53..a89038f6cf 100644 --- a/src/backend/access/nbtree/nbtree.c +++ b/src/backend/access/nbtree/nbtree.c @@ -1058,7 +1058,7 @@ backtrack: backtrack_to = P_NONE; /* call vacuum_delay_point while not holding any buffer lock */ - vacuum_delay_point(); + vacuum_delay_point(true); /* * We can't use _bt_getbuf() here because it always applies diff --git a/src/backend/access/spgist/spgvacuum.c b/src/backend/access/spgist/spgvacuum.c index 8a5b540c80..5dd440123a 100644 --- a/src/backend/access/spgist/spgvacuum.c +++ b/src/backend/access/spgist/spgvacuum.c @@ -615,7 +615,7 @@ spgvacuumpage(spgBulkDeleteState *bds, BlockNumber blkno) Page page; /* call vacuum_delay_point while not holding any buffer lock */ - vacuum_delay_point(); + vacuum_delay_point(true); buffer = ReadBufferExtended(index, MAIN_FORKNUM, blkno, RBM_NORMAL, bds->info->strategy); @@ -694,7 +694,7 @@ spgprocesspending(spgBulkDeleteState *bds) continue; /* ignore already-done items */ /* call vacuum_delay_point while not holding any buffer lock */ - vacuum_delay_point(); + vacuum_delay_point(true); /* examine the referenced page */ blkno = ItemPointerGetBlockNumber(&pitem->tid); diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c index 52ef462dba..af81035b5f 100644 --- a/src/backend/commands/analyze.c +++ b/src/backend/commands/analyze.c @@ -889,7 +889,7 @@ compute_index_stats(Relation onerel, double totalrows, { HeapTuple heapTuple = rows[rowno]; - vacuum_delay_point(); + vacuum_delay_point(true); /* * Reset the per-tuple context each time, to reclaim any cruft @@ -1220,7 +1220,7 @@ acquire_sample_rows(Relation onerel, int elevel, prefetch_targblock = BlockSampler_Next(&prefetch_bs); #endif - vacuum_delay_point(); + vacuum_delay_point(true); block_accepted = table_scan_analyze_next_block(scan, targblock, vac_strategy); @@ -1957,7 +1957,7 @@ compute_trivial_stats(VacAttrStatsP stats, Datum value; bool isnull; - vacuum_delay_point(); + vacuum_delay_point(true); value = fetchfunc(stats, i, &isnull); @@ -2073,7 +2073,7 @@ compute_distinct_stats(VacAttrStatsP stats, int firstcount1, j; - vacuum_delay_point(); + vacuum_delay_point(true); value = fetchfunc(stats, i, &isnull); @@ -2420,7 +2420,7 @@ compute_scalar_stats(VacAttrStatsP stats, Datum value; bool isnull; - vacuum_delay_point(); + vacuum_delay_point(true); value = fetchfunc(stats, i, &isnull); diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index a843f9ad92..706d811636 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -2323,7 +2323,7 @@ vac_close_indexes(int nindexes, Relation *Irel, LOCKMODE lockmode) * typically once per page processed. */ void -vacuum_delay_point(void) +vacuum_delay_point(bool vacuum_config_reload_safe) { double msec = 0; @@ -2340,7 +2340,8 @@ vacuum_delay_point(void) * [autovacuum_]vacuum_cost_delay to take effect while a table is being * vacuumed or analyzed. */ - if (ConfigReloadPending && IsAutoVacuumWorkerProcess()) + if (ConfigReloadPending && IsAutoVacuumWorkerProcess() && + vacuum_config_reload_safe) { ConfigReloadPending = false; ProcessConfigFile(PGC_SIGHUP); diff --git a/src/backend/tsearch/ts_typanalyze.c b/src/backend/tsearch/ts_typanalyze.c index 75ecd72efe..466597b242 100644 --- a/src/backend/tsearch/ts_typanalyze.c +++ b/src/backend/tsearch/ts_typanalyze.c @@ -206,7 +206,7 @@ compute_tsvector_stats(VacAttrStats *stats, char *lexemesptr; int j; - vacuum_delay_point(); + vacuum_delay_point(true); value = fetchfunc(stats, vector_no, &isnull); diff --git a/src/backend/utils/adt/array_typanalyze.c b/src/backend/utils/adt/array_typanalyze.c index 52e160d6bb..2db192d754 100644 --- a/src/backend/utils/adt/array_typanalyze.c +++ b/src/backend/utils/adt/array_typanalyze.c @@ -314,7 +314,7 @@ compute_array_stats(VacAttrStats *stats, AnalyzeAttrFetchFunc fetchfunc, int distinct_count; bool count_item_found; - vacuum_delay_point(); + vacuum_delay_point(true); value = fetchfunc(stats, array_no, &isnull); if (isnull) diff --git a/src/backend/utils/adt/rangetypes_typanalyze.c b/src/backend/utils/adt/rangetypes_typanalyze.c index 86810a1a6e..687ad401fc 100644 --- a/src/backend/utils/adt/rangetypes_typanalyze.c +++ b/src/backend/utils/adt/rangetypes_typanalyze.c @@ -169,7 +169,7 @@ compute_range_stats(VacAttrStats *stats, AnalyzeAttrFetchFunc fetchfunc, upper; float8 length; - vacuum_delay_point(); + vacuum_delay_point(true); value = fetchfunc(stats, range_no, &isnull); if (isnull) diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h index 17e9b4f68e..463b0e4478 100644 --- a/src/include/commands/vacuum.h +++ b/src/include/commands/vacuum.h @@ -339,7 +339,7 @@ extern bool vacuum_get_cutoffs(Relation rel, const VacuumParams *params, struct VacuumCutoffs *cutoffs); extern bool vacuum_xid_failsafe_check(const struct VacuumCutoffs *cutoffs); extern void vac_update_datfrozenxid(void); -extern void vacuum_delay_point(void); +extern void vacuum_delay_point(bool vacuum_config_reload_safe); extern bool vacuum_is_permitted_for_relation(Oid relid, Form_pg_class reltuple, bits32 options); extern Relation vacuum_open_relation(Oid relid, RangeVar *relation, -- 2.30.0.windows.2