Re: Streaming I/O, vectored I/O (WIP)

> I spent most of the past few days trying to regain some lost
> performance.  Thanks to Andres for some key observations and help!
> That began with reports from Bilal and Melanie (possibly related to
> things Tomas had seen too, not sure) of regressions in all-cached
> workloads, which I already improved a bit with the ABC algorithm that
> minimised pinning for this case.  That is, if there's no recent I/O so
> we reach what I call behaviour A, it should try to do as little magic
> as possible.  But it turns out that wasn't enough!  It is very hard to
> beat a tight loop that just does ReadBuffer(), ReleaseBuffer() over
> millions of already-cached blocks, if you have to do exactly the same
> work AND extra instructions for management.

I got a little nerd-sniped by that, and did some micro-benchmarking of 
my own. I tested essentially this, with small values of 'nblocks' so 
that all pages are in cache:

    for (int i = 0; i < niters; i++)
    {
        for (BlockNumber blkno = 0; blkno < nblocks; blkno++)
        {
            buf = ReadBuffer(rel, blkno);
            ReleaseBuffer(buf);
        }
    }

The results look like this (lower is better, test program and script 
attached):

master (213c959a29):        8.0 s
streaming-api v13:        9.5 s

This test exercises just the ReadBuffer() codepath, to check if there is 
a regression there. It does not exercise the new streaming APIs.

So looks like the streaming API patches add some overhead to the simple 
non-streaming ReadBuffer() case. This is a highly narrow 
micro-benchmark, of course, so even though this is a very 
performance-sensitive codepath, we could perhaps accept a small 
regression there. In any real workload, you'd at least need to take the 
buffer lock and read something from the page.

But can we do better? Aside from performance, I was never quite happy 
with the BMR_REL/BMR_SMGR stuff we introduced in PG v16. I like having 
one common struct like BufferManagerRelation that is used in all the 
functions, instead of having separate Relation and SMgrRelation variants 
of every function. But those macros feel a bit hacky and we are not 
consistently using them in all the functions. Why is there no 
ReadBuffer() variant that takes a BufferManagerRelation?

The attached patch expands the use of BufferManagerRelations. The 
principle now is that before calling any bufmgr function, you first 
initialize a BufferManagerRelation struct, and pass that to the 
function. The initialization is done by the InitBMRForRel() or 
InitBMRForSmgr() function, which replace the BMR_REL/BMR_SMGR macros. 
They are full-blown functions now because they do more setup upfront 
than BMR_REL/BMR_SMGR. For example, InitBMRForRel() always initializes 
the 'smgr' field, so that you don't need to repeat this pattern in all 
the other functions:

-       /* Make sure our bmr's smgr and persistent are populated. */
-       if (bmr.smgr == NULL)
-       {
-               bmr.smgr = RelationGetSmgr(bmr.rel);
-               bmr.relpersistence = bmr.rel->rd_rel->relpersistence;
-       }

Initializing the BufferManagerRelation is still pretty cheap, so it's 
feasible to call it separately for every ReadBuffer() call. But you can 
also reuse it across calls, if you read multiple pages in a loop, for 
example. That saves a few cycles.

The microbenchmark results with these changes:

master (213c959a29):        8.0 s
streaming-api v13:        9.5 s
bmr-refactor            8.4 s
bmr-refactor, InitBMR once    7.7 s

The difference between the "bmr-refactor" and "initBMR once" is that in 
the "initBMR once" test, I modified the benchmark to call 
InitBMRForRel() just once, outside the loop. So that shows the benefit 
of reusing the BufferManagerRelation. This refactoring seems to make 
performance regression smaller, even if you don't take advantage of 
reusing the BufferManagerRelation.

This also moves things around a little in ReadBuffer_common() (now 
called ReadBufferBMR). Instead of calling StartReadBuffer(), it calls 
PinBufferForBlock() directly. I tried doing that before the other 
refactorings, but that alone didn't seem to make much difference. Not 
sure if it's needed, it's perhaps an orthogonal refactoring, but it's 
included here nevertheless.

What do you think? The first three attached patches are your v13 patches 
unchanged. The fourth is the micro-benchmark I used. The last patch is 
the interesting one.


PS. To be clear, I'm happy with your v13 streaming patch set as it is. I 
don't think this BufferManagerRelation refactoring is a show-stopper.

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