If you enable the CHECK_WRITE_VS_EXTEND-protected assert in mdwrite(),
you'll trip it anytime you exercise btbuildempty(), blbuildempty(), or
spgbuildempty().
In this case, it may not make any meaningful difference if smgrwrite()
or smgrextend() is called (_mdfd_getseg() behavior won't differ even
with the different flags, so really only the FileWrite() wait event will
be different).
However, it seems like it should still be changed to call smgrextend().
Or, since they only write a few pages, why not use shared buffers like
gistbuildempty() and brinbuildempty() do?
I've attached a patch to move these three into shared buffers.
And, speaking of spgbuildempty(), there is no test exercising it in
check-world, so I've attached a patch to do so. I wasn't sure if it
belonged in spgist.sql or create_index_spgist.sql (on name alone, seems
like the latter but based on content, seems like the former).
- Melanie
From f4f594ed06d5cf54135e4f9974fd2158ead8a7bb Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Wed, 2 Mar 2022 15:42:48 -0500
Subject: [PATCH v1 1/2] Build unlogged index init fork in shared buffers
Move empty index build in the init fork for unlogged bloom, btree, and
spgist indexes into shared buffers. Previously, the initial pages of
these indexes were extended with smgrwrite(). smgrwrite() is not
meant to be used for relation extension. Instead of using smgrextend(),
though, move the build to shared buffers (currently brinbuildempty() and
gistbuildempty() build in shared buffers).
---
contrib/bloom/blinsert.c | 32 ++++--------
src/backend/access/nbtree/nbtree.c | 32 ++++--------
src/backend/access/spgist/spginsert.c | 74 ++++++++++++---------------
3 files changed, 53 insertions(+), 85 deletions(-)
diff --git a/contrib/bloom/blinsert.c b/contrib/bloom/blinsert.c
index c94cf34e69..47f4f1cf62 100644
--- a/contrib/bloom/blinsert.c
+++ b/contrib/bloom/blinsert.c
@@ -163,31 +163,17 @@ blbuild(Relation heap, Relation index, IndexInfo *indexInfo)
void
blbuildempty(Relation index)
{
- Page metapage;
+ Buffer metabuf =
+ ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL);
+ LockBuffer(metabuf, BUFFER_LOCK_EXCLUSIVE);
+ BloomFillMetapage(index, BufferGetPage(metabuf));
- /* Construct metapage. */
- metapage = (Page) palloc(BLCKSZ);
- BloomFillMetapage(index, metapage);
+ START_CRIT_SECTION();
+ MarkBufferDirty(metabuf);
+ log_newpage_buffer(metabuf, true);
+ END_CRIT_SECTION();
- /*
- * Write the page and log it. It might seem that an immediate sync would
- * be sufficient to guarantee that the file exists on disk, but recovery
- * itself might remove it while replaying, for example, an
- * XLOG_DBASE_CREATE or XLOG_TBLSPC_CREATE record. Therefore, we need
- * this even when wal_level=minimal.
- */
- PageSetChecksumInplace(metapage, BLOOM_METAPAGE_BLKNO);
- smgrwrite(RelationGetSmgr(index), INIT_FORKNUM, BLOOM_METAPAGE_BLKNO,
- (char *) metapage, true);
- log_newpage(&(RelationGetSmgr(index))->smgr_rnode.node, INIT_FORKNUM,
- BLOOM_METAPAGE_BLKNO, metapage, true);
-
- /*
- * An immediate sync is required even if we xlog'd the page, because the
- * write did not go through shared_buffers and therefore a concurrent
- * checkpoint may have moved the redo pointer past our xlog record.
- */
- smgrimmedsync(RelationGetSmgr(index), INIT_FORKNUM);
+ UnlockReleaseBuffer(metabuf);
}
/*
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index c9b4964c1e..bac531609d 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -151,31 +151,19 @@ bthandler(PG_FUNCTION_ARGS)
void
btbuildempty(Relation index)
{
- Page metapage;
+ Buffer metabuf =
+ ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL);
+ LockBuffer(metabuf, BUFFER_LOCK_EXCLUSIVE);
- /* Construct metapage. */
- metapage = (Page) palloc(BLCKSZ);
- _bt_initmetapage(metapage, P_NONE, 0, _bt_allequalimage(index, false));
+ _bt_initmetapage(BufferGetPage(metabuf), P_NONE, 0,
+ _bt_allequalimage(index, false));
- /*
- * Write the page and log it. It might seem that an immediate sync would
- * be sufficient to guarantee that the file exists on disk, but recovery
- * itself might remove it while replaying, for example, an
- * XLOG_DBASE_CREATE or XLOG_TBLSPC_CREATE record. Therefore, we need
- * this even when wal_level=minimal.
- */
- PageSetChecksumInplace(metapage, BTREE_METAPAGE);
- smgrwrite(RelationGetSmgr(index), INIT_FORKNUM, BTREE_METAPAGE,
- (char *) metapage, true);
- log_newpage(&RelationGetSmgr(index)->smgr_rnode.node, INIT_FORKNUM,
- BTREE_METAPAGE, metapage, true);
+ START_CRIT_SECTION();
+ MarkBufferDirty(metabuf);
+ log_newpage_buffer(metabuf, true);
+ END_CRIT_SECTION();
- /*
- * An immediate sync is required even if we xlog'd the page, because the
- * write did not go through shared_buffers and therefore a concurrent
- * checkpoint may have moved the redo pointer past our xlog record.
- */
- smgrimmedsync(RelationGetSmgr(index), INIT_FORKNUM);
+ UnlockReleaseBuffer(metabuf);
}
/*
diff --git a/src/backend/access/spgist/spginsert.c b/src/backend/access/spgist/spginsert.c
index bfb74049d0..d574f35899 100644
--- a/src/backend/access/spgist/spginsert.c
+++ b/src/backend/access/spgist/spginsert.c
@@ -155,49 +155,43 @@ spgbuild(Relation heap, Relation index, IndexInfo *indexInfo)
void
spgbuildempty(Relation index)
{
- Page page;
+ Buffer metabuf, rootbuf, nullbuf;
- /* Construct metapage. */
- page = (Page) palloc(BLCKSZ);
- SpGistInitMetapage(page);
+ metabuf =
+ ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL);
+ LockBuffer(metabuf, BUFFER_LOCK_EXCLUSIVE);
- /*
- * Write the page and log it unconditionally. This is important
- * particularly for indexes created on tablespaces and databases whose
- * creation happened after the last redo pointer as recovery removes any
- * of their existing content when the corresponding create records are
- * replayed.
- */
- PageSetChecksumInplace(page, SPGIST_METAPAGE_BLKNO);
- smgrwrite(RelationGetSmgr(index), INIT_FORKNUM, SPGIST_METAPAGE_BLKNO,
- (char *) page, true);
- log_newpage(&(RelationGetSmgr(index))->smgr_rnode.node, INIT_FORKNUM,
- SPGIST_METAPAGE_BLKNO, page, true);
-
- /* Likewise for the root page. */
- SpGistInitPage(page, SPGIST_LEAF);
-
- PageSetChecksumInplace(page, SPGIST_ROOT_BLKNO);
- smgrwrite(RelationGetSmgr(index), INIT_FORKNUM, SPGIST_ROOT_BLKNO,
- (char *) page, true);
- log_newpage(&(RelationGetSmgr(index))->smgr_rnode.node, INIT_FORKNUM,
- SPGIST_ROOT_BLKNO, page, true);
-
- /* Likewise for the null-tuples root page. */
- SpGistInitPage(page, SPGIST_LEAF | SPGIST_NULLS);
-
- PageSetChecksumInplace(page, SPGIST_NULL_BLKNO);
- smgrwrite(RelationGetSmgr(index), INIT_FORKNUM, SPGIST_NULL_BLKNO,
- (char *) page, true);
- log_newpage(&(RelationGetSmgr(index))->smgr_rnode.node, INIT_FORKNUM,
- SPGIST_NULL_BLKNO, page, true);
+ rootbuf =
+ ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL);
+ LockBuffer(rootbuf, BUFFER_LOCK_EXCLUSIVE);
- /*
- * An immediate sync is required even if we xlog'd the pages, because the
- * writes did not go through shared buffers and therefore a concurrent
- * checkpoint may have moved the redo pointer past our xlog record.
- */
- smgrimmedsync(RelationGetSmgr(index), INIT_FORKNUM);
+ nullbuf =
+ ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL);
+ LockBuffer(nullbuf, BUFFER_LOCK_EXCLUSIVE);
+
+ Assert(BufferGetBlockNumber(metabuf) == SPGIST_METAPAGE_BLKNO);
+ Assert(BufferGetBlockNumber(rootbuf) == SPGIST_ROOT_BLKNO);
+ Assert(BufferGetBlockNumber(nullbuf) == SPGIST_NULL_BLKNO);
+
+ START_CRIT_SECTION();
+
+ SpGistInitMetapage(BufferGetPage(metabuf));
+ MarkBufferDirty(metabuf);
+ log_newpage_buffer(metabuf, true);
+
+ SpGistInitPage(BufferGetPage(rootbuf), SPGIST_LEAF);
+ MarkBufferDirty(rootbuf);
+ log_newpage_buffer(rootbuf, true);
+
+ SpGistInitPage(BufferGetPage(nullbuf), SPGIST_LEAF | SPGIST_NULLS);
+ MarkBufferDirty(nullbuf);
+ log_newpage_buffer(nullbuf, true);
+
+ END_CRIT_SECTION();
+
+ UnlockReleaseBuffer(metabuf);
+ UnlockReleaseBuffer(rootbuf);
+ UnlockReleaseBuffer(nullbuf);
}
/*
--
2.30.2
From e6aecd7eb941d9f2c32c974b5708c6b5475694cd Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplage...@gmail.com>
Date: Wed, 2 Mar 2022 15:56:11 -0500
Subject: [PATCH v1 2/2] Add test for unlogged SP-GiST index
---
src/test/regress/expected/spgist.out | 7 +++++++
src/test/regress/sql/spgist.sql | 8 ++++++++
2 files changed, 15 insertions(+)
diff --git a/src/test/regress/expected/spgist.out b/src/test/regress/expected/spgist.out
index 1688e0e0a3..70ede2066b 100644
--- a/src/test/regress/expected/spgist.out
+++ b/src/test/regress/expected/spgist.out
@@ -5,6 +5,13 @@
-- testing SP-GiST code itself.
create table spgist_point_tbl(id int4, p point);
create index spgist_point_idx on spgist_point_tbl using spgist(p) with (fillfactor = 75);
+-- Test creating an unlogged SP-GiST index
+-- This will exercise code to create an empty SP-GiST index in the INIT fork
+CREATE UNLOGGED TABLE spgist_point_tbl_unlogged (
+ id int4,
+ p point
+);
+CREATE INDEX spgist_point_idx_unlogged ON spgist_point_tbl_unlogged USING spgist(p);
-- Test vacuum-root operation. It gets invoked when the root is also a leaf,
-- i.e. the index is very small.
insert into spgist_point_tbl (id, p)
diff --git a/src/test/regress/sql/spgist.sql b/src/test/regress/sql/spgist.sql
index 7644f344a9..ee60869b68 100644
--- a/src/test/regress/sql/spgist.sql
+++ b/src/test/regress/sql/spgist.sql
@@ -7,6 +7,14 @@
create table spgist_point_tbl(id int4, p point);
create index spgist_point_idx on spgist_point_tbl using spgist(p) with (fillfactor = 75);
+-- Test creating an unlogged SP-GiST index
+-- This will exercise code to create an empty SP-GiST index in the INIT fork
+CREATE UNLOGGED TABLE spgist_point_tbl_unlogged (
+ id int4,
+ p point
+);
+CREATE INDEX spgist_point_idx_unlogged ON spgist_point_tbl_unlogged USING spgist(p);
+
-- Test vacuum-root operation. It gets invoked when the root is also a leaf,
-- i.e. the index is very small.
insert into spgist_point_tbl (id, p)
--
2.30.2
