Re: Avoiding smgrimmedsync() during nbtree index builds

Hi,

On 2022-01-11 12:10:54 -0500, Melanie Plageman wrote:
> On Mon, Jan 10, 2022 at 5:50 PM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> >
> > I have attached a v3 which includes two commits -- one of which
> > implements the directmgr API and uses it and the other which adds
> > functionality to use either directmgr or bufmgr API during index build.
> >
> > Also registering for march commitfest.
> 
> Forgot directmgr.h. Attached v4

Are you looking at the failures Justin pointed out? Something isn't quite
right yet. See https://postgr.es/m/20220116202559.GW14051%40telsasoft.com and
the subsequent mail about it also triggering on once on linux.


> Thus, the backend must ensure that
> either the Redo pointer has not moved or that the data is fsync'd before
> freeing the page.

"freeing"?


> This is not a problem with pages written in shared buffers because the
> checkpointer will block until all buffers that were dirtied before it
> began finish before it moves the Redo pointer past their associated WAL
> entries.

> This commit makes two main changes:
> 
> 1) It wraps smgrextend() and smgrwrite() in functions from a new API
>    for writing data outside of shared buffers, directmgr.
> 
> 2) It saves the XLOG Redo pointer location before doing the write or
>    extend. It also adds an fsync request for the page to the
>    checkpointer's pending-ops table. Then, after doing the write or
>    extend, if the Redo pointer has moved (meaning a checkpoint has
>    started since it saved it last), then the backend fsync's the page
>    itself. Otherwise, it lets the checkpointer take care of fsync'ing
>    the page the next time it processes the pending-ops table.

Why combine those two into one commit?


> @@ -654,9 +657,8 @@ vm_extend(Relation rel, BlockNumber vm_nblocks)
>      /* Now extend the file */
>      while (vm_nblocks_now < vm_nblocks)
>      {
> -        PageSetChecksumInplace((Page) pg.data, vm_nblocks_now);
> -
> -        smgrextend(reln, VISIBILITYMAP_FORKNUM, vm_nblocks_now, pg.data, false);
> +        // TODO: aren't these pages empty? why checksum them
> +        unbuffered_extend(&ub_wstate, VISIBILITYMAP_FORKNUM, vm_nblocks_now, (Page) pg.data, false);

Yea, it's a bit odd. PageSetChecksumInplace() will just return immediately:

    /* If we don't need a checksum, just return */
    if (PageIsNew(page) || !DataChecksumsEnabled())
        return;

OTOH, it seems easier to have it there than to later forget it, when
e.g. adding some actual initial content to the pages during the smgrextend().



> @@ -560,6 +562,8 @@ _bt_leafbuild(BTSpool *btspool, BTSpool *btspool2)
>  
>      wstate.heap = btspool->heap;
>      wstate.index = btspool->index;
> +    wstate.ub_wstate.smgr_rel = RelationGetSmgr(btspool->index);
> +    wstate.ub_wstate.redo = InvalidXLogRecPtr;
>      wstate.inskey = _bt_mkscankey(wstate.index, NULL);
>      /* _bt_mkscankey() won't set allequalimage without metapage */
>      wstate.inskey->allequalimage = _bt_allequalimage(wstate.index, true);
> @@ -656,31 +660,19 @@ _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, MAIN_FORKNUM, wstate->btws_pages_written++, wstate->btws_zeropage,
true);
>      }

There's a bunch of long lines in here...


> -    /*
> -     * When we WAL-logged index pages, we must nonetheless fsync index files.
> -     * Since we're building outside shared buffers, a CHECKPOINT occurring
> -     * during the build has no way to flush the previously written data to
> -     * disk (indeed it won't know the index even exists).  A crash later on
> -     * would replay WAL from the checkpoint, therefore it wouldn't replay our
> -     * earlier WAL entries. If we do not fsync those pages here, they might
> -     * still not be on disk when the crash occurs.
> -     */
>      if (wstate->btws_use_wal)
> -        smgrimmedsync(RelationGetSmgr(wstate->index), MAIN_FORKNUM);
> +        unbuffered_finish(&wstate->ub_wstate, MAIN_FORKNUM);
>  }

The API of unbuffered_finish() only sometimes getting called, but
unbuffered_prep() being unconditional, strikes me as prone to bugs. Perhaps
it'd make sense to pass in whether the relation needs to be synced or not instead?



>  spgbuildempty(Relation index)
>  {
>      Page        page;
> +    UnBufferedWriteState wstate;
> +
> +    wstate.smgr_rel = RelationGetSmgr(index);
> +    unbuffered_prep(&wstate);

I don't think that's actually safe, and one of the instances could be the
cause cause of the bug CI is seeing:

 * Note: since a relcache flush can cause the file handle to be closed again,
 * it's unwise to hold onto the pointer returned by this function for any
 * long period.  Recommended practice is to just re-execute RelationGetSmgr
 * each time you need to access the SMgrRelation.  It's quite cheap in
 * comparison to whatever an smgr function is going to do.
 */
static inline SMgrRelation
RelationGetSmgr(Relation rel)


Greetings,

Andres Freund