Re: Relation bulk write facility

Hi,

On 2023-11-17 11:37:21 +0100, Heikki Linnakangas wrote:
> The new facility makes it easier to optimize bulk loading, as the
> logic for buffering, WAL-logging, and syncing the relation only needs
> to be implemented once. It's also less error-prone: We have had a
> number of bugs in how a relation is fsync'd - or not - at the end of a
> bulk loading operation. By centralizing that logic to one place, we
> only need to write it correctly once.

One thing I'd like to use the centralized handling for is to track such
writes in pg_stat_io. I don't mean as part of the initial patch, just that
that's another reason I like the facility.


> The new facility is faster for small relations: Instead of of calling
> smgrimmedsync(), we register the fsync to happen at next checkpoint,
> which avoids the fsync latency. That can make a big difference if you
> are e.g. restoring a schema-only dump with lots of relations.

I think this would be a huge win for people running their application tests
against postgres.


> +    bulkw = bulkw_start_smgr(dst, forkNum, use_wal);
> +
>      nblocks = smgrnblocks(src, forkNum);
>  
>      for (blkno = 0; blkno < nblocks; blkno++)
>      {
> +        Page        page;
> +
>          /* If we got a cancel signal during the copy of the data, quit */
>          CHECK_FOR_INTERRUPTS();
>  
> -        smgrread(src, forkNum, blkno, buf.data);
> +        page = bulkw_alloc_buf(bulkw);
> +        smgrread(src, forkNum, blkno, page);
>  
>          if (!PageIsVerifiedExtended(page, blkno,
>                                      PIV_LOG_WARNING | PIV_REPORT_STAT))
> @@ -511,30 +514,9 @@ RelationCopyStorage(SMgrRelation src, SMgrRelation dst,
>           * page this is, so we have to log the full page including any unused
>           * space.
>           */
> -        if (use_wal)
> -            log_newpage(&dst->smgr_rlocator.locator, forkNum, blkno, page, false);
> -
> -        PageSetChecksumInplace(page, blkno);
> -
> -        /*
> -         * Now write the page.  We say skipFsync = true because there's no
> -         * need for smgr to schedule an fsync for this write; we'll do it
> -         * ourselves below.
> -         */
> -        smgrextend(dst, forkNum, blkno, buf.data, true);
> +        bulkw_write(bulkw, blkno, page, false);

I wonder if bulkw_alloc_buf() is a good name - if you naively read this
change, it looks like it'll just leak memory. It also might be taken to be
valid until freed, which I don't think is the case?


> +/*-------------------------------------------------------------------------
> + *
> + * bulk_write.c
> + *      Efficiently and reliably populate a new relation
> + *
> + * The assumption is that no other backends access the relation while we are
> + * loading it, so we can take some shortcuts.  Alternatively, you can use the
> + * buffer manager as usual, if performance is not critical, but you must not
> + * mix operations through the buffer manager and the bulk loading interface at
> + * the same time.

From "Alternatively" onward this is is somewhat confusing.


> + * We bypass the buffer manager to avoid the locking overhead, and call
> + * smgrextend() directly.  A downside is that the pages will need to be
> + * re-read into shared buffers on first use after the build finishes.  That's
> + * usually a good tradeoff for large relations, and for small relations, the
> + * overhead isn't very significant compared to creating the relation in the
> + * first place.

FWIW, I doubt the "isn't very significant" bit is really true.


> + * One tricky point is that because we bypass the buffer manager, we need to
> + * register the relation for fsyncing at the next checkpoint ourselves, and
> + * make sure that the relation is correctly fsync by us or the checkpointer
> + * even if a checkpoint happens concurrently.

"fsync'ed" or such? Otherwise this reads awkwardly for me.



> + *
> + *
> + * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
> + * Portions Copyright (c) 1994, Regents of the University of California
> + *
> + *
> + * IDENTIFICATION
> + *      src/backend/storage/smgr/bulk_write.c
> + *
> + *-------------------------------------------------------------------------
> + */
> +#include "postgres.h"
> +
> +#include "access/xloginsert.h"
> +#include "access/xlogrecord.h"
> +#include "storage/bufmgr.h"
> +#include "storage/bufpage.h"
> +#include "storage/bulk_write.h"
> +#include "storage/proc.h"
> +#include "storage/smgr.h"
> +#include "utils/rel.h"
> +
> +#define MAX_BUFFERED_PAGES XLR_MAX_BLOCK_ID
> +
> +typedef struct BulkWriteBuffer
> +{
> +    Page        page;
> +    BlockNumber blkno;
> +    bool        page_std;
> +    int16        order;
> +} BulkWriteBuffer;
> +

The name makes it sound like this struct itself contains a buffer - but it's
just pointing to one. *BufferRef or such maybe?

I was wondering how you dealt with the alignment of buffers given the struct
definition, which is what lead me to look at this...


> +/*
> + * Bulk writer state for one relation fork.
> + */
> +typedef struct BulkWriteState
> +{
> +    /* Information about the target relation we're writing */
> +    SMgrRelation smgr;

Isn't there a danger of this becoming a dangling pointer? At least until
https://postgr.es/m/CA%2BhUKGJ8NTvqLHz6dqbQnt2c8XCki4r2QvXjBQcXpVwxTY_pvA%40mail.gmail.com
is merged?


> +    ForkNumber    forknum;
> +    bool        use_wal;
> +
> +    /* We keep several pages buffered, and WAL-log them in batches */
> +    int            nbuffered;
> +    BulkWriteBuffer buffers[MAX_BUFFERED_PAGES];
> +
> +    /* Current size of the relation */
> +    BlockNumber pages_written;
> +
> +    /* The RedoRecPtr at the time that the bulk operation started */
> +    XLogRecPtr    start_RedoRecPtr;
> +
> +    Page        zeropage;        /* workspace for filling zeroes */

We really should just have one such page in shared memory somewhere... For WAL
writes as well.

But until then - why do you allocate the page? Seems like we could just use a
static global variable?


> +/*
> + * Write all buffered pages to disk.
> + */
> +static void
> +bulkw_flush(BulkWriteState *bulkw)
> +{
> +    int            nbuffered = bulkw->nbuffered;
> +    BulkWriteBuffer *buffers = bulkw->buffers;
> +
> +    if (nbuffered == 0)
> +        return;
> +
> +    if (nbuffered > 1)
> +    {
> +        int            o;
> +
> +        qsort(buffers, nbuffered, sizeof(BulkWriteBuffer), buffer_cmp);
> +
> +        /*
> +         * Eliminate duplicates, keeping the last write of each block.
> +         * (buffer_cmp uses 'order' as the last sort key)
> +         */

Huh - which use cases would actually cause duplicate writes?


Greetings,

Andres Freund