On 10/04/2019 20:25, Heikki Linnakangas wrote:
On 09/04/2019 19:11, Anastasia Lubennikova wrote:
05.04.2019 19:41, Anastasia Lubennikova writes:
In attachment, you can find patch with a test that allows to reproduce
the bug not randomly, but on every run.
Now I'm trying to find a way to fix the issue.
The problem was caused by incorrect detection of the page to insert new
tuple after split.
If gistinserttuple() of the tuple formed by gistgetadjusted() had to
split the page, we must to go back to the parent and
descend back to the child that's a better fit for the new tuple.
Previously this was handled by the code block with the following comment:
* Concurrent split detected. There's no guarantee that the
* downlink for this page is consistent with the tuple we're
* inserting anymore, so go back to parent and rechoose the best
* child.
After introducing GistBuildNSN this code path became unreachable.
To fix it, I added new flag to detect such splits during indexbuild.
Isn't it possible that the grandparent page is also split, so that we'd
need to climb further up?
Based on Anastasia's idea i prepare alternative solution to fix the bug
(see attachment).
It utilizes the idea of linear increment of LSN/NSN. WAL write process
is used for change NSN value to 1 for each block of index relation.
I hope this can be a fairly clear and safe solution.
--
Andrey Lepikhov
Postgres Professional
https://postgrespro.com
The Russian Postgres Company
>From 59e1519a0a48b879777820ff68116c68ed31e684 Mon Sep 17 00:00:00 2001
From: "Andrey V. Lepikhov" <a.lepik...@postgrespro.ru>
Date: Thu, 11 Apr 2019 10:52:39 +0500
Subject: [PATCH] Alt fix for gist_optimized_wal_intarray_test bug
---
src/backend/access/gin/gininsert.c | 2 +-
src/backend/access/gist/gist.c | 4 ++--
src/backend/access/gist/gistbuild.c | 17 +++++++++++++++--
src/backend/access/spgist/spginsert.c | 2 +-
src/backend/access/transam/xloginsert.c | 4 +++-
src/include/access/gist.h | 6 ------
src/include/access/gist_private.h | 2 ++
src/include/access/xloginsert.h | 6 +++++-
8 files changed, 29 insertions(+), 14 deletions(-)
diff --git a/src/backend/access/gin/gininsert.c b/src/backend/access/gin/gininsert.c
index 55eab14617..a63f33b429 100644
--- a/src/backend/access/gin/gininsert.c
+++ b/src/backend/access/gin/gininsert.c
@@ -413,7 +413,7 @@ ginbuild(Relation heap, Relation index, IndexInfo *indexInfo)
{
log_newpage_range(index, MAIN_FORKNUM,
0, RelationGetNumberOfBlocks(index),
- true);
+ true, NULL);
}
/*
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 2db790c840..56f6ce04db 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -492,7 +492,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
* we don't need to be able to detect concurrent splits yet.)
*/
if (is_build)
- recptr = GistBuildLSN;
+ recptr = gistBuildLSN++;
else
{
if (RelationNeedsWAL(rel))
@@ -559,7 +559,7 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
MarkBufferDirty(leftchildbuf);
if (is_build)
- recptr = GistBuildLSN;
+ recptr = gistBuildLSN++;
else
{
if (RelationNeedsWAL(rel))
diff --git a/src/backend/access/gist/gistbuild.c b/src/backend/access/gist/gistbuild.c
index 8e81eda517..31118b54cf 100644
--- a/src/backend/access/gist/gistbuild.c
+++ b/src/backend/access/gist/gistbuild.c
@@ -76,6 +76,13 @@ typedef struct
GistBufferingMode bufferingMode;
} GISTBuildState;
+/*
+ * A bogus LSN / NSN value used during index build. Must be smaller than any
+ * real or fake unlogged LSN, so that after an index build finishes, all the
+ * splits are considered completed.
+ */
+XLogRecPtr gistBuildLSN = 0;
+
/* prototypes for private functions */
static void gistInitBuffering(GISTBuildState *buildstate);
static int calculatePagesPerBuffer(GISTBuildState *buildstate, int levelStep);
@@ -107,6 +114,12 @@ static void gistMemorizeParent(GISTBuildState *buildstate, BlockNumber child,
static void gistMemorizeAllDownlinks(GISTBuildState *buildstate, Buffer parent);
static BlockNumber gistGetParent(GISTBuildState *buildstate, BlockNumber child);
+static void
+gistbuild_log_mask(char *page)
+{
+ GistPageSetNSN((Page) page, (uint64) 1);
+}
+
/*
* Main entry point to GiST index build. Initially calls insert over and over,
* but switches to more efficient buffering build algorithm after a certain
@@ -180,7 +193,7 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo)
GISTInitBuffer(buffer, F_LEAF);
MarkBufferDirty(buffer);
- PageSetLSN(page, GistBuildLSN);
+ PageSetLSN(page, gistBuildLSN++);
UnlockReleaseBuffer(buffer);
@@ -222,7 +235,7 @@ gistbuild(Relation heap, Relation index, IndexInfo *indexInfo)
{
log_newpage_range(index, MAIN_FORKNUM,
0, RelationGetNumberOfBlocks(index),
- true);
+ true, gistbuild_log_mask);
}
/*
diff --git a/src/backend/access/spgist/spginsert.c b/src/backend/access/spgist/spginsert.c
index b40bd440cf..50f302d045 100644
--- a/src/backend/access/spgist/spginsert.c
+++ b/src/backend/access/spgist/spginsert.c
@@ -139,7 +139,7 @@ spgbuild(Relation heap, Relation index, IndexInfo *indexInfo)
{
log_newpage_range(index, MAIN_FORKNUM,
0, RelationGetNumberOfBlocks(index),
- true);
+ true, NULL);
}
result = (IndexBuildResult *) palloc0(sizeof(IndexBuildResult));
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index 1c76dcfa0d..ab8af41d99 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -1041,7 +1041,7 @@ log_newpage_buffer(Buffer buffer, bool page_std)
void
log_newpage_range(Relation rel, ForkNumber forkNum,
BlockNumber startblk, BlockNumber endblk,
- bool page_std)
+ bool page_std, log_mask_callback mask)
{
BlockNumber blkno;
@@ -1089,6 +1089,8 @@ log_newpage_range(Relation rel, ForkNumber forkNum,
for (i = 0; i < nbufs; i++)
{
XLogRegisterBuffer(i, bufpack[i], REGBUF_FORCE_IMAGE | REGBUF_STANDARD);
+ if (mask)
+ mask(BufferGetPage(bufpack[i]));
MarkBufferDirty(bufpack[i]);
}
diff --git a/src/include/access/gist.h b/src/include/access/gist.h
index 6902f4115b..d09f992e1f 100644
--- a/src/include/access/gist.h
+++ b/src/include/access/gist.h
@@ -49,12 +49,6 @@
typedef XLogRecPtr GistNSN;
-/*
- * A bogus LSN / NSN value used during index build. Must be smaller than any
- * real or fake unlogged LSN, so that after an index build finishes, all the
- * splits are considered completed.
- */
-#define GistBuildLSN ((XLogRecPtr) 1)
/*
* For on-disk compatibility with pre-9.3 servers, NSN is stored as two
diff --git a/src/include/access/gist_private.h b/src/include/access/gist_private.h
index 78e2e3fb31..64469640e4 100644
--- a/src/include/access/gist_private.h
+++ b/src/include/access/gist_private.h
@@ -381,6 +381,8 @@ typedef struct GiSTOptions
int bufferingModeOffset; /* use buffering build? */
} GiSTOptions;
+extern XLogRecPtr gistBuildLSN;
+
/* gist.c */
extern void gistbuildempty(Relation index);
extern bool gistinsert(Relation r, Datum *values, bool *isnull,
diff --git a/src/include/access/xloginsert.h b/src/include/access/xloginsert.h
index 30c4ff7bea..0d56ee2266 100644
--- a/src/include/access/xloginsert.h
+++ b/src/include/access/xloginsert.h
@@ -38,6 +38,8 @@
#define REGBUF_KEEP_DATA 0x10 /* include data even if a full-page image
* is taken */
+typedef void (*log_mask_callback)(char *page);
+
/* prototypes for public functions in xloginsert.c: */
extern void XLogBeginInsert(void);
extern void XLogSetRecordFlags(uint8 flags);
@@ -55,8 +57,10 @@ extern bool XLogCheckBufferNeedsBackup(Buffer buffer);
extern XLogRecPtr log_newpage(RelFileNode *rnode, ForkNumber forkNum,
BlockNumber blk, char *page, bool page_std);
extern XLogRecPtr log_newpage_buffer(Buffer buffer, bool page_std);
+
extern void log_newpage_range(Relation rel, ForkNumber forkNum,
- BlockNumber startblk, BlockNumber endblk, bool page_std);
+ BlockNumber startblk, BlockNumber endblk, bool page_std,
+ log_mask_callback mask);
extern XLogRecPtr XLogSaveBufferForHint(Buffer buffer, bool buffer_std);
extern void InitXLogInsert(void);
--
2.17.1
