Assert while autovacuum was executing
Hi,
I have been testing 16beta1, last commit
a14e75eb0b6a73821e0d66c0d407372ec8376105
I just let sqlsmith do its magic before trying something else, and
today I found a core with the attached backtrace.
Only information on the log was this:
DETAIL: Failed process was running: autovacuum: VACUUM
public.array_index_op_test
--
Jaime Casanova
Director de Servicios Profesionales
SYSTEMGUARDS - Consultores de PostgreSQL
Attachments:
On Sat, Jun 17, 2023 at 11:29 AM Jaime Casanova
<jcasanov@systemguards.com.ec> wrote:
I have been testing 16beta1, last commit
a14e75eb0b6a73821e0d66c0d407372ec8376105
I just let sqlsmith do its magic before trying something else, and
today I found a core with the attached backtrace.
The assertion that fails is the IsPageLockHeld assertion from commit 72e78d831a.
I think that this is kind of an odd assertion. It's also not justified
by any comments. Why invent this rule at all?
To be fair the use of page heavyweight locks in ginInsertCleanup() is
also odd. The only reason why ginInsertCleanup() uses page-level locks
here is to get the benefit of deadlock detection, and to be able to
hold the lock for a relatively long time if that proves necessary
(i.e., interruptibility). There are reasons to doubt that that's a
good design, but either way it seems fundamentally incompatible with
the rule enforced by the assertion.
--
Peter Geoghegan
On Sun, Jun 18, 2023 at 12:18 AM Peter Geoghegan <pg@bowt.ie> wrote:
On Sat, Jun 17, 2023 at 11:29 AM Jaime Casanova
<jcasanov@systemguards.com.ec> wrote:I have been testing 16beta1, last commit
a14e75eb0b6a73821e0d66c0d407372ec8376105
I just let sqlsmith do its magic before trying something else, and
today I found a core with the attached backtrace.The assertion that fails is the IsPageLockHeld assertion from commit 72e78d831a.
I'll look into this and share my analysis.
--
With Regards,
Amit Kapila.
On Mon, Jun 19, 2023 at 5:13 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sun, Jun 18, 2023 at 12:18 AM Peter Geoghegan <pg@bowt.ie> wrote:
On Sat, Jun 17, 2023 at 11:29 AM Jaime Casanova
<jcasanov@systemguards.com.ec> wrote:I have been testing 16beta1, last commit
a14e75eb0b6a73821e0d66c0d407372ec8376105
I just let sqlsmith do its magic before trying something else, and
today I found a core with the attached backtrace.The assertion that fails is the IsPageLockHeld assertion from commit 72e78d831a.
I'll look into this and share my analysis.
This failure mode appears to be introduced in commit 7d71d3dd08 (in
PG16) where we started to process the config file after acquiring page
lock during autovacuum. The problem here is that after acquiring page
lock (a heavy-weight lock), while processing the config file, we tried
to access the catalog cache which in turn attempts to acquire a lock
on the catalog relation, and that leads to the assertion failure. This
is because of an existing rule that we don't acquire any other
heavyweight lock while holding the page lock except for relation
extension. I think normally we should be careful about the lock
ordering for heavy-weight locks to avoid deadlocks but here there may
not be any existing hazard in acquiring a lock on the catalog table
after acquiring page lock on the gin index's metapage as I am not
aware of a scenario where we can acquire them in reverse order. One
naive idea is to have a parameter like vacuum_config_reload_safe to
allow config reload during autovacuum and make it false for the gin
index cleanup code path.
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. The related commits are 15ef6ff4b9, 72e78d831ab,
85f6b49c2c, and 3ba59ccc89. See 3ba59ccc89 for more details (To allow
parallel inserts and parallel copy, we have ensured that relation
extension and page locks don't participate in group locking which
means such locks can conflict among the same group members. This is
required as it is no safer for two related processes to extend the
same relation or perform clean up in gin indexes at a time than for
unrelated processes to do the same....).
--
With Regards,
Amit Kapila.
On Tuesday, June 20, 2023 5:44 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Mon, Jun 19, 2023 at 5:13 PM Amit Kapila <amit.kapila16@gmail.com>
wrote:On Sun, Jun 18, 2023 at 12:18 AM Peter Geoghegan <pg@bowt.ie> wrote:
On Sat, Jun 17, 2023 at 11:29 AM Jaime Casanova
<jcasanov@systemguards.com.ec> wrote:I have been testing 16beta1, last commit
a14e75eb0b6a73821e0d66c0d407372ec8376105
I just let sqlsmith do its magic before trying something else, and
today I found a core with the attached backtrace.The assertion that fails is the IsPageLockHeld assertion from commit
72e78d831a.
I'll look into this and share my analysis.
This failure mode appears to be introduced in commit 7d71d3dd08 (in
PG16) where we started to process the config file after acquiring page lock
during autovacuum. The problem here is that after acquiring page lock (a
heavy-weight lock), while processing the config file, we tried to access the
catalog cache which in turn attempts to acquire a lock on the catalog relation,
and that leads to the assertion failure. This is because of an existing rule that we
don't acquire any other heavyweight lock while holding the page lock except
for relation extension. I think normally we should be careful about the lock
ordering for heavy-weight locks to avoid deadlocks but here there may not be
any existing hazard in acquiring a lock on the catalog table after acquiring page
lock on the gin index's metapage as I am not aware of a scenario where we can
acquire them in reverse order. One naive idea is to have a parameter like
vacuum_config_reload_safe to allow config reload during autovacuum and
make it false for the gin index cleanup code path.
I also think it would be better to skip reloading config in page lock cases to be consistent
with the rule. And here is the patch which does the same.
I tried to reproduce the assert failure(before applying the patch) using the following steps:
1. I added a sleep before vacuum_delay_point in ginInsertCleanup and LOG("attach this process") before sleeping.
2. And changed few GUC to make autovacuum happen more frequently and then start the server.
-
autovacuum_naptime = 5s
autovacuum_vacuum_threshold = 1
autovacuum_vacuum_insert_threshold = 100
-
3. Then I execute the following sqls:
-
create table gin_test_tbl(i int4[]);
create index gin_test_idx on gin_test_tbl using gin (i)
with (fastupdate = on, gin_pending_list_limit = 4096);
insert into gin_test_tbl select array[1, 2, g] from generate_series(1, 20000) g;
insert into gin_test_tbl select array[1, 3, g] from generate_series(1, 1000) g;
-
4. After a while, I can see the LOG from autovacuum worker and then use gdb to attach to the autovacuum worker.
5. When the autovacuum worker is blocked, I changed the "default_text_search_config = 'pg_catalog.public'" in configure file and reload it.
6. Release the autovacuum worker and then I can see the assert failure.
And I can see the assert failure doesn't happen after applying the patch.
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. The related commits are 15ef6ff4b9,
72e78d831ab, 85f6b49c2c, and 3ba59ccc89. See 3ba59ccc89 for more details
(To allow parallel inserts and parallel copy, we have ensured that relation
extension and page locks don't participate in group locking which means such
locks can conflict among the same group members. This is required as it is no
safer for two related processes to extend the same relation or perform clean
up in gin indexes at a time than for unrelated processes to do the same....).
Best Regards,
Hou zj
Attachments:
0001-disallow-reloading-config-when-holding-the-page-lock.patchapplication/octet-stream; name=0001-disallow-reloading-config-when-holding-the-page-lock.patchDownload
From e6736779f45ceddbccbee156e80b9e91155ba111 Mon Sep 17 00:00:00 2001
From: Hou Zhijie <houzj.fnst@cn.fujitsu.com>
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
Hi,
On 2023-06-20 15:14:26 +0530, Amit Kapila wrote:
On Mon, Jun 19, 2023 at 5:13 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sun, Jun 18, 2023 at 12:18 AM Peter Geoghegan <pg@bowt.ie> wrote:
On Sat, Jun 17, 2023 at 11:29 AM Jaime Casanova
<jcasanov@systemguards.com.ec> wrote:I have been testing 16beta1, last commit
a14e75eb0b6a73821e0d66c0d407372ec8376105
I just let sqlsmith do its magic before trying something else, and
today I found a core with the attached backtrace.The assertion that fails is the IsPageLockHeld assertion from commit 72e78d831a.
I'll look into this and share my analysis.
This failure mode appears to be introduced in commit 7d71d3dd08 (in
PG16) where we started to process the config file after acquiring page
lock during autovacuum.
I find it somewhat hard to believe that this is the only way to reach this
issue. You're basically asserting that there's not a single cache lookup
reachable from inside ginInsertCleanup() - which seems unlikely, given the
range of comparators that can exist.
<plays around>
Yep. Doesn't even require enabling debug_discard_caches or reconnecting.
DROP TABLE IF EXISTS tbl_foo;
DROP TYPE IF EXISTS Foo;
CREATE TYPE foo AS ENUM ('a', 'b', 'c');
ALTER TYPE foo ADD VALUE 'ab' BEFORE 'b';
CREATE TABLE tbl_foo (foo foo);
CREATE INDEX tbl_foo_idx ON tbl_foo USING gin (foo) WITH (fastupdate = on);
INSERT INTO tbl_foo(foo) VALUES ('ab'), ('a'), ('b'), ('c');
SELECT gin_clean_pending_list('tbl_foo_idx');
As far as I can tell 72e78d831a as-is is just bogus. Unfortunately that likely
also means 3ba59ccc89 is not right.
Greetings,
Andres Freund
On Tue, Jun 20, 2023 at 10:27 PM Andres Freund <andres@anarazel.de> wrote:
As far as I can tell 72e78d831a as-is is just bogus. Unfortunately that likely
also means 3ba59ccc89 is not right.
Quite possibly. But I maintain that ginInsertCleanup() is probably
also bogus in a way that's directly relevant.
Did you know that ginInsertCleanup() is the only code that uses
heavyweight page locks these days? Though only on the index metapage!
Isn't this the kind of thing that VACUUM's relation level lock is
supposed to take care of?
--
Peter Geoghegan
On Wed, Jun 21, 2023 at 10:57 AM Andres Freund <andres@anarazel.de> wrote:
As far as I can tell 72e78d831a as-is is just bogus. Unfortunately that likely
also means 3ba59ccc89 is not right.
Indeed. I was thinking of a fix but couldn't find one yet. One idea I
am considering is to allow catalog table locks after page lock but I
think apart from hacky that also won't work because we still need to
remove the check added for page locks in the deadlock code path in
commit 3ba59ccc89 and may need to do something for group locking. Feel
free to share any ideas if you have, I can try to evaluate those in
detail. I think in the worst case we need to remove the changes added
by 72e78d831a and 3ba59ccc89 which won't impact any existing feature
but will add a hurdle in parallelizing other write operations or even
improving the parallelism in vacuum (like allowing multiple workers
for an index).
--
With Regards,
Amit Kapila.
On Wed, Jun 21, 2023 at 11:53 AM Peter Geoghegan <pg@bowt.ie> wrote:
On Tue, Jun 20, 2023 at 10:27 PM Andres Freund <andres@anarazel.de> wrote:
As far as I can tell 72e78d831a as-is is just bogus. Unfortunately that likely
also means 3ba59ccc89 is not right.Quite possibly. But I maintain that ginInsertCleanup() is probably
also bogus in a way that's directly relevant.Did you know that ginInsertCleanup() is the only code that uses
heavyweight page locks these days? Though only on the index metapage!Isn't this the kind of thing that VACUUM's relation level lock is
supposed to take care of?
Yeah, I also can't see why that shouldn't be sufficient for VACUUM.
Assuming your observation is correct, what do you suggest doing in
this regard?
--
With Regards,
Amit Kapila.
Hi,
On 2023-06-22 10:00:01 +0530, Amit Kapila wrote:
On Wed, Jun 21, 2023 at 11:53 AM Peter Geoghegan <pg@bowt.ie> wrote:
On Tue, Jun 20, 2023 at 10:27 PM Andres Freund <andres@anarazel.de> wrote:
As far as I can tell 72e78d831a as-is is just bogus. Unfortunately that likely
also means 3ba59ccc89 is not right.Quite possibly. But I maintain that ginInsertCleanup() is probably
also bogus in a way that's directly relevant.Did you know that ginInsertCleanup() is the only code that uses
heavyweight page locks these days? Though only on the index metapage!Isn't this the kind of thing that VACUUM's relation level lock is
supposed to take care of?Yeah, I also can't see why that shouldn't be sufficient for VACUUM.
I'd replied on that point to Peter earlier, accidentlly loosing the CC
list. The issue is that ginInsertCleanup() isn't just called from VACUUM, but
also from normal inserts (to reduce the size of the fastupdate list).
You can possibly come up with another scheme, but I think just doing this via
the relation lock might be problematic. Suddenly an insert would, temporarily,
also block operations that don't normally conflict with inserts etc.
Greetings,
Andres Freund
On Thu, Jun 22, 2023 at 9:16 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Wed, Jun 21, 2023 at 10:57 AM Andres Freund <andres@anarazel.de> wrote:
As far as I can tell 72e78d831a as-is is just bogus. Unfortunately that likely
also means 3ba59ccc89 is not right.Indeed. I was thinking of a fix but couldn't find one yet. One idea I
am considering is to allow catalog table locks after page lock but I
think apart from hacky that also won't work because we still need to
remove the check added for page locks in the deadlock code path in
commit 3ba59ccc89 and may need to do something for group locking.
I have further thought about this part and I think even if we remove
the changes in commit 72e78d831a (remove the assertion for page locks
in LockAcquireExtended()) and remove the check added for page locks in
FindLockCycleRecurseMember() via commit 3ba59ccc89, it is still okay
to keep the change related to "allow page lock to conflict among
parallel group members" in LockCheckConflicts(). This is because locks
on catalog tables don't conflict among group members. So, we shouldn't
see a deadlock among parallel group members. Let me try to explain
this thought via an example:
Begin;
Lock pg_enum in Access Exclusive mode;
gin_clean_pending_list() -- assume this function is executed by both
leader and parallel worker; also this requires a lock on pg_enum as
shown by Andres in email [1]/messages/by-id/20230621052713.wc5377dyslxpckfj@awork3.anarazel.de
Say the parallel worker acquires page lock first and it will also get
lock on pg_enum because of group locking, so, the leader backend will
wait for page lock for the parallel worker. Eventually, the parallel
worker will release the page lock and the leader backend can get the
lock. So, we should be still okay with parallelism.
OTOH, if the above theory is wrong or people are not convinced, I am
okay with removing all the changes in commits 72e78d831a and
3ba59ccc89.
[1]: /messages/by-id/20230621052713.wc5377dyslxpckfj@awork3.anarazel.de
--
With Regards,
Amit Kapila.
On Fri, Jun 23, 2023 at 2:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Jun 22, 2023 at 9:16 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Wed, Jun 21, 2023 at 10:57 AM Andres Freund <andres@anarazel.de> wrote:
As far as I can tell 72e78d831a as-is is just bogus. Unfortunately that likely
also means 3ba59ccc89 is not right.Indeed. I was thinking of a fix but couldn't find one yet. One idea I
am considering is to allow catalog table locks after page lock but I
think apart from hacky that also won't work because we still need to
remove the check added for page locks in the deadlock code path in
commit 3ba59ccc89 and may need to do something for group locking.I have further thought about this part and I think even if we remove
the changes in commit 72e78d831a (remove the assertion for page locks
in LockAcquireExtended()) and remove the check added for page locks in
FindLockCycleRecurseMember() via commit 3ba59ccc89, it is still okay
to keep the change related to "allow page lock to conflict among
parallel group members" in LockCheckConflicts(). This is because locks
on catalog tables don't conflict among group members. So, we shouldn't
see a deadlock among parallel group members. Let me try to explain
this thought via an example:
IMHO, whatsoever the case this check[1]/* * The relation extension or page lock conflict even between the group * members. */ if (LOCK_LOCKTAG(*lock) == LOCKTAG_RELATION_EXTEND || (LOCK_LOCKTAG(*lock) == LOCKTAG_PAGE)) { PROCLOCK_PRINT("LockCheckConflicts: conflicting (group)", proclock); return true; }, is not wrong at all. I agree
that we do not have parallel write present in the code so having this
check is not necessary as of now. But in theory, this check is
correct because this is saying that parallel leader and worker should
conflict on the 'relation extension lock' and the 'page lock' and
that's the fact. It holds true irrespective of whether it is being
used currently or not.
[1]: /* * The relation extension or page lock conflict even between the group * members. */ if (LOCK_LOCKTAG(*lock) == LOCKTAG_RELATION_EXTEND || (LOCK_LOCKTAG(*lock) == LOCKTAG_PAGE)) { PROCLOCK_PRINT("LockCheckConflicts: conflicting (group)", proclock); return true; }
/*
* The relation extension or page lock conflict even between the group
* members.
*/
if (LOCK_LOCKTAG(*lock) == LOCKTAG_RELATION_EXTEND ||
(LOCK_LOCKTAG(*lock) == LOCKTAG_PAGE))
{
PROCLOCK_PRINT("LockCheckConflicts: conflicting (group)",
proclock);
return true;
}
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
Hi,
On 2023-06-23 14:04:15 +0530, Amit Kapila wrote:
OTOH, if the above theory is wrong or people are not convinced, I am
okay with removing all the changes in commits 72e78d831a and
3ba59ccc89.
I am not convinced. And even if I were, coming up with new justifications in a
released version, when the existing testing clearly wasn't enough to find the
current bug, doesn't strike me as wise.
Greetings,
Andres Freund
On Fri, Jun 23, 2023 at 10:07 PM Andres Freund <andres@anarazel.de> wrote:
On 2023-06-23 14:04:15 +0530, Amit Kapila wrote:
OTOH, if the above theory is wrong or people are not convinced, I am
okay with removing all the changes in commits 72e78d831a and
3ba59ccc89.I am not convinced. And even if I were, coming up with new justifications in a
released version, when the existing testing clearly wasn't enough to find the
current bug, doesn't strike me as wise.
Fair enough. If we could have been convinced of this then we can keep
the required change only for HEAD. But anyway let's remove the work
related to both commits (72e78d831a and 3ba59ccc89) for now and then
we can come back to it when we parallelize writes. The attached patch
removes the changes added by both commits with slight tweaking in
comments/readme based on the recent state.
--
With Regards,
Amit Kapila.
Attachments:
v1-0001-Revert-the-commits-related-to-allowing-page-lock-.patchapplication/octet-stream; name=v1-0001-Revert-the-commits-related-to-allowing-page-lock-.patchDownload
From 9e1ffb0980a7566698237435c976dcb5bd6281d8 Mon Sep 17 00:00:00 2001
From: Amit Kapila <akapila@postgresql.org>
Date: Mon, 26 Jun 2023 08:39:34 +0530
Subject: [PATCH v1] Revert the commits related to allowing page lock to
conflict among parallel group members.
This commit reverts the work done by commits 3ba59ccc89 and 72e78d831a.
Those commits were incorrect in asserting that we never acquire any other
heavy-weight lock after acquring page lock other than relation extension
lock. We can acquire a lock on catalogs while doing catalog look up after
acquring page lock.
This won't impact any existing feature but we need to think some other way
to achieve this before parallelizing other write operations or even
improving the parallelism in vacuum (like allowing multiple workers
for an index).
Reported-by: Jaime Casanova
Author: Amit Kapila
Backpatch-through: 13
Discussion: https://postgr.es/m/CAJKUy5jffnRKNvRHKQ0LynRb0RJC-o4P8Ku3x9vGAVLwDBWumQ@mail.gmail.com
---
src/backend/optimizer/plan/planner.c | 12 ++++++----
src/backend/storage/lmgr/README | 34 +++++++++++-----------------
src/backend/storage/lmgr/deadlock.c | 9 ++++----
src/backend/storage/lmgr/lock.c | 32 ++++----------------------
src/backend/storage/lmgr/proc.c | 12 +++++-----
5 files changed, 34 insertions(+), 65 deletions(-)
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index 1e4dd27dba..7d679996e3 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -330,11 +330,13 @@ standard_planner(Query *parse, const char *query_string, int cursorOptions,
* functions are present in the query tree.
*
* (Note that we do allow CREATE TABLE AS, SELECT INTO, and CREATE
- * MATERIALIZED VIEW to use parallel plans, but as of now, only the leader
- * backend writes into a completely new table. In the future, we can
- * extend it to allow workers to write into the table. However, to allow
- * parallel updates and deletes, we have to solve other problems,
- * especially around combo CIDs.)
+ * MATERIALIZED VIEW to use parallel plans, but this is safe only because
+ * the command is writing into a completely new table which workers won't
+ * be able to see. If the workers could see the table, the fact that
+ * group locking would cause them to ignore the leader's heavyweight
+ * GIN page locks would make this unsafe. We'll have to fix that somehow
+ * if we want to allow parallel inserts in general; updates and deletes
+ * have additional problems especially around combo CIDs.)
*
* For now, we don't try to use parallel mode if we're running inside a
* parallel worker. We might eventually be able to relax this
diff --git a/src/backend/storage/lmgr/README b/src/backend/storage/lmgr/README
index d08ec6c402..45de0fd2bd 100644
--- a/src/backend/storage/lmgr/README
+++ b/src/backend/storage/lmgr/README
@@ -597,10 +597,10 @@ deadlock detection algorithm very much, but it makes the bookkeeping more
complicated.
We choose to regard locks held by processes in the same parallel group as
-non-conflicting with the exception of relation extension and page locks. This
-means that two processes in a parallel group can hold a self-exclusive lock on
-the same relation at the same time, or one process can acquire an AccessShareLock
-while the other already holds AccessExclusiveLock. This might seem dangerous and
+non-conflicting with the exception of relation extension lock. This means that
+two processes in a parallel group can hold a self-exclusive lock on the same
+relation at the same time, or one process can acquire an AccessShareLock while
+the other already holds AccessExclusiveLock. This might seem dangerous and
could be in some cases (more on that below), but if we didn't do this then
parallel query would be extremely prone to self-deadlock. For example, a
parallel query against a relation on which the leader already had
@@ -638,23 +638,15 @@ the other is safe enough. Problems would occur if the leader initiated
parallelism from a point in the code at which it had some backend-private
state that made table access from another process unsafe, for example after
calling SetReindexProcessing and before calling ResetReindexProcessing,
-catastrophe could ensue, because the worker won't have that state.
-
-To allow parallel inserts and parallel copy, we have ensured that relation
-extension and page locks don't participate in group locking which means such
-locks can conflict among the same group members. This is required as it is no
-safer for two related processes to extend the same relation or perform clean up
-in gin indexes at a time than for unrelated processes to do the same. We don't
-acquire a heavyweight lock on any other object after relation extension lock
-which means such a lock can never participate in the deadlock cycle. After
-acquiring page locks, we can acquire relation extension lock but reverse never
-happens, so those will also not participate in deadlock. To allow for other
-parallel writes like parallel update or parallel delete, we'll either need to
-(1) further enhance the deadlock detector to handle those tuple locks in a
-different way than other types; or (2) have parallel workers use some other
-mutual exclusion method for such cases. Currently, the parallel mode is
-strictly read-only, but now we have the infrastructure to allow parallel
-inserts and parallel copy.
+catastrophe could ensue, because the worker won't have that state. Similarly,
+problems could occur with certain kinds of non-relation locks, such as
+GIN page locks. It's no safer for two related processes to perform GIN clean
+up at the same time than for unrelated processes to do the same.
+However, since parallel mode is strictly read-only at present, neither this
+nor most of the similar cases can arise at present. To allow parallel writes,
+we'll either need to (1) further enhance the deadlock detector to handle those
+types of locks in a different way than other types; or (2) have parallel
+workers use some other mutual exclusion method for such cases.
Group locking adds three new members to each PGPROC: lockGroupLeader,
lockGroupMembers, and lockGroupLink. A PGPROC's lockGroupLeader is NULL for
diff --git a/src/backend/storage/lmgr/deadlock.c b/src/backend/storage/lmgr/deadlock.c
index 617ebacdd4..2bdd20b1fe 100644
--- a/src/backend/storage/lmgr/deadlock.c
+++ b/src/backend/storage/lmgr/deadlock.c
@@ -546,12 +546,11 @@ FindLockCycleRecurseMember(PGPROC *checkProc,
lm;
/*
- * The relation extension or page lock can never participate in actual
- * deadlock cycle. See Asserts in LockAcquireExtended. So, there is no
- * advantage in checking wait edges from them.
+ * The relation extension lock can never participate in actual deadlock
+ * cycle. See Assert in LockAcquireExtended. So, there is no advantage
+ * in checking wait edges from it.
*/
- if (LOCK_LOCKTAG(*lock) == LOCKTAG_RELATION_EXTEND ||
- (LOCK_LOCKTAG(*lock) == LOCKTAG_PAGE))
+ if (LOCK_LOCKTAG(*lock) == LOCKTAG_RELATION_EXTEND)
return false;
lockMethodTable = GetLocksMethodTable(lock);
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index 0a692ee0a6..f595bce31b 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -186,18 +186,6 @@ static int FastPathLocalUseCount = 0;
*/
static bool IsRelationExtensionLockHeld PG_USED_FOR_ASSERTS_ONLY = false;
-/*
- * Flag to indicate if the page lock is held by this backend. We don't
- * acquire any other heavyweight lock while holding the page lock except for
- * relation extension. However, these locks are never taken in reverse order
- * which implies that page locks will also never participate in the deadlock
- * cycle.
- *
- * Similar to relation extension, page locks are also held for a short
- * duration, so imposing such a restriction won't hurt.
- */
-static bool IsPageLockHeld PG_USED_FOR_ASSERTS_ONLY = false;
-
/* Macros for manipulating proc->fpLockBits */
#define FAST_PATH_BITS_PER_SLOT 3
#define FAST_PATH_LOCKNUMBER_OFFSET 1
@@ -886,13 +874,6 @@ LockAcquireExtended(const LOCKTAG *locktag,
*/
Assert(!IsRelationExtensionLockHeld);
- /*
- * We don't acquire any other heavyweight lock while holding the page lock
- * except for relation extension.
- */
- Assert(!IsPageLockHeld ||
- (locktag->locktag_type == LOCKTAG_RELATION_EXTEND));
-
/*
* Prepare to emit a WAL record if acquisition of this lock needs to be
* replayed in a standby server.
@@ -1340,10 +1321,10 @@ SetupLockInTable(LockMethod lockMethodTable, PGPROC *proc,
}
/*
- * Check and set/reset the flag that we hold the relation extension/page lock.
+ * Check and set/reset the flag that we hold the relation extension lock.
*
* It is callers responsibility that this function is called after
- * acquiring/releasing the relation extension/page lock.
+ * acquiring/releasing the relation extension lock.
*
* Pass acquired as true if lock is acquired, false otherwise.
*/
@@ -1353,9 +1334,6 @@ CheckAndSetLockHeld(LOCALLOCK *locallock, bool acquired)
#ifdef USE_ASSERT_CHECKING
if (LOCALLOCK_LOCKTAG(*locallock) == LOCKTAG_RELATION_EXTEND)
IsRelationExtensionLockHeld = acquired;
- else if (LOCALLOCK_LOCKTAG(*locallock) == LOCKTAG_PAGE)
- IsPageLockHeld = acquired;
-
#endif
}
@@ -1480,11 +1458,9 @@ LockCheckConflicts(LockMethod lockMethodTable,
}
/*
- * The relation extension or page lock conflict even between the group
- * members.
+ * The relation extension lock conflict even between the group members.
*/
- if (LOCK_LOCKTAG(*lock) == LOCKTAG_RELATION_EXTEND ||
- (LOCK_LOCKTAG(*lock) == LOCKTAG_PAGE))
+ if (LOCK_LOCKTAG(*lock) == LOCKTAG_RELATION_EXTEND)
{
PROCLOCK_PRINT("LockCheckConflicts: conflicting (group)",
proclock);
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index dac921219f..5b663a2997 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -1021,12 +1021,12 @@ ProcSleep(LOCALLOCK *locallock, LockMethod lockMethodTable)
/*
* If group locking is in use, locks held by members of my locking group
* need to be included in myHeldLocks. This is not required for relation
- * extension or page locks which conflict among group members. However,
- * including them in myHeldLocks will give group members the priority to
- * get those locks as compared to other backends which are also trying to
- * acquire those locks. OTOH, we can avoid giving priority to group
- * members for that kind of locks, but there doesn't appear to be a clear
- * advantage of the same.
+ * extension lock which conflict among group members. However, including
+ * them in myHeldLocks will give group members the priority to get those
+ * locks as compared to other backends which are also trying to acquire
+ * those locks. OTOH, we can avoid giving priority to group members for
+ * that kind of locks, but there doesn't appear to be a clear advantage of
+ * the same.
*/
if (leader != NULL)
{
--
2.28.0.windows.1
On Monday, June 26, 2023 12:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Fri, Jun 23, 2023 at 10:07 PM Andres Freund <andres@anarazel.de> wrote:
On 2023-06-23 14:04:15 +0530, Amit Kapila wrote:
OTOH, if the above theory is wrong or people are not convinced, I am
okay with removing all the changes in commits 72e78d831a and
3ba59ccc89.I am not convinced. And even if I were, coming up with new
justifications in a released version, when the existing testing
clearly wasn't enough to find the current bug, doesn't strike me as wise.Fair enough. If we could have been convinced of this then we can keep the
required change only for HEAD. But anyway let's remove the work related to
both commits (72e78d831a and 3ba59ccc89) for now and then we can come
back to it when we parallelize writes. The attached patch removes the changes
added by both commits with slight tweaking in comments/readme based on
the recent state.
Thanks for the patch. I have confirmed that the patch to revert page lock
handling applies cleanly on all branches(13~HEAD) and the assert failure and
undetectable deadlock problem are fixed after applying the patch.
Best Regards,
Hou zj
On Wed, Jun 28, 2023 at 7:26 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
On Monday, June 26, 2023 12:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
Fair enough. If we could have been convinced of this then we can keep the
required change only for HEAD. But anyway let's remove the work related to
both commits (72e78d831a and 3ba59ccc89) for now and then we can come
back to it when we parallelize writes. The attached patch removes the changes
added by both commits with slight tweaking in comments/readme based on
the recent state.Thanks for the patch. I have confirmed that the patch to revert page lock
handling applies cleanly on all branches(13~HEAD) and the assert failure and
undetectable deadlock problem are fixed after applying the patch.
Thanks for the verification. Unless someone has any further comments
or suggestions, I'll push this next week sometime.
--
With Regards,
Amit Kapila.
On Fri, Jun 30, 2023 at 8:26 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Wed, Jun 28, 2023 at 7:26 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:Thanks for the verification. Unless someone has any further comments
or suggestions, I'll push this next week sometime.
Pushed but forgot to do indent which leads to BF failure[1]https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=koel&dt=2023-07-06%2005%3A19%3A03. I'll take
care of it.
[1]: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=koel&dt=2023-07-06%2005%3A19%3A03
--
With Regards,
Amit Kapila.