Re: Avoiding smgrimmedsync() during nbtree index builds

Hi,

> From a06407b19c8d168ea966e45c0e483b38d49ddc12 Mon Sep 17 00:00:00 2001
> From: Melanie Plageman <melanieplageman(at)gmail(dot)com>
> Date: Fri, 4 Mar 2022 14:48:39 -0500
> Subject: [PATCH v6 1/4] Add unbuffered IO API

I think this or one of the following patches should update src/backend/access/transam/README

> @@ -164,6 +164,16 @@ void
> blbuildempty(Relation index)
> {
> Page metapage;
> + UnBufferedWriteState wstate;
> +
> + /*
> + * Though this is an unlogged relation, pass do_wal=true since the init
> + * fork of an unlogged index must be wal-logged and fsync'd. This currently
> + * has no effect, as unbuffered_write() does not do log_newpage()
> + * internally. However, were this to be replaced with unbuffered_extend(),
> + * do_wal must be true to ensure the data is logged and fsync'd.
> + */
> + unbuffered_prep(&wstate, true);

Wonder if unbuffered_write should have an assert checking that no writes to
INIT_FORKNUM are non-durable? Looks like it's pretty easy to forget...

I'd choose unbuffered_begin over _prep().

> /* Construct metapage. */
> metapage = (Page) palloc(BLCKSZ);
> @@ -176,18 +186,13 @@ blbuildempty(Relation index)
> * 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);
> + unbuffered_write(&wstate, RelationGetSmgr(index), INIT_FORKNUM,
> + BLOOM_METAPAGE_BLKNO, metapage);
> +
> 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);
> + unbuffered_finish(&wstate, RelationGetSmgr(index), INIT_FORKNUM);
> }

I mildly prefer complete over finish, but ...

> - * We can't use the normal heap_insert function to insert into the new
> - * heap, because heap_insert overwrites the visibility information.
> - * We use a special-purpose raw_heap_insert function instead, which
> - * is optimized for bulk inserting a lot of tuples, knowing that we have
> - * exclusive access to the heap. raw_heap_insert builds new pages in
> - * local storage. When a page is full, or at the end of the process,
> - * we insert it to WAL as a single record and then write it to disk
> - * directly through smgr. Note, however, that any data sent to the new
> - * heap's TOAST table will go through the normal bufmgr.
> + * We can't use the normal heap_insert function to insert into the new heap,
> + * because heap_insert overwrites the visibility information. We use a
> + * special-purpose raw_heap_insert function instead, which is optimized for
> + * bulk inserting a lot of tuples, knowing that we have exclusive access to the
> + * heap. raw_heap_insert builds new pages in local storage. When a page is
> + * full, or at the end of the process, we insert it to WAL as a single record
> + * and then write it to disk directly through directmgr. Note, however, that
> + * any data sent to the new heap's TOAST table will go through the normal
> + * bufmgr.

Part of this just reflows existing lines that seem otherwise unchanged, making
it harder to see the actual change.

> @@ -643,13 +644,6 @@ _bt_blnewpage(uint32 level)
> static void
> _bt_blwritepage(BTWriteState *wstate, Page page, BlockNumber blkno)
> {
> - /* XLOG stuff */
> - if (wstate->btws_use_wal)
> - {
> - /* We use the XLOG_FPI record type for this */
> - log_newpage(&wstate->index->rd_node, MAIN_FORKNUM, blkno, page, true);
> - }
> -
> /*
> * If we have to write pages nonsequentially, fill in the space with
> * zeroes until we come back and overwrite. This is not logically
> @@ -661,33 +655,33 @@ _bt_blwritepage(BTWriteState *wstate, Page page, BlockNumber blkno)
> {
> if (!wstate->btws_zeropage)
> wstate->btws_zeropage = (Page) palloc0(BLCKSZ);
> - /* don't set checksum for all-zero page */
> - smgrextend(RelationGetSmgr(wstate->index), MAIN_FORKNUM,
> - wstate->btws_pages_written++,
> - (char *) wstate->btws_zeropage,
> - true);
> +
> + unbuffered_extend(&wstate->ub_wstate, RelationGetSmgr(wstate->index),
> + MAIN_FORKNUM, wstate->btws_pages_written++,
> + wstate->btws_zeropage, true);
> }

For a bit I thought the true argument to unbuffered_extend was about
durability or registering fsyncs. Perhaps worth making it flags argument with
an enum for flag arguments?

> diff --git a/src/backend/storage/direct/directmgr.c b/src/backend/storage/direct/directmgr.c
> new file mode 100644
> index 0000000000..42c37daa7a
> --- /dev/null
> +++ b/src/backend/storage/direct/directmgr.c
> @@ -0,0 +1,98 @@

Now that the API is called unbuffered, the filename / directory seem
confusing.

> +void
> +unbuffered_prep(UnBufferedWriteState *wstate, bool do_wal)
> +{
> + wstate->do_wal = do_wal;
> +}
> +
> +void
> +unbuffered_extend(UnBufferedWriteState *wstate, SMgrRelation
> + smgrrel, ForkNumber forknum, BlockNumber blocknum, Page page, bool
> + empty)
> +{
> + /*
> + * Don't checksum empty pages
> + */
> + if (!empty)
> + PageSetChecksumInplace(page, blocknum);
> +
> + smgrextend(smgrrel, forknum, blocknum, (char *) page, true);
> +
> + /*
> + * Don't WAL-log empty pages
> + */
> + if (!empty && wstate->do_wal)
> + log_newpage(&(smgrrel)->smgr_rnode.node, forknum,
> + blocknum, page, true);
> +}
> +
> +void
> +unbuffered_extend_range(UnBufferedWriteState *wstate, SMgrRelation smgrrel,
> + ForkNumber forknum, int num_pages, BlockNumber *blocknums, Page *pages,
> + bool empty, XLogRecPtr custom_lsn)
> +{
> + for (int i = 0; i < num_pages; i++)
> + {
> + Page page = pages[i];
> + BlockNumber blkno = blocknums[i];
> +
> + if (!XLogRecPtrIsInvalid(custom_lsn))
> + PageSetLSN(page, custom_lsn);
> +
> + if (!empty)
> + PageSetChecksumInplace(page, blkno);
> +
> + smgrextend(smgrrel, forknum, blkno, (char *) page, true);
> + }
> +
> + if (!empty && wstate->do_wal)
> + log_newpages(&(smgrrel)->smgr_rnode.node, forknum, num_pages,
> + blocknums, pages, true);
> +}
> +
> +void
> +unbuffered_write(UnBufferedWriteState *wstate, SMgrRelation smgrrel, ForkNumber
> + forknum, BlockNumber blocknum, Page page)
> +{
> + PageSetChecksumInplace(page, blocknum);
> +
> + smgrwrite(smgrrel, forknum, blocknum, (char *) page, true);
> +}

Seem several of these should have some minimal documentation?

> +/*
> + * When writing data outside shared buffers, a concurrent CHECKPOINT can move
> + * the redo pointer past our WAL entries and won't flush our data to disk. If
> + * the database crashes before the data makes it to disk, our WAL won't be
> + * replayed and the data will be lost.
> + * Thus, if a CHECKPOINT begins between unbuffered_prep() and
> + * unbuffered_finish(), the backend must fsync the data itself.
> + */

Hm. The last sentence sounds like this happens conditionally, but it doesn't
at this point.

> +typedef struct UnBufferedWriteState
> +{
> + /*
> + * When writing WAL-logged relation data outside of shared buffers, there
> + * is a risk of a concurrent CHECKPOINT moving the redo pointer past the
> + * data's associated WAL entries. To avoid this, callers in this situation
> + * must fsync the pages they have written themselves. This is necessary
> + * only if the relation is WAL-logged or in special cases such as the init
> + * fork of an unlogged index.
> + */
> + bool do_wal;
> +} UnBufferedWriteState;
> +/*
> + * prototypes for functions in directmgr.c
> + */

Newline in between end of struct and comment.

> +extern void
> +unbuffered_prep(UnBufferedWriteState *wstate, bool do_wal);

In headers we don't put the return type on a separate line :/

> --- a/contrib/bloom/blinsert.c
> +++ b/contrib/bloom/blinsert.c
> @@ -173,7 +173,7 @@ blbuildempty(Relation index)
> * internally. However, were this to be replaced with unbuffered_extend(),
> * do_wal must be true to ensure the data is logged and fsync'd.
> */
> - unbuffered_prep(&wstate, true);
> + unbuffered_prep(&wstate, true, false);

This makes me think this really should be a flag argument...

I also don't like the current name of the parameter "do_optimization" doesn't
explain much.

> +bool RedoRecPtrChanged(XLogRecPtr comparator_ptr)
> +{

newline after return type.

> void
> -unbuffered_prep(UnBufferedWriteState *wstate, bool do_wal)
> +unbuffered_prep(UnBufferedWriteState *wstate, bool do_wal, bool
> + do_optimization)

See earlier comments about documentation and parameter naming...

> + * These callers can optionally use the following optimization: attempt to
> + * use the sync request queue and fall back to fsync'ing the pages
> + * themselves if the Redo pointer moves between the start and finish of
> + * their write. In order to do this, they must set do_optimization to true
> + * so that the redo pointer is saved before the write begins.
> */

When do we not want this?

> From 17fb22142ade65fdbe8c90889e49d0be60ba45e4 Mon Sep 17 00:00:00 2001
> From: Melanie Plageman <melanieplageman(at)gmail(dot)com>
> Date: Fri, 4 Mar 2022 15:53:05 -0500
> Subject: [PATCH v6 3/4] BTree index use unbuffered IO optimization
>
> While building a btree index, the backend can avoid fsync'ing all of the
> pages if it uses the optimization introduced in a prior commit.
>
> This can substantially improve performance when many indexes are being
> built during DDL operations.
> ---
> src/backend/access/nbtree/nbtree.c | 2 +-
> src/backend/access/nbtree/nbtsort.c | 6 +++++-
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
> index 6b78acefbe..fc5cce4603 100644
> --- a/src/backend/access/nbtree/nbtree.c
> +++ b/src/backend/access/nbtree/nbtree.c
> @@ -161,7 +161,7 @@ btbuildempty(Relation index)
> * internally. However, were this to be replaced with unbuffered_extend(),
> * do_wal must be true to ensure the data is logged and fsync'd.
> */
> - unbuffered_prep(&wstate, true, false);
> + unbuffered_prep(&wstate, true, true);
>
> /* Construct metapage. */
> metapage = (Page) palloc(BLCKSZ);
> diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c
> index d6d0d4b361..f1b9e2e24e 100644
> --- a/src/backend/access/nbtree/nbtsort.c
> +++ b/src/backend/access/nbtree/nbtsort.c
> @@ -1189,7 +1189,11 @@ _bt_load(BTWriteState *wstate, BTSpool *btspool, BTSpool *btspool2)
> int64 tuples_done = 0;
> bool deduplicate;
>
> - unbuffered_prep(&wstate->ub_wstate, wstate->btws_use_wal, false);
> + /*
> + * The fsync optimization done by directmgr is only relevant if
> + * WAL-logging, so pass btws_use_wal for this parameter.
> + */
> + unbuffered_prep(&wstate->ub_wstate, wstate->btws_use_wal, wstate->btws_use_wal);
>
> deduplicate = wstate->inskey->allequalimage && !btspool->isunique &&
> BTGetDeduplicateItems(wstate->index);

Why just here?

> From 377c195bccf2dd2529e64d0d453104485f7662b7 Mon Sep 17 00:00:00 2001
> From: Melanie Plageman <melanieplageman(at)gmail(dot)com>
> Date: Fri, 4 Mar 2022 15:52:45 -0500
> Subject: [PATCH v6 4/4] Use shared buffers when possible for index build
>
> When there are not too many tuples, building the index in shared buffers
> makes sense. It allows the buffer manager to handle how best to do the
> IO.
> ---

Perhaps it'd be worth making this an independent patch that could be applied
separately?

> src/backend/access/nbtree/nbtree.c | 32 ++--
> src/backend/access/nbtree/nbtsort.c | 273 +++++++++++++++++++++-------
> 2 files changed, 223 insertions(+), 82 deletions(-)
>
> diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
> index fc5cce4603..d3982b9388 100644
> --- a/src/backend/access/nbtree/nbtree.c
> +++ b/src/backend/access/nbtree/nbtree.c
> @@ -152,34 +152,24 @@ void
> btbuildempty(Relation index)
> {
> Page metapage;
> - UnBufferedWriteState wstate;
> + Buffer metabuf;
>
> /*
> - * Though this is an unlogged relation, pass do_wal=true since the init
> - * fork of an unlogged index must be wal-logged and fsync'd. This currently
> - * has no effect, as unbuffered_write() does not do log_newpage()
> - * internally. However, were this to be replaced with unbuffered_extend(),
> - * do_wal must be true to ensure the data is logged and fsync'd.
> + * Allocate a buffer for metapage and initialize metapage.
> */
> - unbuffered_prep(&wstate, true, true);
> -
> - /* Construct metapage. */
> - metapage = (Page) palloc(BLCKSZ);
> + metabuf = ReadBufferExtended(index, INIT_FORKNUM, P_NEW, RBM_ZERO_AND_LOCK,
> + NULL);
> + metapage = BufferGetPage(metabuf);
> _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.
> + * Mark metapage buffer as dirty and XLOG it
> */
> - unbuffered_write(&wstate, RelationGetSmgr(index), INIT_FORKNUM,
> - BTREE_METAPAGE, metapage);
> - log_newpage(&RelationGetSmgr(index)->smgr_rnode.node, INIT_FORKNUM,
> - BTREE_METAPAGE, metapage, true);
> -
> - unbuffered_finish(&wstate, RelationGetSmgr(index), INIT_FORKNUM);
> + START_CRIT_SECTION();
> + MarkBufferDirty(metabuf);
> + log_newpage_buffer(metabuf, true);
> + END_CRIT_SECTION();
> + _bt_relbuf(index, metabuf);
> }

I don't understand why this patch changes btbuildempty()? That data is never
accessed again, so it doesn't really seem beneficial to shuffle it through
shared buffers, even if we benefit from using s_b for the main fork...

> + /*
> + * If not using shared buffers, for a WAL-logged relation, save the redo
> + * pointer location in case a checkpoint begins during the index build.
> + */
> + if (wstate->_bt_bl_unbuffered_prep)
> + wstate->_bt_bl_unbuffered_prep(&wstate->ub_wstate,
> + wstate->btws_use_wal, wstate->btws_use_wal);

Code would probably look cleaner if an empty callback were used when no
_bt_bl_unbuffered_prep callback is needed.

> /*
> - * allocate workspace for a new, clean btree page, not linked to any siblings.
> + * Set up workspace for a new, clean btree page, not linked to any siblings.
> + * Caller must allocate the passed in page.

More interesting bit seems to be whether the passed in page needs to be
initialized?

> @@ -1154,20 +1285,24 @@ _bt_uppershutdown(BTWriteState *wstate, BTPageState *state)
> * back one slot. Then we can dump out the page.
> */
> _bt_slideleft(s->btps_page);
> - _bt_blwritepage(wstate, s->btps_page, s->btps_blkno);
> + wstate->_bt_blwritepage(wstate, s->btps_page, s->btps_blkno, s->btps_buf);
> + s->btps_buf = InvalidBuffer;
> s->btps_page = NULL; /* writepage freed the workspace */
> }

Do we really have to add underscores even to struct members? That just looks
wrong.

Greetings,

Andres Freund