Re: Vectored I/O in bulk_write.c

(Replying to all your messages in this thread together)

> This made me wonder why smgrwritev() and smgrextendv() shouldn't be
> backed by the same implementation, since they are essentially the same
> operation.

+1 to the general idea of merging the write and extend functions.

> The differences are some assertions which might as well be
> moved up to the smgr.c level as they must surely apply to any future
> smgr implementation too, right?, and the segment file creation policy
> which can be controlled with a new argument.  I tried that here.

Currently, smgrwrite/smgrextend just calls through to mdwrite/mdextend. 
I'd like to keep it that way. Otherwise we need to guess what a 
hypothetical smgr implementation might look like.

For example this assertion:

>     /* This assert is too expensive to have on normally ... */
> #ifdef CHECK_WRITE_VS_EXTEND
>     Assert(blocknum >= mdnblocks(reln, forknum));
> #endif

I think that should continue to be md.c's internal affair. For example, 
imagine that you had a syscall like write() but which always writes to 
the end of the file and also returns the offset that the data was 
written to. You would want to assert the returned offset instead of the 
above.

So -1 on moving up the assertions to smgr.c.

Let's bite the bullet and merge the smgrwrite and smgrextend functions 
at the smgr level too. I propose the following signature:

#define SWF_SKIP_FSYNC        0x01
#define SWF_EXTEND        0x02
#define SWF_ZERO        0x04

void smgrwritev(SMgrRelation reln, ForkNumber forknum,
        BlockNumber blocknum,
        const void **buffer, BlockNumber nblocks,
        int flags);

This would replace smgwrite, smgrextend, and smgrzeroextend. The 
mdwritev() function would have the same signature. A single 'flags' arg 
looks better in the callers than booleans, because you don't need to 
remember what 'true' or 'false' means. The callers would look like this:

/* like old smgrwrite(reln, MAIN_FORKNUM, 123, buf, false) */
smgrwritev(reln, MAIN_FORKNUM, 123, buf, 1, 0);

/* like old smgrwrite(reln, MAIN_FORKNUM, 123, buf, true) */
smgrwritev(reln, MAIN_FORKNUM, 123, buf, 1, SWF_SKIP_FSYNC);

/* like old smgrextend(reln, MAIN_FORKNUM, 123, buf, true) */
smgrwritev(reln, MAIN_FORKNUM, 123, buf, 1,
            SWF_EXTEND | SWF_SKIP_FSYNC);

/* like old smgrzeroextend(reln, MAIN_FORKNUM, 123, 10, true) */
smgrwritev(reln, MAIN_FORKNUM, 123, NULL, 10,
            SWF_EXTEND | SWF_ZERO | SWF_SKIP_FSYNC);

> While thinking about that I realised that an existing write-or-extend
> assertion in master is wrong because it doesn't add nblocks.
> 
> Hmm, it's a bit weird that we have nblocks as int or BlockNumber in
> various places, which I think should probably be fixed.

+1

> Here also is a first attempt at improving the memory allocation and
> memory layout.
> ...
> +typedef union BufferSlot
> +{
> +    PGIOAlignedBlock buffer;
> +    dlist_node    freelist_node;
> +}            BufferSlot;
> +

If you allocated the buffers in one large contiguous chunk, you could 
often do one large write() instead of a gathered writev() of multiple 
blocks. That should be even better, although I don't know much of a 
difference it makes. The above layout wastes a fair amount memory too, 
because 'buffer' is I/O aligned.

> I wonder if bulk logging should trigger larger WAL writes in the "Have
> to write it ourselves" case in AdvanceXLInsertBuffer(), since writing
> 8kB of WAL at a time seems like an unnecessarily degraded level of
> performance, especially with wal_sync_method=open_datasync.  Of course
> the real answer is "make sure wal_buffers is high enough for your
> workload" (usually indirectly by automatically scaling from
> shared_buffers), but this problem jumps out when tracing bulk_writes.c
> with default settings.  We write out the index 128kB at a time, but
> the WAL 8kB at a time.

Makes sense.

On 12/03/2024 23:38, Thomas Munro wrote:
> One more observation while I'm thinking about bulk_write.c... hmm, it
> writes the data out and asks the checkpointer to fsync it, but doesn't
> call smgrwriteback().  I assume that means that on Linux the physical
> writeback sometimes won't happen until the checkpointer eventually
> calls fsync() sequentially, one segment file at a time.  I see that
> it's difficult to decide how to do that though; unlike checkpoints,
> which have rate control/spreading, bulk writes could more easily flood
> the I/O subsystem in a burst.  I expect most non-Linux systems'
> write-behind heuristics to fire up for bulk sequential writes, but on
> Linux where most users live, there is no write-behind heuristic AFAIK
> (on the most common file systems anyway), so you have to crank that
> handle if you want it to wake up and smell the coffee before it hits
> internal limits, but then you have to decide how fast to crank it.
> 
> This problem will come into closer focus when we start talking about
> streaming writes.  For the current non-streaming bulk_write.c coding,
> I don't have any particular idea of what to do about that, so I'm just
> noting the observation here.

It would be straightforward to call smgrwriteback() from sgmr_bulk_flush 
every 1 GB of written data for example. It would be nice to do it in the 
background though, and not stall the bulk writing for it. With the AIO 
patches, I presume we could easily start an asynchronous writeback and 
not wait for it to finish.

> Sorry for the sudden wall of text/monologue; this is all a sort of
> reaction to bulk_write.c that I should perhaps have sent to the
> bulk_write.c thread, triggered by a couple of debugging sessions.

Thanks for looking! This all makes sense. The idea of introducing the 
bulk write interface was that now we have a natural place to put all 
these heuristics and optimizations in. That seems to be a success, 
AFAICS none of the things discussed here will change the bulk_write API, 
just the implementation.

-- 
Heikki Linnakangas
Neon (https://neon.tech)