Use the buffer cache when initializing an unlogged index.

Some of the ambuildempty functions used smgrwrite() directly, followed
by smgrimmedsync(). A few small problems with that:

Firstly, one is supposed to use smgrextend() when extending a
relation, not smgrwrite(). It doesn't make much difference in
production builds. smgrextend() updates the relation size cache, so
you miss that, but that's harmless because we never use the cached
relation size of an init fork. But if you compile with
CHECK_WRITE_VS_EXTEND, you get an assertion failure.

Secondly, the smgrwrite() calls were performed before WAL-logging, so
the page image written to disk had 0/0 as the LSN, not the LSN of the
WAL record. That's also harmless in practice, but seems sloppy.

Thirdly, it's better to use the buffer cache, because then you don't
need to smgrimmedsync() the relation to disk, which adds latency.
Bypassing the cache makes sense for bulk operations like index
creation, but not when you're just initializing an empty index.
Creation of unlogged tables is hardly performance bottleneck in any
real world applications, but nevertheless.

Backpatch to v16, but no further. These issues should be harmless in
practice, so better to not rock the boat in older branches.

Reviewed-by: Robert Haas
Discussion: https://www.postgresql.org/message-id/6e5bbc08-cdfc-b2b3-9e23-1a914b9850a9@iki.fi

diff --git a/contrib/bloom/blinsert.c b/contrib/bloom/blinsert.c
index b42b9e6c41f5d35130268192a09879c0ffd71cb8..b90145148d4608b945efa4f520c6108f15cbdca8 100644 (file)
--- a/contrib/bloom/blinsert.c
+++ b/contrib/bloom/blinsert.c
@@ -129,7 +129,7 @@ blbuild(Relation heap, Relation index, IndexInfo *indexInfo)
             RelationGetRelationName(index));
 
    /* Initialize the meta page */
-   BloomInitMetapage(index);
+   BloomInitMetapage(index, MAIN_FORKNUM);
 
    /* Initialize the bloom build state */
    memset(&buildstate, 0, sizeof(buildstate));
@@ -163,31 +163,8 @@ blbuild(Relation heap, Relation index, IndexInfo *indexInfo)
 void
 blbuildempty(Relation index)
 {
-   Page        metapage;
-
-   /* Construct metapage. */
-   metapage = (Page) palloc_aligned(BLCKSZ, PG_IO_ALIGN_SIZE, 0);
-   BloomFillMetapage(index, metapage);
-
-   /*
-    * 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,
-             metapage, true);
-   log_newpage(&(RelationGetSmgr(index))->smgr_rlocator.locator, 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);
+   /* Initialize the meta page */
+   BloomInitMetapage(index, INIT_FORKNUM);
 }
 
 /*

diff --git a/contrib/bloom/bloom.h b/contrib/bloom/bloom.h
index efdf9415d151a70c4c192c06678801867db8fa33..330811ec60817b1b7d0731aa0f6eccefbe109668 100644 (file)
--- a/contrib/bloom/bloom.h
+++ b/contrib/bloom/bloom.h
@@ -177,7 +177,7 @@ typedef BloomScanOpaqueData *BloomScanOpaque;
 /* blutils.c */
 extern void initBloomState(BloomState *state, Relation index);
 extern void BloomFillMetapage(Relation index, Page metaPage);
-extern void BloomInitMetapage(Relation index);
+extern void BloomInitMetapage(Relation index, ForkNumber forknum);
 extern void BloomInitPage(Page page, uint16 flags);
 extern Buffer BloomNewBuffer(Relation index);
 extern void signValue(BloomState *state, BloomSignatureWord *sign, Datum value, int attno);

diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c
index a017d58bbe4aa4ca2158fc623f422be849024aaa..f23fbb1d9e0a6ef4c5a1705326079f87bdc90bc8 100644 (file)
--- a/contrib/bloom/blutils.c
+++ b/contrib/bloom/blutils.c
@@ -443,7 +443,7 @@ BloomFillMetapage(Relation index, Page metaPage)
  * Initialize metapage for bloom index.
  */
 void
-BloomInitMetapage(Relation index)
+BloomInitMetapage(Relation index, ForkNumber forknum)
 {
    Buffer      metaBuffer;
    Page        metaPage;
@@ -451,9 +451,11 @@ BloomInitMetapage(Relation index)
 
    /*
     * Make a new page; since it is first page it should be associated with
-    * block number 0 (BLOOM_METAPAGE_BLKNO).
+    * block number 0 (BLOOM_METAPAGE_BLKNO).  No need to hold the extension
+    * lock because there cannot be concurrent inserters yet.
     */
-   metaBuffer = BloomNewBuffer(index);
+   metaBuffer = ReadBufferExtended(index, forknum, P_NEW, RBM_NORMAL, NULL);
+   LockBuffer(metaBuffer, BUFFER_LOCK_EXCLUSIVE);
    Assert(BufferGetBlockNumber(metaBuffer) == BLOOM_METAPAGE_BLKNO);
 
    /* Initialize contents of meta page */

diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 4553aaee53187805d2a2477ea1fd525ffa52e52d..ad07b80f758de12f96010b434d8eb8c6e1fd6bef 100644 (file)
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -151,31 +151,32 @@ bthandler(PG_FUNCTION_ARGS)
 void
 btbuildempty(Relation index)
 {
+   Buffer      metabuf;
    Page        metapage;
 
-   /* Construct metapage. */
-   metapage = (Page) palloc_aligned(BLCKSZ, PG_IO_ALIGN_SIZE, 0);
-   _bt_initmetapage(metapage, 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.
+    * Initalize the metapage.
+    *
+    * Regular index build bypasses the buffer manager and uses smgr functions
+    * directly, with an smgrimmedsync() call at the end.  That makes sense
+    * when the index is large, but for an empty index, it's better to use the
+    * buffer cache to avoid the smgrimmedsync().
     */
-   PageSetChecksumInplace(metapage, BTREE_METAPAGE);
-   smgrwrite(RelationGetSmgr(index), INIT_FORKNUM, BTREE_METAPAGE,
-             metapage, true);
-   log_newpage(&RelationGetSmgr(index)->smgr_rlocator.locator, INIT_FORKNUM,
-               BTREE_METAPAGE, metapage, true);
+   metabuf = ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL);
+   Assert(BufferGetBlockNumber(metabuf) == BTREE_METAPAGE);
+   _bt_lockbuf(index, metabuf, BT_WRITE);
 
-   /*
-    * 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);
+   START_CRIT_SECTION();
+
+   metapage = BufferGetPage(metabuf);
+   _bt_initmetapage(metapage, P_NONE, 0, _bt_allequalimage(index, false));
+   MarkBufferDirty(metabuf);
+   log_newpage_buffer(metabuf, true);
+
+   END_CRIT_SECTION();
+
+   _bt_unlockbuf(index, metabuf);
+   ReleaseBuffer(metabuf);
 }
 
 /*

diff --git a/src/backend/access/spgist/spginsert.c b/src/backend/access/spgist/spginsert.c
index 72d2e1551cd4fd0c749e3eb6afbf436b9a5409f1..4443f1918dfe87c8661375d3ac57062b8b8f29dd 100644 (file)
--- a/src/backend/access/spgist/spginsert.c
+++ b/src/backend/access/spgist/spginsert.c
@@ -155,49 +155,42 @@ spgbuild(Relation heap, Relation index, IndexInfo *indexInfo)
 void
 spgbuildempty(Relation index)
 {
-   Page        page;
-
-   /* Construct metapage. */
-   page = (Page) palloc_aligned(BLCKSZ, PG_IO_ALIGN_SIZE, 0);
-   SpGistInitMetapage(page);
+   Buffer      metabuffer,
+               rootbuffer,
+               nullbuffer;
 
    /*
-    * 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.
+    * Initialize the meta page and root pages
     */
-   PageSetChecksumInplace(page, SPGIST_METAPAGE_BLKNO);
-   smgrwrite(RelationGetSmgr(index), INIT_FORKNUM, SPGIST_METAPAGE_BLKNO,
-             page, true);
-   log_newpage(&(RelationGetSmgr(index))->smgr_rlocator.locator, 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,
-             page, true);
-   log_newpage(&(RelationGetSmgr(index))->smgr_rlocator.locator, 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,
-             page, true);
-   log_newpage(&(RelationGetSmgr(index))->smgr_rlocator.locator, INIT_FORKNUM,
-               SPGIST_NULL_BLKNO, page, true);
+   metabuffer = ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL);
+   LockBuffer(metabuffer, BUFFER_LOCK_EXCLUSIVE);
+   rootbuffer = ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL);
+   LockBuffer(rootbuffer, BUFFER_LOCK_EXCLUSIVE);
+   nullbuffer = ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_NORMAL, NULL);
+   LockBuffer(nullbuffer, 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);
+   Assert(BufferGetBlockNumber(metabuffer) == SPGIST_METAPAGE_BLKNO);
+   Assert(BufferGetBlockNumber(rootbuffer) == SPGIST_ROOT_BLKNO);
+   Assert(BufferGetBlockNumber(nullbuffer) == SPGIST_NULL_BLKNO);
+
+   START_CRIT_SECTION();
+
+   SpGistInitMetapage(BufferGetPage(metabuffer));
+   MarkBufferDirty(metabuffer);
+   SpGistInitBuffer(rootbuffer, SPGIST_LEAF);
+   MarkBufferDirty(rootbuffer);
+   SpGistInitBuffer(nullbuffer, SPGIST_LEAF | SPGIST_NULLS);
+   MarkBufferDirty(nullbuffer);
+
+   log_newpage_buffer(metabuffer, true);
+   log_newpage_buffer(rootbuffer, true);
+   log_newpage_buffer(nullbuffer, true);
+
+   END_CRIT_SECTION();
+
+   UnlockReleaseBuffer(metabuffer);
+   UnlockReleaseBuffer(rootbuffer);
+   UnlockReleaseBuffer(nullbuffer);
 }
 
 /*