pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedMarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit
14+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2024-01-29 6a1ea02c49 b899e00e71 e43425f481 f120c08724 e74c916657 e6511fe649 Fix locking when fixing an incomplete split of a GIN internal page
* MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit
@ 2024-01-22 07:59 feichanghong <feichanghong@qq.com>
2024-01-22 09:42 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 1 reply; 14+ messages in thread
From: feichanghong @ 2024-01-22 07:59 UTC (permalink / raw)
To: pgsql-bugs <pgsql-bugs@lists.postgresql.org>
Hi,
Environmental information is as follows:
PostgreSQL Version: 17, branch: master, commit: abb0b4fc
Operating system: centos 7, Kernel version: 5.10.13
Client: psql
After the instance crash recovery, I encountered an assertion failure with
MarkBufferDirty when inserting data into a gin index. The specific stack trace
is as follows:
```
#0 0x00007feb10d19277 in raise () from /lib64/libc.so.6
#1 0x00007feb10d1a968 in abort () from /lib64/libc.so.6
#2 0x00000000009a4184 in ExceptionalCondition (
conditionName=conditionName@entry=0xb44e38
"LWLockHeldByMeInMode(BufferDescriptorGetContentLock(bufHdr), LW_EXCLUSIVE)",
fileName=fileName@entry=0xb45594 "bufmgr.c",
lineNumber=lineNumber@entry=2216) at assert.c:66
#3 0x0000000000840da0 in MarkBufferDirty (buffer=buffer@entry=865) at bufmgr.c:2216
#4 0x000000000052f953 in ginPlaceToPage (btree=0x7fff6a3316b0, stack=0x1e5cbd0,
insertdata=0x1e31ce8, updateblkno=626, childbuf=865, buildStats=0x0)
at ginbtree.c:407
#5 0x000000000053084b in ginFinishSplit (btree=0x7fff6a3316b0, stack=0x1e5bac8,
freestack=false, buildStats=0x0) at ginbtree.c:738
#6 0x000000000053103f in ginFindLeafPage (btree=btree@entry=0x7fff6a3316b0,
searchMode=searchMode@entry=false, rootConflictCheck=rootConflictCheck@entry=false)
at ginbtree.c:111
#7 0x000000000053b850 in ginEntryInsert (ginstate=ginstate@entry=0x1e1f708, attnum=1,
key=31597248, category=0 '\000', items=0x1e222d8, nitem=1, buildStats=0x0)
at gininsert.c:195
#8 0x00000000005373f2 in ginInsertCleanup (ginstate=ginstate@entry=0x1e1f708,
full_clean=full_clean@entry=false, fill_fsm=fill_fsm@entry=true,
forceCleanup=forceCleanup@entry=false, stats=stats@entry=0x0) at ginfast.c:930
#9 0x0000000000537f2a in ginHeapTupleFastInsert (ginstate=ginstate@entry=0x1e1f708,
collector=collector@entry=0x7fff6a331ab0) at ginfast.c:471
```
In the function ginFindLeafPage, if we encounter a page marked with
GIN_INCOMPLETE_SPLIT, we call ginFinishSplit to finish the incomplete split and
remove the GIN_INCOMPLETE_SPLIT flag from that page. ginFinishSplit requires
that "On entry, stack->buffer is exclusively locked," as explained in its
comments.
The function ginFindLeafPage determines the type of lock held by ginTraverseLock:
leaf pages hold GIN_EXCLUSIVE, and internal page hold GIN_SHARE. If ginFindLeafPage
traverses to an internal page with an incomplete split, it will only hold a
GIN_SHARE lock, which does not meet the requirements of ginFinishSplit. If a
crash occurs when an internal page is split, but no downlink is inserted in the
parent page, this problem may occur.
I injected the following code into the ginbtree.c file to trigger a crash
during the splitting of an internal page:
```
inline patch 1: src/backend/access/gin/ginbtree.c (+0/-0 lines) download inline-1.patch
--- a/src/backend/access/gin/ginbtree.c
+++ b/src/backend/access/gin/ginbtree.c
@@ -621,6 +621,12 @@ ginPlaceToPage(GinBtree btree, GinBtreeStack *stack,
v1-fix-ginFinishSplit-lock.patch (application/octet-stream, 1.2K) download
diff --git a/src/backend/access/gin/ginbtree.c b/src/backend/access/gin/ginbtree.c
index 39e2c07ad3..4f6fcd4266 100644
--- a/src/backend/access/gin/ginbtree.c
+++ b/src/backend/access/gin/ginbtree.c
@@ -108,7 +108,16 @@ ginFindLeafPage(GinBtree btree, bool searchMode,
* encounter on the way.
*/
if (!searchMode && GinPageIsIncompleteSplit(page))
- ginFinishSplit(btree, stack, false, NULL);
+ {
+ if (access == GIN_SHARE)
+ {
+ /* for internal page, we should relock our page */
+ LockBuffer(stack->buffer, GIN_UNLOCK);
+ LockBuffer(stack->buffer, GIN_EXCLUSIVE);
+ }
+ if GinPageIsIncompleteSplit(page)
+ ginFinishSplit(btree, stack, false, NULL);
+ }
/*
* ok, page is correctly locked, we should check to move right ..,
@@ -123,7 +132,10 @@ ginFindLeafPage(GinBtree btree, bool searchMode,
/* rightmost page */
break;
- stack->buffer = ginStepRight(stack->buffer, btree->index, access);
+ if (!searchMode)
+ stack->buffer = ginStepRight(stack->buffer, btree->index, GIN_EXCLUSIVE);
+ else
+ stack->buffer = ginStepRight(stack->buffer, btree->index, access);
stack->blkno = rightlink;
page = BufferGetPage(stack->buffer);
* Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit
2024-01-22 07:59 MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
@ 2024-01-22 09:42 ` Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 15:27 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
0 siblings, 1 reply; 14+ messages in thread
From: Heikki Linnakangas @ 2024-01-22 09:42 UTC (permalink / raw)
To: feichanghong <feichanghong@qq.com>; pgsql-bugs <pgsql-bugs@lists.postgresql.org>
On 22/01/2024 09:59, feichanghong wrote:
> In the function ginFindLeafPage, if we encounter a page marked with
> GIN_INCOMPLETE_SPLIT, we call ginFinishSplit to finish the incomplete
> split and
> remove the GIN_INCOMPLETE_SPLIT flag from that page. ginFinishSplit requires
> that "On entry, stack->buffer is exclusively locked," as explained in its
> comments.
>
> The function ginFindLeafPage determines the type of lock held by
> ginTraverseLock:
> leaf pages hold GIN_EXCLUSIVE, and internal page hold GIN_SHARE. If
> ginFindLeafPage
> traverses to an internal page with an incomplete split, it will only hold a
> GIN_SHARE lock, which does not meet the requirements of ginFinishSplit. If a
> crash occurs when an internal page is split, but no downlink is inserted
> in the
> parent page, this problem may occur.
>
> I injected the following code into the ginbtree.c file to trigger a crash
> during the splitting of an internal page:
> ```
> --- a/src/backend/access/gin/ginbtree.c
> +++ b/src/backend/access/gin/ginbtree.c
> @@ -621,6 +621,12 @@ ginPlaceToPage(GinBtree btree, GinBtreeStack *stack,
> PageSetLSN(BufferGetPage(lbuffer), recptr);
> if (BufferIsValid(childbuf))
> PageSetLSN(childpage, recptr);
> +
> + if (stack->parent != NULL && !GinPageIsLeaf(newlpage))
> + {
> + XLogFlush(recptr);
> + elog(PANIC, "internal page split for block: %u",
> stack->blkno);
> + }
> }
> END_CRIT_SECTION();
> ```
>
> With the modifications above, the problem can be reproduced with the
> following
> steps:
> ```
> alter system set autovacuum to off;
> alter system set gin_pending_list_limit to 64;
> select pg_reload_conf();
> create table t (a text);
> create extension btree_gin;
> create index on t USING gin (a);
> -- PANIC crash
> insert into t select generate_series(1, 100000);
>
> -- Assert fail
> insert into t select 1;
> ```
>
> I reviewed all places where ginFinishSplit is called, and only in two
> instances
> in ginFindLeafPage might it be possible to not hold an exclusive lock on
> stack->buffer.
>
> The patch I provided in the attachment has been verified to fix the issue
> mentioned above.
> However, it may not be perfect: passing GIN_EXCLUSIVE to ginStepRight might
> affect performance. Do we need to add a parameter to ginStepRight, and
> within
> the function ginStepRight, elevate the lock level for an incomplete
> split page?
>
> Looking forward to suggestions from developers!
Thanks, I'll look into this. The fix seems fine at a quick glance, but
I'll think about the performance aspect a bit more.
I'm thinking of adding tests for this using the new fault-injection
facility that Michael just merged in commit d86d20f0ba. For GIN, but
also B-tree which has a similar incomplete-split mechanism.
Another way to create a scenario with incomplete splits, which doesn't
involve any crashes or errors, would be to perform PITR to just between
the insert and the finish-split records. But the fault-injection seems
easier.
--
Heikki Linnakangas
Neon (https://neon.tech)
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit
2024-01-22 07:59 MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 09:42 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
@ 2024-01-22 15:27 ` feichanghong <feichanghong@qq.com>
2024-01-22 16:01 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 1 reply; 14+ messages in thread
From: feichanghong @ 2024-01-22 15:27 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>
Thank you for your attention.
> Thanks, I'll look into this. The fix seems fine at a quick glance, but I'll think about the performance aspect a bit more.
I apologize for the mistake in my patch: "if GinPageIsIncompleteSplit(page)" is
missing a parenthesis, it should be "if (GinPageIsIncompleteSplit(page))"
> Another way to create a scenario with incomplete splits, which doesn't involve any crashes or errors, would be to perform PITR to just between the insert and the finish-split records. But the fault-injection seems easier.
I agree it, fault-injection is easier than PITR.
Best Regards,
Fei Changhong
Alibaba Cloud Computing Ltd.
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit
2024-01-22 07:59 MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 09:42 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 15:27 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
@ 2024-01-22 16:01 ` Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 16:21 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
0 siblings, 1 reply; 14+ messages in thread
From: Heikki Linnakangas @ 2024-01-22 16:01 UTC (permalink / raw)
To: feichanghong <feichanghong@qq.com>; Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>
(Adding Michael for the fault-injection parts)
On 22/01/2024 17:27, feichanghong wrote:
> Thank you for your attention.
>
>> Thanks, I'll look into this. The fix seems fine at a quick glance, but
>> I'll think about the performance aspect a bit more.
From a performance point of view, this doesn't matter. Incomplete split
are extremely rare. For convenience, though, I added a new function
specifically for handling these "leftover" incomplete splits as opposed
to finishing a split that you just made, which performs the
lock-upgrade. See attached. I think that helps with readability, and
makes it less likely that we'll forget the lock-upgrade in the future if
the insertion code is refactored.
> I apologize for the mistake in my patch: "if
> GinPageIsIncompleteSplit(page)" is
> missing a parenthesis, it should be "if (GinPageIsIncompleteSplit(page))"
>
>> Another way to create a scenario with incomplete splits, which doesn't
>> involve any crashes or errors, would be to perform PITR to just
>> between the insert and the finish-split records. But the
>> fault-injection seems easier.
> I agree it, fault-injection is easier than PITR.
The attached patch contains a test case using the fault-injection facility.
Michael, it was a pleasure to write this test with the injection points,
compared to trying to set up PITR at just the right moment. Thank you!
Since this is the first test that uses it, I didn't have any precedence
to copy-paste; can you take a look and verify if this is how you
imagined the facility to be used?
--
Heikki Linnakangas
Neon (https://neon.tech)
Attachments:
v2-0001-Fix-locking-when-fixing-an-incomplete-split-of-a-.patch (text/x-patch, 19.1K) download
From afc70f11de89b54474176c0fac07ee00274ec28b Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Mon, 22 Jan 2024 17:37:55 +0200
Subject: [PATCH v2 1/1] Fix locking when fixing an incomplete split of a GIN
internal page
ginFinishSplit() expects the caller to hold an exclusive lock on the
buffer, but when finishing an earlier "leftover" incomplete split of
an internal page, the caller held a shared lock. That caused an
assertion failure in MarkBufferDirty(). Without assertions, it could
lead to corruption if two backends tried to complete the split at the
same time.
On master, add a test case using the new injection point facility.
Report and analysis by Fei Changhong. Backpatch the fix to all
supported versions.
Discussion: https://www.postgresql.org/message-id/tencent_A3CE810F59132D8E230475A5F0F7A08C8307@qq.com
---
src/backend/access/gin/ginbtree.c | 68 +++++--
src/test/modules/Makefile | 4 +-
src/test/modules/gin/Makefile | 16 ++
.../gin/expected/gin_incomplete_splits.out | 180 ++++++++++++++++++
src/test/modules/gin/meson.build | 16 ++
.../modules/gin/sql/gin_incomplete_splits.sql | 144 ++++++++++++++
src/test/modules/meson.build | 1 +
7 files changed, 411 insertions(+), 18 deletions(-)
create mode 100644 src/test/modules/gin/Makefile
create mode 100644 src/test/modules/gin/expected/gin_incomplete_splits.out
create mode 100644 src/test/modules/gin/meson.build
create mode 100644 src/test/modules/gin/sql/gin_incomplete_splits.sql
diff --git a/src/backend/access/gin/ginbtree.c b/src/backend/access/gin/ginbtree.c
index 39e2c07ad3d..37c8acaef51 100644
--- a/src/backend/access/gin/ginbtree.c
+++ b/src/backend/access/gin/ginbtree.c
@@ -19,6 +19,7 @@
#include "access/xloginsert.h"
#include "miscadmin.h"
#include "storage/predicate.h"
+#include "utils/injection_point.h"
#include "utils/memutils.h"
#include "utils/rel.h"
@@ -28,6 +29,8 @@ static bool ginPlaceToPage(GinBtree btree, GinBtreeStack *stack,
Buffer childbuf, GinStatsData *buildStats);
static void ginFinishSplit(GinBtree btree, GinBtreeStack *stack,
bool freestack, GinStatsData *buildStats);
+static void ginFinishOldSplit(GinBtree btree, GinBtreeStack *stack,
+ GinStatsData *buildStats, int access);
/*
* Lock buffer by needed method for search.
@@ -108,7 +111,7 @@ ginFindLeafPage(GinBtree btree, bool searchMode,
* encounter on the way.
*/
if (!searchMode && GinPageIsIncompleteSplit(page))
- ginFinishSplit(btree, stack, false, NULL);
+ ginFinishOldSplit(btree, stack, NULL, access);
/*
* ok, page is correctly locked, we should check to move right ..,
@@ -128,7 +131,7 @@ ginFindLeafPage(GinBtree btree, bool searchMode,
page = BufferGetPage(stack->buffer);
if (!searchMode && GinPageIsIncompleteSplit(page))
- ginFinishSplit(btree, stack, false, NULL);
+ ginFinishOldSplit(btree, stack, NULL, access);
}
if (GinPageIsLeaf(page)) /* we found, return locked page */
@@ -262,7 +265,7 @@ ginFindParents(GinBtree btree, GinBtreeStack *stack)
ptr->parent = root;
ptr->off = InvalidOffsetNumber;
- ginFinishSplit(btree, ptr, false, NULL);
+ ginFinishOldSplit(btree, ptr, NULL, GIN_EXCLUSIVE);
}
leftmostBlkno = btree->getLeftMostChild(btree, page);
@@ -291,7 +294,7 @@ ginFindParents(GinBtree btree, GinBtreeStack *stack)
ptr->parent = root;
ptr->off = InvalidOffsetNumber;
- ginFinishSplit(btree, ptr, false, NULL);
+ ginFinishOldSplit(btree, ptr, NULL, GIN_EXCLUSIVE);
}
}
@@ -670,15 +673,6 @@ ginFinishSplit(GinBtree btree, GinBtreeStack *stack, bool freestack,
bool done;
bool first = true;
- /*
- * freestack == false when we encounter an incompletely split page during
- * a scan, while freestack == true is used in the normal scenario that a
- * split is finished right after the initial insert.
- */
- if (!freestack)
- elog(DEBUG1, "finishing incomplete split of block %u in gin index \"%s\"",
- stack->blkno, RelationGetRelationName(btree->index));
-
/* this loop crawls up the stack until the insertion is complete */
do
{
@@ -686,6 +680,17 @@ ginFinishSplit(GinBtree btree, GinBtreeStack *stack, bool freestack,
void *insertdata;
BlockNumber updateblkno;
+#ifdef USE_INJECTION_POINTS
+ {
+ Page page = BufferGetPage(stack->buffer);
+
+ if (GinPageIsLeaf(page))
+ INJECTION_POINT("gin-leave-leaf-split-incomplete");
+ else
+ INJECTION_POINT("gin-leave-internal-split-incomplete");
+ }
+#endif
+
/* search parent to lock */
LockBuffer(parent->buffer, GIN_EXCLUSIVE);
@@ -699,7 +704,7 @@ ginFinishSplit(GinBtree btree, GinBtreeStack *stack, bool freestack,
* would fail.
*/
if (GinPageIsIncompleteSplit(BufferGetPage(parent->buffer)))
- ginFinishSplit(btree, parent, false, buildStats);
+ ginFinishOldSplit(btree, parent, buildStats, GIN_EXCLUSIVE);
/* move right if it's needed */
page = BufferGetPage(parent->buffer);
@@ -723,7 +728,7 @@ ginFinishSplit(GinBtree btree, GinBtreeStack *stack, bool freestack,
page = BufferGetPage(parent->buffer);
if (GinPageIsIncompleteSplit(BufferGetPage(parent->buffer)))
- ginFinishSplit(btree, parent, false, buildStats);
+ ginFinishOldSplit(btree, parent, buildStats, GIN_EXCLUSIVE);
}
/* insert the downlink */
@@ -759,6 +764,37 @@ ginFinishSplit(GinBtree btree, GinBtreeStack *stack, bool freestack,
freeGinBtreeStack(stack);
}
+/*
+ * An entry point to ginFinishSplit() that is used we stumble upon an existing
+ * incompletely split page in the tree, as opposed to completing a split that
+ * we just made outselves. The difference is that stack->buffer may be merely
+ * share-locked on entry, and will be upgraded to exclusive mode.
+ */
+static void
+ginFinishOldSplit(GinBtree btree, GinBtreeStack *stack, GinStatsData *buildStats, int access)
+{
+ INJECTION_POINT("gin-finish-incomplete-split");
+ elog(DEBUG1, "finishing incomplete split of block %u in gin index \"%s\"",
+ stack->blkno, RelationGetRelationName(btree->index));
+
+ if (access == GIN_SHARE)
+ {
+ LockBuffer(stack->buffer, GIN_UNLOCK);
+ LockBuffer(stack->buffer, GIN_EXCLUSIVE);
+
+ if (!GinPageIsIncompleteSplit(BufferGetPage(stack->buffer)))
+ {
+ /*
+ * Someone else already completed the split while we were not
+ * holding the lock.
+ */
+ return;
+ }
+ }
+
+ ginFinishSplit(btree, stack, false, buildStats);
+}
+
/*
* Insert a value to tree described by stack.
*
@@ -779,7 +815,7 @@ ginInsertValue(GinBtree btree, GinBtreeStack *stack, void *insertdata,
/* If the leaf page was incompletely split, finish the split first */
if (GinPageIsIncompleteSplit(BufferGetPage(stack->buffer)))
- ginFinishSplit(btree, stack, false, buildStats);
+ ginFinishOldSplit(btree, stack, buildStats, GIN_EXCLUSIVE);
done = ginPlaceToPage(btree, stack,
insertdata, InvalidBlockNumber,
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index e32c8925f67..89aa41b5e31 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -40,9 +40,9 @@ SUBDIRS = \
ifeq ($(enable_injection_points),yes)
-SUBDIRS += injection_points
+SUBDIRS += injection_points gin
else
-ALWAYS_SUBDIRS += injection_points
+ALWAYS_SUBDIRS += injection_points gin
endif
ifeq ($(with_ssl),openssl)
diff --git a/src/test/modules/gin/Makefile b/src/test/modules/gin/Makefile
new file mode 100644
index 00000000000..a7731b7fa18
--- /dev/null
+++ b/src/test/modules/gin/Makefile
@@ -0,0 +1,16 @@
+# src/test/modules/gin/Makefile
+
+EXTRA_INSTALL = src/test/modules/injection_points
+
+REGRESS = gin_incomplete_splits
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/gin
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/gin/expected/gin_incomplete_splits.out b/src/test/modules/gin/expected/gin_incomplete_splits.out
new file mode 100644
index 00000000000..9a60f8c128a
--- /dev/null
+++ b/src/test/modules/gin/expected/gin_incomplete_splits.out
@@ -0,0 +1,180 @@
+--
+-- The test uses a GIN index over int[]. The table contains arrays
+-- with integers from 1 to :next_i. Each integer occurs exactly once,
+-- no gaps or duplicates, although the index does contain some
+-- duplicate elements because some of the inserting transactions are
+-- rolled back during the test. The exact contents of the table depend
+-- on the physical layout of the index, which in turn depends at least
+-- on the block size, so instead of check for the exact contents, we
+-- check those invariants. :next_i psql variable is maintained at all
+-- times to hold the last inserted integer + 1.
+--
+-- This uses injection points to cause errors that leave some page
+-- splits in "incomplete" state
+create extension injection_points;
+-- Use the index for all the queries
+set enable_seqscan=off;
+-- Print a NOTICE whenever an incomplete split gets fixed
+SELECT injection_points_attach('gin-finish-incomplete-split', 'notice');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+--
+-- First create the test table and some helper functions
+--
+create table gin_incomplete_splits(i int4[]) with (autovacuum_enabled = off);
+create index on gin_incomplete_splits using gin (i) with (fastupdate = off);
+-- Creates an array with all integers from $1 (inclusive) $2 (exclusive)
+create function range_array(int, int) returns int[] language sql immutable as $$
+ select array_agg(g) from generate_series($1, $2 - 1) g
+$$;
+-- Inserts an array with 'n' rows to the test table. Pass :next_i as
+-- the first argument, returns the new value for :next_i.
+create function insert_n(first_i int, n int) returns int language plpgsql as $$
+begin
+ insert into gin_incomplete_splits values (range_array(first_i, first_i+n));
+ return first_i + n;
+end;
+$$;
+-- Inserts to the table until an insert fails. Like insert_n(), returns the
+-- new value for :next_i.
+create function insert_until_fail(next_i int, step int default 1) returns int language plpgsql as $$
+declare
+ i integer;
+begin
+ -- Insert arrays with 'step' elements each, until an error occurs.
+ loop
+ begin
+ select insert_n(next_i, step) into next_i;
+ exception when others then
+ raise notice 'failed with: %', sqlerrm;
+ exit;
+ end;
+
+ -- The caller is expected to set an injection point that eventuall
+ -- causes an error. But bail out if still no error after 10000
+ -- attempts, so that we don't get stuck in an infinite loop.
+ i := i + 1;
+ if i = 10000 then
+ raise 'no error on inserts after ';
+ end if;
+ end loop;
+
+ return next_i;
+end;
+$$;
+-- Check the invariants.
+create function verify(next_i int) returns bool language plpgsql as $$
+declare
+ a integer[];
+ elem integer;
+ c integer;
+begin
+ -- Perform a scan over the trailing part of the index, where the
+ -- possible incomplete splits are. (We don't check the whole table,
+ -- because that'd be pretty slow.)
+ c := 0;
+ -- Find all arrays that overlap with the last 200 inserted integers. Or
+ -- the next 100, which shouldn't exist.
+ for a in select i from gin_incomplete_splits where i && range_array(next_i - 200, next_i + 100)
+ loop
+ -- count all elements in those arrays in the window.
+ foreach elem in ARRAY a loop
+ if elem >= next_i then
+ raise 'unexpected value % in array', elem;
+ end if;
+ if elem >= next_i - 200 then
+ c := c + 1;
+ end if;
+ end loop;
+ end loop;
+ if c <> 200 then
+ raise 'unexpected count % ', c;
+ end if;
+
+ return true;
+end;
+$$;
+-- Insert one array to get started.
+select insert_n(1, 1000) as next_i
+\gset
+select verify(:next_i);
+ verify
+--------
+ t
+(1 row)
+
+--
+-- Test incomplete leaf split
+--
+SELECT injection_points_attach('gin-leave-leaf-split-incomplete', 'error');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+select insert_until_fail(:next_i) as next_i
+\gset
+NOTICE: failed with: error triggered for injection point gin-leave-leaf-split-incomplete
+SELECT injection_points_detach('gin-leave-leaf-split-incomplete');
+ injection_points_detach
+-------------------------
+
+(1 row)
+
+-- Verify that a scan works even though there's an incomplete split
+select verify(:next_i);
+ verify
+--------
+ t
+(1 row)
+
+-- Insert some more rows, finishing the split
+select insert_n(:next_i, 10) as next_i
+\gset
+NOTICE: notice triggered for injection point gin-finish-incomplete-split
+-- Verify that a scan still works
+select verify(:next_i);
+ verify
+--------
+ t
+(1 row)
+
+--
+-- Test incomplete internal page split
+--
+SELECT injection_points_attach('gin-leave-internal-split-incomplete', 'error');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+select insert_until_fail(:next_i, 100) as next_i
+\gset
+NOTICE: failed with: error triggered for injection point gin-leave-internal-split-incomplete
+SELECT injection_points_detach('gin-leave-internal-split-incomplete');
+ injection_points_detach
+-------------------------
+
+(1 row)
+
+ -- Verify that a scan works even though there's an incomplete split
+select verify(:next_i);
+ verify
+--------
+ t
+(1 row)
+
+-- Insert some more rows, finishing the split
+select insert_n(:next_i, 10) as next_i
+\gset
+NOTICE: notice triggered for injection point gin-finish-incomplete-split
+-- Verify that a scan still works
+select verify(:next_i);
+ verify
+--------
+ t
+(1 row)
+
diff --git a/src/test/modules/gin/meson.build b/src/test/modules/gin/meson.build
new file mode 100644
index 00000000000..9734b51de2d
--- /dev/null
+++ b/src/test/modules/gin/meson.build
@@ -0,0 +1,16 @@
+# Copyright (c) 2022-2024, PostgreSQL Global Development Group
+
+if not get_option('injection_points')
+ subdir_done()
+endif
+
+tests += {
+ 'name': 'gin',
+ 'sd': meson.current_source_dir(),
+ 'bd': meson.current_build_dir(),
+ 'regress': {
+ 'sql': [
+ 'gin_incomplete_splits',
+ ],
+ },
+}
diff --git a/src/test/modules/gin/sql/gin_incomplete_splits.sql b/src/test/modules/gin/sql/gin_incomplete_splits.sql
new file mode 100644
index 00000000000..4e180b25195
--- /dev/null
+++ b/src/test/modules/gin/sql/gin_incomplete_splits.sql
@@ -0,0 +1,144 @@
+--
+-- The test uses a GIN index over int[]. The table contains arrays
+-- with integers from 1 to :next_i. Each integer occurs exactly once,
+-- no gaps or duplicates, although the index does contain some
+-- duplicate elements because some of the inserting transactions are
+-- rolled back during the test. The exact contents of the table depend
+-- on the physical layout of the index, which in turn depends at least
+-- on the block size, so instead of check for the exact contents, we
+-- check those invariants. :next_i psql variable is maintained at all
+-- times to hold the last inserted integer + 1.
+--
+
+-- This uses injection points to cause errors that leave some page
+-- splits in "incomplete" state
+create extension injection_points;
+
+-- Use the index for all the queries
+set enable_seqscan=off;
+
+-- Print a NOTICE whenever an incomplete split gets fixed
+SELECT injection_points_attach('gin-finish-incomplete-split', 'notice');
+
+--
+-- First create the test table and some helper functions
+--
+create table gin_incomplete_splits(i int4[]) with (autovacuum_enabled = off);
+
+create index on gin_incomplete_splits using gin (i) with (fastupdate = off);
+
+-- Creates an array with all integers from $1 (inclusive) $2 (exclusive)
+create function range_array(int, int) returns int[] language sql immutable as $$
+ select array_agg(g) from generate_series($1, $2 - 1) g
+$$;
+
+-- Inserts an array with 'n' rows to the test table. Pass :next_i as
+-- the first argument, returns the new value for :next_i.
+create function insert_n(first_i int, n int) returns int language plpgsql as $$
+begin
+ insert into gin_incomplete_splits values (range_array(first_i, first_i+n));
+ return first_i + n;
+end;
+$$;
+
+-- Inserts to the table until an insert fails. Like insert_n(), returns the
+-- new value for :next_i.
+create function insert_until_fail(next_i int, step int default 1) returns int language plpgsql as $$
+declare
+ i integer;
+begin
+ -- Insert arrays with 'step' elements each, until an error occurs.
+ loop
+ begin
+ select insert_n(next_i, step) into next_i;
+ exception when others then
+ raise notice 'failed with: %', sqlerrm;
+ exit;
+ end;
+
+ -- The caller is expected to set an injection point that eventuall
+ -- causes an error. But bail out if still no error after 10000
+ -- attempts, so that we don't get stuck in an infinite loop.
+ i := i + 1;
+ if i = 10000 then
+ raise 'no error on inserts after ';
+ end if;
+ end loop;
+
+ return next_i;
+end;
+$$;
+
+-- Check the invariants.
+create function verify(next_i int) returns bool language plpgsql as $$
+declare
+ a integer[];
+ elem integer;
+ c integer;
+begin
+ -- Perform a scan over the trailing part of the index, where the
+ -- possible incomplete splits are. (We don't check the whole table,
+ -- because that'd be pretty slow.)
+ c := 0;
+ -- Find all arrays that overlap with the last 200 inserted integers. Or
+ -- the next 100, which shouldn't exist.
+ for a in select i from gin_incomplete_splits where i && range_array(next_i - 200, next_i + 100)
+ loop
+ -- count all elements in those arrays in the window.
+ foreach elem in ARRAY a loop
+ if elem >= next_i then
+ raise 'unexpected value % in array', elem;
+ end if;
+ if elem >= next_i - 200 then
+ c := c + 1;
+ end if;
+ end loop;
+ end loop;
+ if c <> 200 then
+ raise 'unexpected count % ', c;
+ end if;
+
+ return true;
+end;
+$$;
+
+-- Insert one array to get started.
+select insert_n(1, 1000) as next_i
+\gset
+select verify(:next_i);
+
+
+--
+-- Test incomplete leaf split
+--
+SELECT injection_points_attach('gin-leave-leaf-split-incomplete', 'error');
+select insert_until_fail(:next_i) as next_i
+\gset
+SELECT injection_points_detach('gin-leave-leaf-split-incomplete');
+
+-- Verify that a scan works even though there's an incomplete split
+select verify(:next_i);
+
+-- Insert some more rows, finishing the split
+select insert_n(:next_i, 10) as next_i
+\gset
+-- Verify that a scan still works
+select verify(:next_i);
+
+
+--
+-- Test incomplete internal page split
+--
+SELECT injection_points_attach('gin-leave-internal-split-incomplete', 'error');
+select insert_until_fail(:next_i, 100) as next_i
+\gset
+SELECT injection_points_detach('gin-leave-internal-split-incomplete');
+
+ -- Verify that a scan works even though there's an incomplete split
+select verify(:next_i);
+
+-- Insert some more rows, finishing the split
+select insert_n(:next_i, 10) as next_i
+\gset
+-- Verify that a scan still works
+select verify(:next_i);
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
index 397e0906e6f..8fbe742d385 100644
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -5,6 +5,7 @@ subdir('commit_ts')
subdir('delay_execution')
subdir('dummy_index_am')
subdir('dummy_seclabel')
+subdir('gin')
subdir('injection_points')
subdir('ldap_password_func')
subdir('libpq_pipeline')
--
2.39.2
* Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit
2024-01-22 07:59 MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 09:42 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 15:27 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 16:01 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
@ 2024-01-22 16:21 ` feichanghong <feichanghong@qq.com>
2024-01-22 20:47 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
0 siblings, 1 reply; 14+ messages in thread
From: feichanghong @ 2024-01-22 16:21 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-bugs <pgsql-bugs@lists.postgresql.org>
> From a performance point of view, this doesn't matter. Incomplete split are extremely rare. For convenience, though, I added a new function specifically for handling these "leftover" incomplete splits as opposed to finishing a split that you just made, which performs the lock-upgrade. See attached. I think that helps with readability, and makes it less likely that we'll forget the lock-upgrade in the future if the insertion code is refactored.
I think that the lock-upgrade in the ginFinishOldSplit function is unsafe
because it violates the requirement of the ginStepRight function that
"The next page is locked first, before releasing the current page.”
Best Regards,
Fei Changhong
Alibaba Cloud Computing Ltd.
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit
2024-01-22 07:59 MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 09:42 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 15:27 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 16:01 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 16:21 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
@ 2024-01-22 20:47 ` Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 23:38 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Michael Paquier <michael@paquier.xyz>
2024-01-23 03:39 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
0 siblings, 2 replies; 14+ messages in thread
From: Heikki Linnakangas @ 2024-01-22 20:47 UTC (permalink / raw)
To: feichanghong <feichanghong@qq.com>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-bugs <pgsql-bugs@lists.postgresql.org>
On 22/01/2024 18:21, feichanghong wrote:
>
>> From a performance point of view, this doesn't matter. Incomplete
>> split are extremely rare. For convenience, though, I added a new
>> function specifically for handling these "leftover" incomplete splits
>> as opposed to finishing a split that you just made, which performs the
>> lock-upgrade. See attached. I think that helps with readability, and
>> makes it less likely that we'll forget the lock-upgrade in the future
>> if the insertion code is refactored.
> I think that the lock-upgrade in the ginFinishOldSplit function is unsafe
> because it violates the requirement of the ginStepRight function that
> "The next page is locked first, before releasing the current page.”
Good point.
I started to work on a more invasive patch that would move the
ginFinishOldSplit() calls to ginTraverseLock() and ginStepRight(), doing
the interlocking correctly. That makes life easier for the callers; they
don't need to deal with incomplete-splits anymore.
But then I read the Page deletion section in the README and understood
that my earlier patch is safe, after all. The lock-coupling in
ginStepRight() is only needed for searches, not for inserts. There is
another mechanism that prevents concurrent page deletions during an
insert: VACUUM holds a cleanup-lock on the root page.
Does that make sense, or am I missing something? Here's the same patch
as before, with some extra comments to explain why it's safe.
--
Heikki Linnakangas
Neon (https://neon.tech)
Attachments:
v3-0001-Fix-locking-when-fixing-an-incomplete-split-of-a-.patch (text/x-patch, 20.2K) download
From 002f9522232f0868a611ec4d707f9e2f3bb2cdd5 Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Mon, 22 Jan 2024 22:45:28 +0200
Subject: [PATCH v3 1/1] Fix locking when fixing an incomplete split of a GIN
internal page
ginFinishSplit() expects the caller to hold an exclusive lock on the
buffer, but when finishing an earlier "leftover" incomplete split of
an internal page, the caller held a shared lock. That caused an
assertion failure in MarkBufferDirty(). Without assertions, it could
lead to corruption if two backends tried to complete the split at the
same time.
On master, add a test case using the new injection point facility.
Report and analysis by Fei Changhong. Backpatch the fix to all
supported versions.
Discussion: https://www.postgresql.org/message-id/tencent_A3CE810F59132D8E230475A5F0F7A08C8307@qq.com
---
src/backend/access/gin/ginbtree.c | 77 ++++++--
src/test/modules/Makefile | 4 +-
src/test/modules/gin/Makefile | 16 ++
.../gin/expected/gin_incomplete_splits.out | 180 ++++++++++++++++++
src/test/modules/gin/meson.build | 16 ++
.../modules/gin/sql/gin_incomplete_splits.sql | 144 ++++++++++++++
src/test/modules/meson.build | 1 +
7 files changed, 418 insertions(+), 20 deletions(-)
create mode 100644 src/test/modules/gin/Makefile
create mode 100644 src/test/modules/gin/expected/gin_incomplete_splits.out
create mode 100644 src/test/modules/gin/meson.build
create mode 100644 src/test/modules/gin/sql/gin_incomplete_splits.sql
diff --git a/src/backend/access/gin/ginbtree.c b/src/backend/access/gin/ginbtree.c
index 39e2c07ad3d..86f938686c3 100644
--- a/src/backend/access/gin/ginbtree.c
+++ b/src/backend/access/gin/ginbtree.c
@@ -19,6 +19,7 @@
#include "access/xloginsert.h"
#include "miscadmin.h"
#include "storage/predicate.h"
+#include "utils/injection_point.h"
#include "utils/memutils.h"
#include "utils/rel.h"
@@ -28,6 +29,8 @@ static bool ginPlaceToPage(GinBtree btree, GinBtreeStack *stack,
Buffer childbuf, GinStatsData *buildStats);
static void ginFinishSplit(GinBtree btree, GinBtreeStack *stack,
bool freestack, GinStatsData *buildStats);
+static void ginFinishOldSplit(GinBtree btree, GinBtreeStack *stack,
+ GinStatsData *buildStats, int access);
/*
* Lock buffer by needed method for search.
@@ -108,7 +111,7 @@ ginFindLeafPage(GinBtree btree, bool searchMode,
* encounter on the way.
*/
if (!searchMode && GinPageIsIncompleteSplit(page))
- ginFinishSplit(btree, stack, false, NULL);
+ ginFinishOldSplit(btree, stack, NULL, access);
/*
* ok, page is correctly locked, we should check to move right ..,
@@ -128,7 +131,7 @@ ginFindLeafPage(GinBtree btree, bool searchMode,
page = BufferGetPage(stack->buffer);
if (!searchMode && GinPageIsIncompleteSplit(page))
- ginFinishSplit(btree, stack, false, NULL);
+ ginFinishOldSplit(btree, stack, NULL, access);
}
if (GinPageIsLeaf(page)) /* we found, return locked page */
@@ -164,8 +167,11 @@ ginFindLeafPage(GinBtree btree, bool searchMode,
* Step right from current page.
*
* The next page is locked first, before releasing the current page. This is
- * crucial to protect from concurrent page deletion (see comment in
- * ginDeletePage).
+ * crucial to prevent concurrent VACUUM from deleting a page that we are about
+ * to step to. (The lock-coupling isn't strictly necessary when we are
+ * traversing the tree to find an insert location, because page deletion grabs
+ * a cleanup lock on the root to prevent any concurrent inserts. See Page
+ * deletion section in the README. But there's no harm in doing it always.)
*/
Buffer
ginStepRight(Buffer buffer, Relation index, int lockmode)
@@ -262,7 +268,7 @@ ginFindParents(GinBtree btree, GinBtreeStack *stack)
ptr->parent = root;
ptr->off = InvalidOffsetNumber;
- ginFinishSplit(btree, ptr, false, NULL);
+ ginFinishOldSplit(btree, ptr, NULL, GIN_EXCLUSIVE);
}
leftmostBlkno = btree->getLeftMostChild(btree, page);
@@ -291,7 +297,7 @@ ginFindParents(GinBtree btree, GinBtreeStack *stack)
ptr->parent = root;
ptr->off = InvalidOffsetNumber;
- ginFinishSplit(btree, ptr, false, NULL);
+ ginFinishOldSplit(btree, ptr, NULL, GIN_EXCLUSIVE);
}
}
@@ -670,15 +676,6 @@ ginFinishSplit(GinBtree btree, GinBtreeStack *stack, bool freestack,
bool done;
bool first = true;
- /*
- * freestack == false when we encounter an incompletely split page during
- * a scan, while freestack == true is used in the normal scenario that a
- * split is finished right after the initial insert.
- */
- if (!freestack)
- elog(DEBUG1, "finishing incomplete split of block %u in gin index \"%s\"",
- stack->blkno, RelationGetRelationName(btree->index));
-
/* this loop crawls up the stack until the insertion is complete */
do
{
@@ -686,6 +683,13 @@ ginFinishSplit(GinBtree btree, GinBtreeStack *stack, bool freestack,
void *insertdata;
BlockNumber updateblkno;
+#ifdef USE_INJECTION_POINTS
+ if (GinPageIsLeaf(BufferGetPage(stack->buffer)))
+ INJECTION_POINT("gin-leave-leaf-split-incomplete");
+ else
+ INJECTION_POINT("gin-leave-internal-split-incomplete");
+#endif
+
/* search parent to lock */
LockBuffer(parent->buffer, GIN_EXCLUSIVE);
@@ -699,7 +703,7 @@ ginFinishSplit(GinBtree btree, GinBtreeStack *stack, bool freestack,
* would fail.
*/
if (GinPageIsIncompleteSplit(BufferGetPage(parent->buffer)))
- ginFinishSplit(btree, parent, false, buildStats);
+ ginFinishOldSplit(btree, parent, buildStats, GIN_EXCLUSIVE);
/* move right if it's needed */
page = BufferGetPage(parent->buffer);
@@ -723,7 +727,7 @@ ginFinishSplit(GinBtree btree, GinBtreeStack *stack, bool freestack,
page = BufferGetPage(parent->buffer);
if (GinPageIsIncompleteSplit(BufferGetPage(parent->buffer)))
- ginFinishSplit(btree, parent, false, buildStats);
+ ginFinishOldSplit(btree, parent, buildStats, GIN_EXCLUSIVE);
}
/* insert the downlink */
@@ -759,6 +763,43 @@ ginFinishSplit(GinBtree btree, GinBtreeStack *stack, bool freestack,
freeGinBtreeStack(stack);
}
+/*
+ * An entry point to ginFinishSplit() that is used when we stumble upon an
+ * existing incompletely split page in the tree, as opposed to completing a
+ * split that we just made outselves. The difference is that stack->buffer may
+ * be merely share-locked on entry, and will be upgraded to exclusive mode.
+ *
+ * Note: Upgrading the lock momentarily releases it. Doing that in a scan
+ * would not be OK, because a concurrent VACUUM might delete the page while
+ * we're not holding the lock. It's OK in an insert, though, because VACUUM
+ * has a different mechanism that prevents it from running concurrently with
+ * inserts. (Namely, it holds a cleanup lock on the root.)
+ */
+static void
+ginFinishOldSplit(GinBtree btree, GinBtreeStack *stack, GinStatsData *buildStats, int access)
+{
+ INJECTION_POINT("gin-finish-incomplete-split");
+ elog(DEBUG1, "finishing incomplete split of block %u in gin index \"%s\"",
+ stack->blkno, RelationGetRelationName(btree->index));
+
+ if (access == GIN_SHARE)
+ {
+ LockBuffer(stack->buffer, GIN_UNLOCK);
+ LockBuffer(stack->buffer, GIN_EXCLUSIVE);
+
+ if (!GinPageIsIncompleteSplit(BufferGetPage(stack->buffer)))
+ {
+ /*
+ * Someone else already completed the split while we were not
+ * holding the lock.
+ */
+ return;
+ }
+ }
+
+ ginFinishSplit(btree, stack, false, buildStats);
+}
+
/*
* Insert a value to tree described by stack.
*
@@ -779,7 +820,7 @@ ginInsertValue(GinBtree btree, GinBtreeStack *stack, void *insertdata,
/* If the leaf page was incompletely split, finish the split first */
if (GinPageIsIncompleteSplit(BufferGetPage(stack->buffer)))
- ginFinishSplit(btree, stack, false, buildStats);
+ ginFinishOldSplit(btree, stack, buildStats, GIN_EXCLUSIVE);
done = ginPlaceToPage(btree, stack,
insertdata, InvalidBlockNumber,
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index e32c8925f67..89aa41b5e31 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -40,9 +40,9 @@ SUBDIRS = \
ifeq ($(enable_injection_points),yes)
-SUBDIRS += injection_points
+SUBDIRS += injection_points gin
else
-ALWAYS_SUBDIRS += injection_points
+ALWAYS_SUBDIRS += injection_points gin
endif
ifeq ($(with_ssl),openssl)
diff --git a/src/test/modules/gin/Makefile b/src/test/modules/gin/Makefile
new file mode 100644
index 00000000000..a7731b7fa18
--- /dev/null
+++ b/src/test/modules/gin/Makefile
@@ -0,0 +1,16 @@
+# src/test/modules/gin/Makefile
+
+EXTRA_INSTALL = src/test/modules/injection_points
+
+REGRESS = gin_incomplete_splits
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/gin
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/gin/expected/gin_incomplete_splits.out b/src/test/modules/gin/expected/gin_incomplete_splits.out
new file mode 100644
index 00000000000..9a60f8c128a
--- /dev/null
+++ b/src/test/modules/gin/expected/gin_incomplete_splits.out
@@ -0,0 +1,180 @@
+--
+-- The test uses a GIN index over int[]. The table contains arrays
+-- with integers from 1 to :next_i. Each integer occurs exactly once,
+-- no gaps or duplicates, although the index does contain some
+-- duplicate elements because some of the inserting transactions are
+-- rolled back during the test. The exact contents of the table depend
+-- on the physical layout of the index, which in turn depends at least
+-- on the block size, so instead of check for the exact contents, we
+-- check those invariants. :next_i psql variable is maintained at all
+-- times to hold the last inserted integer + 1.
+--
+-- This uses injection points to cause errors that leave some page
+-- splits in "incomplete" state
+create extension injection_points;
+-- Use the index for all the queries
+set enable_seqscan=off;
+-- Print a NOTICE whenever an incomplete split gets fixed
+SELECT injection_points_attach('gin-finish-incomplete-split', 'notice');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+--
+-- First create the test table and some helper functions
+--
+create table gin_incomplete_splits(i int4[]) with (autovacuum_enabled = off);
+create index on gin_incomplete_splits using gin (i) with (fastupdate = off);
+-- Creates an array with all integers from $1 (inclusive) $2 (exclusive)
+create function range_array(int, int) returns int[] language sql immutable as $$
+ select array_agg(g) from generate_series($1, $2 - 1) g
+$$;
+-- Inserts an array with 'n' rows to the test table. Pass :next_i as
+-- the first argument, returns the new value for :next_i.
+create function insert_n(first_i int, n int) returns int language plpgsql as $$
+begin
+ insert into gin_incomplete_splits values (range_array(first_i, first_i+n));
+ return first_i + n;
+end;
+$$;
+-- Inserts to the table until an insert fails. Like insert_n(), returns the
+-- new value for :next_i.
+create function insert_until_fail(next_i int, step int default 1) returns int language plpgsql as $$
+declare
+ i integer;
+begin
+ -- Insert arrays with 'step' elements each, until an error occurs.
+ loop
+ begin
+ select insert_n(next_i, step) into next_i;
+ exception when others then
+ raise notice 'failed with: %', sqlerrm;
+ exit;
+ end;
+
+ -- The caller is expected to set an injection point that eventuall
+ -- causes an error. But bail out if still no error after 10000
+ -- attempts, so that we don't get stuck in an infinite loop.
+ i := i + 1;
+ if i = 10000 then
+ raise 'no error on inserts after ';
+ end if;
+ end loop;
+
+ return next_i;
+end;
+$$;
+-- Check the invariants.
+create function verify(next_i int) returns bool language plpgsql as $$
+declare
+ a integer[];
+ elem integer;
+ c integer;
+begin
+ -- Perform a scan over the trailing part of the index, where the
+ -- possible incomplete splits are. (We don't check the whole table,
+ -- because that'd be pretty slow.)
+ c := 0;
+ -- Find all arrays that overlap with the last 200 inserted integers. Or
+ -- the next 100, which shouldn't exist.
+ for a in select i from gin_incomplete_splits where i && range_array(next_i - 200, next_i + 100)
+ loop
+ -- count all elements in those arrays in the window.
+ foreach elem in ARRAY a loop
+ if elem >= next_i then
+ raise 'unexpected value % in array', elem;
+ end if;
+ if elem >= next_i - 200 then
+ c := c + 1;
+ end if;
+ end loop;
+ end loop;
+ if c <> 200 then
+ raise 'unexpected count % ', c;
+ end if;
+
+ return true;
+end;
+$$;
+-- Insert one array to get started.
+select insert_n(1, 1000) as next_i
+\gset
+select verify(:next_i);
+ verify
+--------
+ t
+(1 row)
+
+--
+-- Test incomplete leaf split
+--
+SELECT injection_points_attach('gin-leave-leaf-split-incomplete', 'error');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+select insert_until_fail(:next_i) as next_i
+\gset
+NOTICE: failed with: error triggered for injection point gin-leave-leaf-split-incomplete
+SELECT injection_points_detach('gin-leave-leaf-split-incomplete');
+ injection_points_detach
+-------------------------
+
+(1 row)
+
+-- Verify that a scan works even though there's an incomplete split
+select verify(:next_i);
+ verify
+--------
+ t
+(1 row)
+
+-- Insert some more rows, finishing the split
+select insert_n(:next_i, 10) as next_i
+\gset
+NOTICE: notice triggered for injection point gin-finish-incomplete-split
+-- Verify that a scan still works
+select verify(:next_i);
+ verify
+--------
+ t
+(1 row)
+
+--
+-- Test incomplete internal page split
+--
+SELECT injection_points_attach('gin-leave-internal-split-incomplete', 'error');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+select insert_until_fail(:next_i, 100) as next_i
+\gset
+NOTICE: failed with: error triggered for injection point gin-leave-internal-split-incomplete
+SELECT injection_points_detach('gin-leave-internal-split-incomplete');
+ injection_points_detach
+-------------------------
+
+(1 row)
+
+ -- Verify that a scan works even though there's an incomplete split
+select verify(:next_i);
+ verify
+--------
+ t
+(1 row)
+
+-- Insert some more rows, finishing the split
+select insert_n(:next_i, 10) as next_i
+\gset
+NOTICE: notice triggered for injection point gin-finish-incomplete-split
+-- Verify that a scan still works
+select verify(:next_i);
+ verify
+--------
+ t
+(1 row)
+
diff --git a/src/test/modules/gin/meson.build b/src/test/modules/gin/meson.build
new file mode 100644
index 00000000000..9734b51de2d
--- /dev/null
+++ b/src/test/modules/gin/meson.build
@@ -0,0 +1,16 @@
+# Copyright (c) 2022-2024, PostgreSQL Global Development Group
+
+if not get_option('injection_points')
+ subdir_done()
+endif
+
+tests += {
+ 'name': 'gin',
+ 'sd': meson.current_source_dir(),
+ 'bd': meson.current_build_dir(),
+ 'regress': {
+ 'sql': [
+ 'gin_incomplete_splits',
+ ],
+ },
+}
diff --git a/src/test/modules/gin/sql/gin_incomplete_splits.sql b/src/test/modules/gin/sql/gin_incomplete_splits.sql
new file mode 100644
index 00000000000..4e180b25195
--- /dev/null
+++ b/src/test/modules/gin/sql/gin_incomplete_splits.sql
@@ -0,0 +1,144 @@
+--
+-- The test uses a GIN index over int[]. The table contains arrays
+-- with integers from 1 to :next_i. Each integer occurs exactly once,
+-- no gaps or duplicates, although the index does contain some
+-- duplicate elements because some of the inserting transactions are
+-- rolled back during the test. The exact contents of the table depend
+-- on the physical layout of the index, which in turn depends at least
+-- on the block size, so instead of check for the exact contents, we
+-- check those invariants. :next_i psql variable is maintained at all
+-- times to hold the last inserted integer + 1.
+--
+
+-- This uses injection points to cause errors that leave some page
+-- splits in "incomplete" state
+create extension injection_points;
+
+-- Use the index for all the queries
+set enable_seqscan=off;
+
+-- Print a NOTICE whenever an incomplete split gets fixed
+SELECT injection_points_attach('gin-finish-incomplete-split', 'notice');
+
+--
+-- First create the test table and some helper functions
+--
+create table gin_incomplete_splits(i int4[]) with (autovacuum_enabled = off);
+
+create index on gin_incomplete_splits using gin (i) with (fastupdate = off);
+
+-- Creates an array with all integers from $1 (inclusive) $2 (exclusive)
+create function range_array(int, int) returns int[] language sql immutable as $$
+ select array_agg(g) from generate_series($1, $2 - 1) g
+$$;
+
+-- Inserts an array with 'n' rows to the test table. Pass :next_i as
+-- the first argument, returns the new value for :next_i.
+create function insert_n(first_i int, n int) returns int language plpgsql as $$
+begin
+ insert into gin_incomplete_splits values (range_array(first_i, first_i+n));
+ return first_i + n;
+end;
+$$;
+
+-- Inserts to the table until an insert fails. Like insert_n(), returns the
+-- new value for :next_i.
+create function insert_until_fail(next_i int, step int default 1) returns int language plpgsql as $$
+declare
+ i integer;
+begin
+ -- Insert arrays with 'step' elements each, until an error occurs.
+ loop
+ begin
+ select insert_n(next_i, step) into next_i;
+ exception when others then
+ raise notice 'failed with: %', sqlerrm;
+ exit;
+ end;
+
+ -- The caller is expected to set an injection point that eventuall
+ -- causes an error. But bail out if still no error after 10000
+ -- attempts, so that we don't get stuck in an infinite loop.
+ i := i + 1;
+ if i = 10000 then
+ raise 'no error on inserts after ';
+ end if;
+ end loop;
+
+ return next_i;
+end;
+$$;
+
+-- Check the invariants.
+create function verify(next_i int) returns bool language plpgsql as $$
+declare
+ a integer[];
+ elem integer;
+ c integer;
+begin
+ -- Perform a scan over the trailing part of the index, where the
+ -- possible incomplete splits are. (We don't check the whole table,
+ -- because that'd be pretty slow.)
+ c := 0;
+ -- Find all arrays that overlap with the last 200 inserted integers. Or
+ -- the next 100, which shouldn't exist.
+ for a in select i from gin_incomplete_splits where i && range_array(next_i - 200, next_i + 100)
+ loop
+ -- count all elements in those arrays in the window.
+ foreach elem in ARRAY a loop
+ if elem >= next_i then
+ raise 'unexpected value % in array', elem;
+ end if;
+ if elem >= next_i - 200 then
+ c := c + 1;
+ end if;
+ end loop;
+ end loop;
+ if c <> 200 then
+ raise 'unexpected count % ', c;
+ end if;
+
+ return true;
+end;
+$$;
+
+-- Insert one array to get started.
+select insert_n(1, 1000) as next_i
+\gset
+select verify(:next_i);
+
+
+--
+-- Test incomplete leaf split
+--
+SELECT injection_points_attach('gin-leave-leaf-split-incomplete', 'error');
+select insert_until_fail(:next_i) as next_i
+\gset
+SELECT injection_points_detach('gin-leave-leaf-split-incomplete');
+
+-- Verify that a scan works even though there's an incomplete split
+select verify(:next_i);
+
+-- Insert some more rows, finishing the split
+select insert_n(:next_i, 10) as next_i
+\gset
+-- Verify that a scan still works
+select verify(:next_i);
+
+
+--
+-- Test incomplete internal page split
+--
+SELECT injection_points_attach('gin-leave-internal-split-incomplete', 'error');
+select insert_until_fail(:next_i, 100) as next_i
+\gset
+SELECT injection_points_detach('gin-leave-internal-split-incomplete');
+
+ -- Verify that a scan works even though there's an incomplete split
+select verify(:next_i);
+
+-- Insert some more rows, finishing the split
+select insert_n(:next_i, 10) as next_i
+\gset
+-- Verify that a scan still works
+select verify(:next_i);
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
index 397e0906e6f..8fbe742d385 100644
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -5,6 +5,7 @@ subdir('commit_ts')
subdir('delay_execution')
subdir('dummy_index_am')
subdir('dummy_seclabel')
+subdir('gin')
subdir('injection_points')
subdir('ldap_password_func')
subdir('libpq_pipeline')
--
2.39.2
* Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit
2024-01-22 07:59 MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 09:42 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 15:27 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 16:01 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 16:21 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 20:47 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
@ 2024-01-22 23:38 ` Michael Paquier <michael@paquier.xyz>
2024-01-23 01:15 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Michael Paquier <michael@paquier.xyz>
2024-01-23 07:12 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
1 sibling, 2 replies; 14+ messages in thread
From: Michael Paquier @ 2024-01-22 23:38 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: feichanghong <feichanghong@qq.com>; pgsql-bugs <pgsql-bugs@lists.postgresql.org>
On Mon, Jan 22, 2024 at 10:47:17PM +0200, Heikki Linnakangas wrote:
> Michael, it was a pleasure to write this test with the injection points,
> compared to trying to set up PITR at just the right moment. Thank you! Since
> this is the first test that uses it, I didn't have any precedence to
> copy-paste; can you take a look and verify if this is how you imagined the
> facility to be used?
I was wondering how many weeks it would take for somebody to begin
playing with that, and here you are 12 hours later..
+EXTRA_INSTALL = src/test/modules/injection_points
[...]
+#ifdef USE_INJECTION_POINTS
+ if (GinPageIsLeaf(BufferGetPage(stack->buffer)))
+ INJECTION_POINT("gin-leave-leaf-split-incomplete");
+ else
+ INJECTION_POINT("gin-leave-internal-split-incomplete");
+#endif
Yes, that would be one way of doing it. It makes the code a bit more
invasive but that would work. Now, you expect this point to be
conditional depending on the state of the buffer. One thing I was
thinking of was to extend the APIs in injection_point.c to be able to
handle a set of arguments as a set of (void*) so as callbacks could
internally decide what they want to do depending on the running state.
I didn't have a use for it, but well, you do, so perhaps we could just
bite the bullet and do it.
That reduces the footprint on the backend code, moving more logic
behind USE_INJECTION_POINTS in injection_point.c. At the end, you
should be able to patch the core backend with something like that,
without using some ifdefs on USE_INJECTION_POINTS:
INJECTION_POINT_1ARG("gin-leave-leaf-split", (void *) buffer)
And INJECTION_POINT_1ARG() would be just a wrapper around something
like:
extern void InjectionPointRun1(const char *name, void *arg1);
And then plug in the callback you want. Having the callback in a new
test module with the SQL test itself would be OK, or you could just
add it to src/test/modules/injection_points/ with a SQL test in it, or
just use EXTRA_INSTALL.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit
2024-01-22 07:59 MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 09:42 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 15:27 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 16:01 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 16:21 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 20:47 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 23:38 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Michael Paquier <michael@paquier.xyz>
@ 2024-01-23 01:15 ` Michael Paquier <michael@paquier.xyz>
1 sibling, 0 replies; 14+ messages in thread
From: Michael Paquier @ 2024-01-23 01:15 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: feichanghong <feichanghong@qq.com>; pgsql-bugs <pgsql-bugs@lists.postgresql.org>
On Tue, Jan 23, 2024 at 08:38:09AM +0900, Michael Paquier wrote:
> And INJECTION_POINT_1ARG() would be just a wrapper around something
> like:
> extern void InjectionPointRun1(const char *name, void *arg1);
>
> And then plug in the callback you want. Having the callback in a new
> test module with the SQL test itself would be OK, or you could just
> add it to src/test/modules/injection_points/ with a SQL test in it, or
> just use EXTRA_INSTALL.
That would mean to extend the code like the attached, I think. Would
that be useful for you?
--
Michael
Attachments:
0001-Add-support-for-runtime-checks-in-injection-points.patch (text/x-diff, 14.2K) download
From 4767c548b718f30e7f3f427db19fb31146420f3d Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 23 Jan 2024 10:11:46 +0900
Subject: [PATCH] Add support for runtime checks in injection points
This extends injections points with a 1-argument flavor, so as it
becomes possible for callbacks to perform runtime checks.
---
src/include/utils/injection_point.h | 7 +-
src/backend/utils/misc/injection_point.c | 81 +++++++++++++++----
.../expected/injection_points.out | 25 ++++++
.../injection_points--1.0.sql | 10 +++
.../injection_points/injection_points.c | 31 ++++++-
.../injection_points/sql/injection_points.sql | 7 ++
doc/src/sgml/xfunc.sgml | 10 ++-
7 files changed, 151 insertions(+), 20 deletions(-)
diff --git a/src/include/utils/injection_point.h b/src/include/utils/injection_point.h
index 55524b568f..8e1527bb4a 100644
--- a/src/include/utils/injection_point.h
+++ b/src/include/utils/injection_point.h
@@ -16,22 +16,27 @@
*/
#ifdef USE_INJECTION_POINTS
#define INJECTION_POINT(name) InjectionPointRun(name)
+#define INJECTION_POINT_1ARG(name, arg1) InjectionPointRun1Arg(name, arg1)
#else
#define INJECTION_POINT(name) ((void) name)
+#define INJECTION_POINT_1ARG(name) ((void) name, (void) arg1)
#endif
/*
* Typedef for callback function launched by an injection point.
*/
typedef void (*InjectionPointCallback) (const char *name);
+typedef void (*InjectionPointCallback1Arg) (const char *name, void *arg1);
extern Size InjectionPointShmemSize(void);
extern void InjectionPointShmemInit(void);
extern void InjectionPointAttach(const char *name,
const char *library,
- const char *function);
+ const char *function,
+ int num_args);
extern void InjectionPointRun(const char *name);
+extern void InjectionPointRun1Arg(const char *name, void *arg1);
extern void InjectionPointDetach(const char *name);
#endif /* INJECTION_POINT_H */
diff --git a/src/backend/utils/misc/injection_point.c b/src/backend/utils/misc/injection_point.c
index 4f52f19853..082622dde9 100644
--- a/src/backend/utils/misc/injection_point.c
+++ b/src/backend/utils/misc/injection_point.c
@@ -49,6 +49,7 @@ typedef struct InjectionPointEntry
char name[INJ_NAME_MAXLEN]; /* hash key */
char library[INJ_LIB_MAXLEN]; /* library */
char function[INJ_FUNC_MAXLEN]; /* function */
+ int num_args; /* number of arguments */
} InjectionPointEntry;
#define INJECTION_POINT_HASH_INIT_SIZE 16
@@ -61,7 +62,12 @@ typedef struct InjectionPointEntry
typedef struct InjectionPointCacheEntry
{
char name[INJ_NAME_MAXLEN];
- InjectionPointCallback callback;
+ int num_args;
+ union
+ {
+ InjectionPointCallback callback;
+ InjectionPointCallback1Arg callback_1arg;
+ };
} InjectionPointCacheEntry;
static HTAB *InjectionPointCache = NULL;
@@ -73,7 +79,8 @@ static HTAB *InjectionPointCache = NULL;
*/
static void
injection_point_cache_add(const char *name,
- InjectionPointCallback callback)
+ void *callback,
+ int num_args)
{
InjectionPointCacheEntry *entry;
bool found;
@@ -99,6 +106,13 @@ injection_point_cache_add(const char *name,
Assert(!found);
strlcpy(entry->name, name, sizeof(entry->name));
entry->callback = callback;
+ entry->num_args = num_args;
+ if (num_args == 0)
+ entry->callback = (InjectionPointCallback) callback;
+ else if (num_args == 1)
+ entry->callback_1arg = (InjectionPointCallback1Arg) callback;
+ else
+ elog(ERROR, "unsupported number of arguments"); /* not reachable */
}
/*
@@ -123,7 +137,7 @@ injection_point_cache_remove(const char *name)
*
* Retrieve an injection point from the local cache, if any.
*/
-static InjectionPointCallback
+static InjectionPointCacheEntry *
injection_point_cache_get(const char *name)
{
bool found;
@@ -137,7 +151,7 @@ injection_point_cache_get(const char *name)
hash_search(InjectionPointCache, name, HASH_FIND, &found);
if (found)
- return entry->callback;
+ return entry;
return NULL;
}
@@ -186,7 +200,8 @@ InjectionPointShmemInit(void)
void
InjectionPointAttach(const char *name,
const char *library,
- const char *function)
+ const char *function,
+ int num_args)
{
#ifdef USE_INJECTION_POINTS
InjectionPointEntry *entry_by_name;
@@ -223,6 +238,7 @@ InjectionPointAttach(const char *name,
entry_by_name->library[INJ_LIB_MAXLEN - 1] = '\0';
strlcpy(entry_by_name->function, function, sizeof(entry_by_name->function));
entry_by_name->function[INJ_FUNC_MAXLEN - 1] = '\0';
+ entry_by_name->num_args = num_args;
LWLockRelease(InjectionPointLock);
@@ -253,18 +269,18 @@ InjectionPointDetach(const char *name)
}
/*
- * Execute an injection point, if defined.
+ * Workhorse for execution of an injection point, if defined.
*
* Check first the shared hash table, and adapt the local cache depending
* on that as it could be possible that an entry to run has been removed.
*/
-void
-InjectionPointRun(const char *name)
+static inline void
+InjectionPointRunInternal(const char *name, int num_args, void *arg1)
{
#ifdef USE_INJECTION_POINTS
InjectionPointEntry *entry_by_name;
bool found;
- InjectionPointCallback injection_callback;
+ InjectionPointCacheEntry *cache_entry;
LWLockAcquire(InjectionPointLock, LW_SHARED);
entry_by_name = (InjectionPointEntry *)
@@ -286,10 +302,11 @@ InjectionPointRun(const char *name)
* Check if the callback exists in the local cache, to avoid unnecessary
* external loads.
*/
- injection_callback = injection_point_cache_get(name);
- if (injection_callback == NULL)
+ cache_entry = injection_point_cache_get(name);
+ if (cache_entry == NULL)
{
char path[MAXPGPATH];
+ void *injection_callback;
/* not found in local cache, so load and register */
snprintf(path, MAXPGPATH, "%s/%s%s", pkglib_path,
@@ -299,19 +316,53 @@ InjectionPointRun(const char *name)
elog(ERROR, "could not find library \"%s\" for injection point \"%s\"",
path, name);
- injection_callback = (InjectionPointCallback)
+ injection_callback = (void *)
load_external_function(path, entry_by_name->function, true, NULL);
if (injection_callback == NULL)
elog(ERROR, "could not find function \"%s\" in library \"%s\" for injection point \"%s\"",
- name, entry_by_name->function, path);
+ entry_by_name->function, path, name);
+
+ if (entry_by_name->num_args != num_args)
+ elog(ERROR, "incorrect number of arguments in function \"%s\" for injection point \"%s\": defined %d but expected %d",
+ entry_by_name->function, name,
+ entry_by_name->num_args, num_args);
/* add it to the local cache when found */
- injection_point_cache_add(name, injection_callback);
+ injection_point_cache_add(name, injection_callback, num_args);
+
+ /* and fetch it */
+ cache_entry = injection_point_cache_get(name);
}
- injection_callback(name);
+ if (cache_entry->num_args != num_args)
+ elog(ERROR, "unexpected number of arguments for injection point \"%s\": defined %d but expected %d",
+ name, cache_entry->num_args, num_args);
+
+ if (cache_entry->num_args == 0)
+ cache_entry->callback(name);
+ else if (cache_entry->num_args == 1)
+ cache_entry->callback_1arg(name, arg1);
+ else
+ {
+ Assert(false); /* cannot be reached */
+ }
#else
elog(ERROR, "Injection points are not supported by this build");
#endif
}
+
+/*
+ * Execute an injection point, with no arguments.
+ */
+void
+InjectionPointRun(const char *name)
+{
+ InjectionPointRunInternal(name, 0, NULL);
+}
+/* 1-argument version */
+void
+InjectionPointRun1Arg(const char *name, void *arg1)
+{
+ InjectionPointRunInternal(name, 1, arg1);
+}
diff --git a/src/test/modules/injection_points/expected/injection_points.out b/src/test/modules/injection_points/expected/injection_points.out
index 827456ccc5..b4e70c6831 100644
--- a/src/test/modules/injection_points/expected/injection_points.out
+++ b/src/test/modules/injection_points/expected/injection_points.out
@@ -115,4 +115,29 @@ NOTICE: notice triggered for injection point TestInjectionLog2
(1 row)
+-- 1-argument runs
+SELECT injection_points_run_1arg('TestInjectionLog2', 'blah'); -- error
+ERROR: unexpected number of arguments for injection point "TestInjectionLog2": defined 0 but expected 1
+SELECT injection_points_attach('TestInjectionLogArg1', 'notice_1arg');
+ injection_points_attach
+-------------------------
+
+(1 row)
+
+SELECT injection_points_run('TestInjectionLogArg1'); -- error
+ERROR: incorrect number of arguments in function "injection_notice_1arg" for injection point "TestInjectionLogArg1": defined 1 but expected 0
+SELECT injection_points_run_1arg('TestInjectionLogArg1', 'blah'); -- notice
+NOTICE: notice triggered for injection point TestInjectionLogArg1: blah
+ injection_points_run_1arg
+---------------------------
+
+(1 row)
+
+SELECT injection_points_run_1arg('TestInjectionLogArg1', 'foobar'); -- notice
+NOTICE: notice triggered for injection point TestInjectionLogArg1: foobar
+ injection_points_run_1arg
+---------------------------
+
+(1 row)
+
DROP EXTENSION injection_points;
diff --git a/src/test/modules/injection_points/injection_points--1.0.sql b/src/test/modules/injection_points/injection_points--1.0.sql
index 5944c41716..9b99c17199 100644
--- a/src/test/modules/injection_points/injection_points--1.0.sql
+++ b/src/test/modules/injection_points/injection_points--1.0.sql
@@ -24,6 +24,16 @@ RETURNS void
AS 'MODULE_PATHNAME', 'injection_points_run'
LANGUAGE C STRICT PARALLEL UNSAFE;
+--
+-- injection_points_run_1arg()
+--
+-- Executes the action attached to the injection point.
+--
+CREATE FUNCTION injection_points_run_1arg(IN point_name TEXT, IN arg1 TEXT)
+RETURNS void
+AS 'MODULE_PATHNAME', 'injection_points_run_1arg'
+LANGUAGE C STRICT PARALLEL UNSAFE;
+
--
-- injection_points_detach()
--
diff --git a/src/test/modules/injection_points/injection_points.c b/src/test/modules/injection_points/injection_points.c
index e843e6594f..862f904a75 100644
--- a/src/test/modules/injection_points/injection_points.c
+++ b/src/test/modules/injection_points/injection_points.c
@@ -28,6 +28,7 @@ PG_MODULE_MAGIC;
extern PGDLLEXPORT void injection_error(const char *name);
extern PGDLLEXPORT void injection_notice(const char *name);
+extern PGDLLEXPORT void injection_notice_1arg(const char *name, void *arg1);
/* Set of callbacks available to be attached to an injection point. */
@@ -43,6 +44,13 @@ injection_notice(const char *name)
elog(NOTICE, "notice triggered for injection point %s", name);
}
+void
+injection_notice_1arg(const char *name, void *arg1)
+{
+ const char *str = (const char *) arg1;
+ elog(NOTICE, "notice triggered for injection point %s: %s", name, str);
+}
+
/*
* SQL function for creating an injection point.
*/
@@ -53,15 +61,21 @@ injection_points_attach(PG_FUNCTION_ARGS)
char *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
char *action = text_to_cstring(PG_GETARG_TEXT_PP(1));
char *function;
+ int num_args = 0;
if (strcmp(action, "error") == 0)
function = "injection_error";
else if (strcmp(action, "notice") == 0)
function = "injection_notice";
+ else if (strcmp(action, "notice_1arg") == 0)
+ {
+ function = "injection_notice_1arg";
+ num_args = 1;
+ }
else
elog(ERROR, "incorrect action \"%s\" for injection point creation", action);
- InjectionPointAttach(name, "injection_points", function);
+ InjectionPointAttach(name, "injection_points", function, num_args);
PG_RETURN_VOID();
}
@@ -80,6 +94,21 @@ injection_points_run(PG_FUNCTION_ARGS)
PG_RETURN_VOID();
}
+/*
+ * SQL function for triggering an injection point, 1-argument flavor.
+ */
+PG_FUNCTION_INFO_V1(injection_points_run_1arg);
+Datum
+injection_points_run_1arg(PG_FUNCTION_ARGS)
+{
+ char *name = text_to_cstring(PG_GETARG_TEXT_PP(0));
+ char *arg1 = text_to_cstring(PG_GETARG_TEXT_PP(1));
+
+ INJECTION_POINT_1ARG(name, (void *) arg1);
+
+ PG_RETURN_VOID();
+}
+
/*
* SQL function for dropping an injection point.
*/
diff --git a/src/test/modules/injection_points/sql/injection_points.sql b/src/test/modules/injection_points/sql/injection_points.sql
index 23c7e435ad..eca528508b 100644
--- a/src/test/modules/injection_points/sql/injection_points.sql
+++ b/src/test/modules/injection_points/sql/injection_points.sql
@@ -31,4 +31,11 @@ SELECT injection_points_detach('TestInjectionLog'); -- fails
SELECT injection_points_run('TestInjectionLog2'); -- notice
+-- 1-argument runs
+SELECT injection_points_run_1arg('TestInjectionLog2', 'blah'); -- error
+SELECT injection_points_attach('TestInjectionLogArg1', 'notice_1arg');
+SELECT injection_points_run('TestInjectionLogArg1'); -- error
+SELECT injection_points_run_1arg('TestInjectionLogArg1', 'blah'); -- notice
+SELECT injection_points_run_1arg('TestInjectionLogArg1', 'foobar'); -- notice
+
DROP EXTENSION injection_points;
diff --git a/doc/src/sgml/xfunc.sgml b/doc/src/sgml/xfunc.sgml
index 8a79ad0943..7659c81c09 100644
--- a/doc/src/sgml/xfunc.sgml
+++ b/doc/src/sgml/xfunc.sgml
@@ -3607,13 +3607,15 @@ uint32 WaitEventExtensionNew(const char *wait_event_name)
macro:
<programlisting>
INJECTION_POINT(name);
+INJECTION_POINT_1ARG(name, arg1);
</programlisting>
There are a few injection points already declared at strategic points
within the server code. After adding a new injection point the code needs
to be compiled in order for that injection point to be available in the
binary. Add-ins written in C-language can declare injection points in
- their own code using the same macro.
+ their own code using the same macro. It is possible to pass down arguments
+ to callbacks for runtime checks.
</para>
<para>
@@ -3622,12 +3624,14 @@ INJECTION_POINT(name);
<programlisting>
extern void InjectionPointAttach(const char *name,
const char *library,
- const char *function);
+ const char *function,
+ int num_args);
</programlisting>
<literal>name</literal> is the name of the injection point, which when
reached during execution will execute the <literal>function</literal>
- loaded from <literal>library</literal>.
+ loaded from <literal>library</literal>. <literal>num_args</literal> is
+ the number of arguments that are used by the callback.
</para>
<para>
--
2.43.0
* Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit
2024-01-22 07:59 MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 09:42 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 15:27 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 16:01 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 16:21 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 20:47 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 23:38 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Michael Paquier <michael@paquier.xyz>
@ 2024-01-23 07:12 ` Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-23 23:26 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 14+ messages in thread
From: Heikki Linnakangas @ 2024-01-23 07:12 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: feichanghong <feichanghong@qq.com>; pgsql-bugs <pgsql-bugs@lists.postgresql.org>
On 23/01/2024 01:38, Michael Paquier wrote:
> On Mon, Jan 22, 2024 at 10:47:17PM +0200, Heikki Linnakangas wrote:
>> Michael, it was a pleasure to write this test with the injection points,
>> compared to trying to set up PITR at just the right moment. Thank you! Since
>> this is the first test that uses it, I didn't have any precedence to
>> copy-paste; can you take a look and verify if this is how you imagined the
>> facility to be used?
>
> I was wondering how many weeks it would take for somebody to begin
> playing with that, and here you are 12 hours later..
:-D
> +EXTRA_INSTALL = src/test/modules/injection_points
> [...]
> +#ifdef USE_INJECTION_POINTS
> + if (GinPageIsLeaf(BufferGetPage(stack->buffer)))
> + INJECTION_POINT("gin-leave-leaf-split-incomplete");
> + else
> + INJECTION_POINT("gin-leave-internal-split-incomplete");
> +#endif
>
> Yes, that would be one way of doing it. It makes the code a bit more
> invasive but that would work. Now, you expect this point to be
> conditional depending on the state of the buffer. One thing I was
> thinking of was to extend the APIs in injection_point.c to be able to
> handle a set of arguments as a set of (void*) so as callbacks could
> internally decide what they want to do depending on the running state.
> I didn't have a use for it, but well, you do, so perhaps we could just
> bite the bullet and do it.
>
> That reduces the footprint on the backend code, moving more logic
> behind USE_INJECTION_POINTS in injection_point.c. At the end, you
> should be able to patch the core backend with something like that,
> without using some ifdefs on USE_INJECTION_POINTS:
> INJECTION_POINT_1ARG("gin-leave-leaf-split", (void *) buffer)
>
> And INJECTION_POINT_1ARG() would be just a wrapper around something
> like:
> extern void InjectionPointRun1(const char *name, void *arg1);
I can see that being useful in general. It requires a bespoken callback
function, though. One nice thing about this test was that I didn't need
to write a new C module, only that snippet at the injection point and
some SQL. In more complicated tests a new C module is warranted, but I
think a lot of tests - like this gin test - can be written without it.
Perhaps if the argument was uint64 instead of a pointer, so that the
'injection_point' module could contain ready-made functions that do
things with the argument.
Overall I feel we should not bother with injection point arguments for
now, though. Let's wait until we have a few more tests that would
benefit from that, and then we'll have more information on what the
ideal interface would look like.
--
Heikki Linnakangas
Neon (https://neon.tech)
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit
2024-01-22 07:59 MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 09:42 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 15:27 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 16:01 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 16:21 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 20:47 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 23:38 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Michael Paquier <michael@paquier.xyz>
2024-01-23 07:12 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
@ 2024-01-23 23:26 ` Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 14+ messages in thread
From: Michael Paquier @ 2024-01-23 23:26 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: feichanghong <feichanghong@qq.com>; pgsql-bugs <pgsql-bugs@lists.postgresql.org>
On Tue, Jan 23, 2024 at 09:12:25AM +0200, Heikki Linnakangas wrote:
> I can see that being useful in general. It requires a bespoken callback
> function, though. One nice thing about this test was that I didn't need to
> write a new C module, only that snippet at the injection point and some SQL.
> In more complicated tests a new C module is warranted, but I think a lot of
> tests - like this gin test - can be written without it.
FWIW, I don't mind if src/test/modules/injection_points/ is used as a
single point across the tree as a library storing a large set of
callbacks. All these don't have to be generic, and I think that we
should remain a maximum flexible for bug fixing and advanced testing.
I'm OK to leave the call up to the committer because it comes down to
invasiveness in src/backend/ vs invasiveness in the number of
callbacks or the logic inside the callbacks (sometimes it makes sense
to me to also complicate more the backend), and I'm OK with what you
are doing here for this thread.
> Perhaps if the argument was uint64 instead of a pointer, so that the
> 'injection_point' module could contain ready-made functions that do things
> with the argument.
I am not exactly sure to understand what you mean here.
> Overall I feel we should not bother with injection point arguments for now,
> though. Let's wait until we have a few more tests that would benefit from
> that, and then we'll have more information on what the ideal interface would
> look like.
Yeah, I am not sure if that's worth having for now, but that's always
good to keep in mind that the infra can be extended to handle such
cases even for stable branches.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit
2024-01-22 07:59 MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 09:42 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 15:27 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 16:01 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 16:21 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 20:47 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
@ 2024-01-23 03:39 ` feichanghong <feichanghong@qq.com>
2024-01-29 11:48 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
1 sibling, 1 reply; 14+ messages in thread
From: feichanghong @ 2024-01-23 03:39 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-bugs <pgsql-bugs@lists.postgresql.org>
> On Jan 23, 2024, at 04:47, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>
> On 22/01/2024 18:21, feichanghong wrote:
>>> From a performance point of view, this doesn't matter. Incomplete split are extremely rare. For convenience, though, I added a new function specifically for handling these "leftover" incomplete splits as opposed to finishing a split that you just made, which performs the lock-upgrade. See attached. I think that helps with readability, and makes it less likely that we'll forget the lock-upgrade in the future if the insertion code is refactored.
>> I think that the lock-upgrade in the ginFinishOldSplit function is unsafe
>> because it violates the requirement of the ginStepRight function that
>> "The next page is locked first, before releasing the current page.”
>
> Good point.
>
> I started to work on a more invasive patch that would move the ginFinishOldSplit() calls to ginTraverseLock() and ginStepRight(), doing the interlocking correctly. That makes life easier for the callers; they don't need to deal with incomplete-splits anymore.
>
> But then I read the Page deletion section in the README and understood that my earlier patch is safe, after all. The lock-coupling in ginStepRight() is only needed for searches, not for inserts. There is another mechanism that prevents concurrent page deletions during an insert: VACUUM holds a cleanup-lock on the root page.
>
> Does that make sense, or am I missing something? Here's the same patch as before, with some extra comments to explain why it's safe.
From my understanding, you are right. The README includes the following
explanation:
ginScanToDelete() traverses the whole tree in depth-first manner. It starts
from the full cleanup lock on the tree root. This lock prevents all the
concurrent insertions into this tree while we're deleting pages. However,
there are still might be some in-progress readers, who traversed root before
we locked it.
For insert, ginFindLeafPage maintains a pin on the relevant pages along the
path, naturally retaining the pin on the root page as well. Before calling
ginScanToDelete to delete a page, it secures an exclusive lock on the root page
and no other backend holds a pin on the root page through LockBufferForCleanup.
Best Regards,
Fei Changhong
Alibaba Cloud Computing Ltd.
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit
2024-01-22 07:59 MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 09:42 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 15:27 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 16:01 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 16:21 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 20:47 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-23 03:39 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
@ 2024-01-29 11:48 ` Heikki Linnakangas <hlinnaka@iki.fi>
2024-03-27 06:43 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
0 siblings, 1 reply; 14+ messages in thread
From: Heikki Linnakangas @ 2024-01-29 11:48 UTC (permalink / raw)
To: feichanghong <feichanghong@qq.com>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-bugs <pgsql-bugs@lists.postgresql.org>
On 23/01/2024 05:39, feichanghong wrote:
>
>> On Jan 23, 2024, at 04:47, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>>
>> On 22/01/2024 18:21, feichanghong wrote:
>>>> From a performance point of view, this doesn't matter. Incomplete
>>>> split are extremely rare. For convenience, though, I added a new
>>>> function specifically for handling these "leftover" incomplete
>>>> splits as opposed to finishing a split that you just made, which
>>>> performs the lock-upgrade. See attached. I think that helps with
>>>> readability, and makes it less likely that we'll forget the
>>>> lock-upgrade in the future if the insertion code is refactored.
>>> I think that the lock-upgrade in the ginFinishOldSplit function is unsafe
>>> because it violates the requirement of the ginStepRight function that
>>> "The next page is locked first, before releasing the current page.”
>>
>> Good point.
>>
>> I started to work on a more invasive patch that would move the
>> ginFinishOldSplit() calls to ginTraverseLock() and ginStepRight(),
>> doing the interlocking correctly. That makes life easier for the
>> callers; they don't need to deal with incomplete-splits anymore.
>>
>> But then I read the Page deletion section in the README and understood
>> that my earlier patch is safe, after all. The lock-coupling in
>> ginStepRight() is only needed for searches, not for inserts. There is
>> another mechanism that prevents concurrent page deletions during an
>> insert: VACUUM holds a cleanup-lock on the root page.
>>
>> Does that make sense, or am I missing something? Here's the same patch
>> as before, with some extra comments to explain why it's safe.
>
> From my understanding, you are right. The README includes the following
> explanation:
> ginScanToDelete() traverses the whole tree in depth-first manner. It starts
> from the full cleanup lock on the tree root. This lock prevents all the
> concurrent insertions into this tree while we're deleting pages. However,
> there are still might be some in-progress readers, who traversed root before
> we locked it.
>
> For insert, ginFindLeafPage maintains a pin on the relevant pages along the
> path, naturally retaining the pin on the root page as well. Before calling
> ginScanToDelete to delete a page, it secures an exclusive lock on the
> root page
> and no other backend holds a pin on the root page through
> LockBufferForCleanup.
I have pushed this fix. Thanks for the report and the review!
--
Heikki Linnakangas
Neon (https://neon.tech)
^ permalink raw reply [nested|flat] 14+ messages in thread
* Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit
2024-01-22 07:59 MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 09:42 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 15:27 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 16:01 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 16:21 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 20:47 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-23 03:39 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-29 11:48 ` Re: MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit Heikki Linnakangas <hlinnaka@iki.fi>
@ 2024-03-27 06:43 ` feichanghong <feichanghong@qq.com>
0 siblings, 0 replies; 14+ messages in thread
From: feichanghong @ 2024-03-27 06:43 UTC (permalink / raw)
To: Heikki Linnakangas <hlinnaka@iki.fi>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-bugs <pgsql-bugs@lists.postgresql.org>
Dear hlinnaka,
I once attempted to construct a scenario with multiple backends concurrently
performing the split that leads to gin index corruption. Unfortunately, I did
not succeed. The reason is that ginInsertCleanup requires an Exclusive Lock
on METAPAGE, preventing multiple processes from simultaneously executing the
cleanup.
Maybe I missed something. hlinnaka, do you have any suggestions?
Best Regards,
Fei Changhong
Alibaba Cloud Computing Ltd.
^ permalink raw reply [nested|flat] 14+ messages in thread
* MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit
@ 2024-01-22 08:04 起个啥名好呢 <feichanghong@qq.com>
0 siblings, 0 replies; 14+ messages in thread
From: 起个啥名好呢 @ 2024-01-22 08:04 UTC (permalink / raw)
To: pgsql-bugs <pgsql-bugs@lists.postgresql.org>
Hi,
Environmental information is as follows:
PostgreSQL Version: 17, branch: master, commit: abb0b4fc
Operating system: centos 7, Kernel version: 5.10.13
Client: psql
After the instance crash recovery, I encountered an assertion failure with
MarkBufferDirty when inserting data into a gin index. The specific stack trace
is as follows:
```
#0 0x00007feb10d19277 in raise () from /lib64/libc.so.6
#1 0x00007feb10d1a968 in abort () from /lib64/libc.so.6
#2 0x00000000009a4184 in ExceptionalCondition (
conditionName=conditionName@entry=0xb44e38
"LWLockHeldByMeInMode(BufferDescriptorGetContentLock(bufHdr), LW_EXCLUSIVE)",
fileName=fileName@entry=0xb45594 "bufmgr.c",
lineNumber=lineNumber@entry=2216) at assert.c:66
#3 0x0000000000840da0 in MarkBufferDirty (buffer=buffer@entry=865) at bufmgr.c:2216
#4 0x000000000052f953 in ginPlaceToPage (btree=0x7fff6a3316b0, stack=0x1e5cbd0,
insertdata=0x1e31ce8, updateblkno=626, childbuf=865, buildStats=0x0)
at ginbtree.c:407
#5 0x000000000053084b in ginFinishSplit (btree=0x7fff6a3316b0, stack=0x1e5bac8,
freestack=false, buildStats=0x0) at ginbtree.c:738
#6 0x000000000053103f in ginFindLeafPage (btree=btree@entry=0x7fff6a3316b0,
searchMode=searchMode@entry=false, rootConflictCheck=rootConflictCheck@entry=false)
at ginbtree.c:111
#7 0x000000000053b850 in ginEntryInsert (ginstate=ginstate@entry=0x1e1f708, attnum=1,
key=31597248, category=0 '\000', items=0x1e222d8, nitem=1, buildStats=0x0)
at gininsert.c:195
#8 0x00000000005373f2 in ginInsertCleanup (ginstate=ginstate@entry=0x1e1f708,
full_clean=full_clean@entry=false, fill_fsm=fill_fsm@entry=true,
forceCleanup=forceCleanup@entry=false, stats=stats@entry=0x0) at ginfast.c:930
#9 0x0000000000537f2a in ginHeapTupleFastInsert (ginstate=ginstate@entry=0x1e1f708,
collector=collector@entry=0x7fff6a331ab0) at ginfast.c:471
```
In the function ginFindLeafPage, if we encounter a page marked with
GIN_INCOMPLETE_SPLIT, we call ginFinishSplit to finish the incomplete split and
remove the GIN_INCOMPLETE_SPLIT flag from that page. ginFinishSplit requires
that "On entry, stack->buffer is exclusively locked," as explained in its
comments.
The function ginFindLeafPage determines the type of lock held by ginTraverseLock:
leaf pages hold GIN_EXCLUSIVE, and internal page hold GIN_SHARE. If ginFindLeafPage
traverses to an internal page with an incomplete split, it will only hold a
GIN_SHARE lock, which does not meet the requirements of ginFinishSplit. If a
crash occurs when an internal page is split, but no downlink is inserted in the
parent page, this problem may occur.
I injected the following code into the ginbtree.c file to trigger a crash
during the splitting of an internal page:
```
inline patch 1: src/backend/access/gin/ginbtree.c (+0/-0 lines) download inline-1.patch
--- a/src/backend/access/gin/ginbtree.c
+++ b/src/backend/access/gin/ginbtree.c
@@ -621,6 +621,12 @@ ginPlaceToPage(GinBtree btree, GinBtreeStack *stack,
v1-fix-ginFinishSplit-lock.patch (application/octet-stream, 1.2K) download
diff --git a/src/backend/access/gin/ginbtree.c b/src/backend/access/gin/ginbtree.c
index 39e2c07ad3..4f6fcd4266 100644
--- a/src/backend/access/gin/ginbtree.c
+++ b/src/backend/access/gin/ginbtree.c
@@ -108,7 +108,16 @@ ginFindLeafPage(GinBtree btree, bool searchMode,
* encounter on the way.
*/
if (!searchMode && GinPageIsIncompleteSplit(page))
- ginFinishSplit(btree, stack, false, NULL);
+ {
+ if (access == GIN_SHARE)
+ {
+ /* for internal page, we should relock our page */
+ LockBuffer(stack->buffer, GIN_UNLOCK);
+ LockBuffer(stack->buffer, GIN_EXCLUSIVE);
+ }
+ if GinPageIsIncompleteSplit(page)
+ ginFinishSplit(btree, stack, false, NULL);
+ }
/*
* ok, page is correctly locked, we should check to move right ..,
@@ -123,7 +132,10 @@ ginFindLeafPage(GinBtree btree, bool searchMode,
/* rightmost page */
break;
- stack->buffer = ginStepRight(stack->buffer, btree->index, access);
+ if (!searchMode)
+ stack->buffer = ginStepRight(stack->buffer, btree->index, GIN_EXCLUSIVE);
+ else
+ stack->buffer = ginStepRight(stack->buffer, btree->index, access);
stack->blkno = rightlink;
page = BufferGetPage(stack->buffer);
end of thread, other threads:[~2024-03-27 06:43 UTC | newest]
Thread overview: 14+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-01-22 07:59 MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit feichanghong <feichanghong@qq.com>
2024-01-22 09:42 ` Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 15:27 ` feichanghong <feichanghong@qq.com>
2024-01-22 16:01 ` Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 16:21 ` feichanghong <feichanghong@qq.com>
2024-01-22 20:47 ` Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-22 23:38 ` Michael Paquier <michael@paquier.xyz>
2024-01-23 01:15 ` Michael Paquier <michael@paquier.xyz>
2024-01-23 07:12 ` Heikki Linnakangas <hlinnaka@iki.fi>
2024-01-23 23:26 ` Michael Paquier <michael@paquier.xyz>
2024-01-23 03:39 ` feichanghong <feichanghong@qq.com>
2024-01-29 11:48 ` Heikki Linnakangas <hlinnaka@iki.fi>
2024-03-27 06:43 ` feichanghong <feichanghong@qq.com>
2024-01-22 08:04 MarkBufferDirty Assert held LW_EXCLUSIVE lock fail when ginFinishSplit 起个啥名好呢 <feichanghong@qq.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox