On 11/01/2024 05:19, Thomas Munro wrote:
On Thu, Jan 11, 2024 at 8:58 AM Heikki Linnakangas <hlinn...@iki.fi> wrote:
On 10/01/2024 06:13, Thomas Munro wrote:
Bikeshedding call: I am open to better suggestions for the names
PrepareReadBuffer() and CompleteReadBuffers(), they seem a little
grammatically clumsy.
How will these functions work in the brave new async I/O world? I assume
PrepareReadBuffer() will initiate the async I/O, and
CompleteReadBuffers() will wait for it to complete. How about
StartReadBuffer() and WaitReadBuffer()? Or StartBufferRead() and
WaitBufferRead()?
What we have imagined so far is that the asynchronous version would
probably have three steps, like this:
* PrepareReadBuffer() -> pins one buffer, reports if found or IO/zeroing
needed
* StartReadBuffers() -> starts the I/O for n contiguous !found buffers
* CompleteReadBuffers() -> waits, completes as necessary
Ok. It feels surprising to have three steps. I understand that you need
two steps, one to start the I/O and another to wait for them to finish,
but why do you need separate Prepare and Start steps? What can you do in
between them? (You explained that. I'm just saying that that's my
initial reaction when seeing that API. It is surprising.)
If StartReadBuffer() starts the async I/O, the idea that you can call
ZeroBuffer() instead of WaitReadBuffer() doesn't work. I think
StartReadBuffer() needs to take ReadBufferMode, and do the zeroing for
you in RBM_ZERO_* modes.
Yeah, good thoughts, and topics that have occupied me for some time
now. I also thought that StartReadBuffer() should take
ReadBufferMode, but I came to the idea that it probably shouldn't like
this:
I started working on all this by trying to implement the most
complicated case I could imagine, streaming recovery, and then working
back to the easy cases that just do scans with RBM_NORMAL. In
recovery, we can predict that a block will be zeroed using WAL flags,
and pre-existing cross-checks at redo time that enforce that the flags
and redo code definitely agree on that, but we can't predict which
exact ReadBufferMode the redo code will use, RBM_ZERO_AND_LOCK or
RBM_ZERO_AND_CLEANUP_LOCK (or mode=RBM_NORMAL and
get_cleanup_lock=true, as the comment warns them not to, but I
digress).
That's OK, because we can't take locks while looking ahead in recovery
anyway (the redo routine carefully controls lock order/protocol), so
the code to actually do the locking needs to be somewhere near the
output end of the stream when the redo code calls
XLogReadBufferForRedoExtended(). But if you try to use RBM_XXX in
these interfaces, it begins to look pretty funny: the streaming
callback needs to be able to say which ReadBufferMode, but anywhere
near Prepare*(), Start*() or even Complete*() is too soon, so maybe we
need to invent a new value RBM_WILL_ZERO that doesn't yet say which of
the zero modes to use, and then the redo routine needs to pass in the
RBM_ZERO_AND_{LOCK,CLEANUP_LOCK} value to
XLogReadBufferForRedoExtended() and it does it in a separate step
anyway, so we are ignoring ReadBufferMode. But that feels just wrong
-- we'd be using RBM but implementing them only partially.
I see. When you're about to zero the page, there's not much point in
splitting the operation into Prepare/Start/Complete stages anyway.
You're not actually doing any I/O. Perhaps it's best to have a separate
"Buffer ZeroBuffer(Relation, ForkNumber, BlockNumber, lockmode)"
function that does the same as
ReadBuffer(RBM_ZERO_AND_[LOCK|CLEANUP_LOCK]) today.
The _ZERO_ON_ERROR aspect is a case where CompleteReadBuffers() is the
right time and makes sense to process as a batch, so it becomes a
flag.
+1
Putting all that together, I propose:
/*
* Initiate reading a block from disk to the buffer cache.
*
* XXX: Until we have async I/O, this just allocates the buffer in the
buffer
* cache. The actual I/O happens in WaitReadBuffer().
*/
Buffer
StartReadBuffer(BufferManagerRelation bmr,
ForkNumber forkNum,
BlockNumber blockNum,
BufferAccessStrategy strategy,
ReadBufferMode mode,
bool *foundPtr);
/*
* Wait for a read that was started earlier with StartReadBuffer() to
finish.
*
* XXX: Until we have async I/O, this is the function that actually
performs
* the I/O. If multiple I/Os have been started with StartReadBuffer(), this
* will try to perform all of them in one syscall. Subsequent calls to
* WaitReadBuffer(), for those other buffers, will finish quickly.
*/
void
WaitReadBuffer(Buffer buf);
I'm confused about where the extra state lives that would allow the
communication required to build a larger I/O. In the AIO branch, it
does look a little more like that, but there is more magic state and
machinery hiding behind the curtain: the backend's pending I/O list
builds up a chain of I/Os, and when you try to wait, if it hasn't
already been submitted to the kernel/bgworkers yet it will be, and
before that merging will happen. So you get bigger I/Os without
having to say so explicitly.
Yeah, I was thinking that there would be a global variable that holds a
list of operations started with StartReadBuffer().
For this synchronous version (and hopefully soon a more efficient
improved version in the AIO branch), we want to take advantage of the
client's pre-existing and more cheaply obtained knowledge of ranges.
Ok.
In an asynchronous version, that BM_IO_IN_PROGRESS negotiation would
take place in StartReadBuffers() instead, which would be responsible
for kicking off asynchronous I/Os (= asking a background worker to
call pread(), or equivalent fancy kernel async APIs). One question is
what it does if it finds a block in the middle that chops the read up,
or for that matter a segment boundary. I don't think we have to
decide now, but the two options seem to be that it starts one single
I/O and reports its size, making it the client's problem to call again
with the rest, or that it starts more than one I/O and they are
somehow chained together so that the caller doesn't know about that
and can later wait for all of them to finish using just one <handle
thing>.
(The reason why I'm not 100% certain how it will look is that the
real, working code in the aio branch right now doesn't actually expose
a vector/nblocks bufmgr interface at all, yet. Andres's original
prototype had a single-block Start*(), Complete*() design, but a lower
level of the AIO system notices if pending read operations are
adjacent and could be merged. While discussing all this we decided it
was a bit strange to have lower code deal with allocating, chaining
and processing lots of separate I/O objects in shared memory, when
higher level code could often work in bigger ranges up front, and then
interact with the AIO subsystem with many fewer objects and steps.
Also, the present simple and lightweight synchronous proposal that
lacks the whole subsystem that could do that by magic.)
Hmm, let's sketch out what this would look like with the approach that
you always start one I/O and report the size:
/*
* Initiate reading a range of blocks from disk to the buffer cache.
*
* If the pages were already found in the buffer cache, returns true.
* Otherwise false, and the caller must call WaitReadBufferRange() to
* wait for the I/O to finish, before accessing the buffers.
*
* 'buffers' is a caller-supplied array large enough to hold (endBlk -
* startBlk) buffers. It is filled with the buffers that the pages are
* read into.
*
* This always starts a read of at least one block, and tries to
* initiate one read I/O for the whole range if possible. But if the
* read cannot be performed as a single I/O, a partial read is started,
* and *endBlk is updated to reflect the range for which the read was
* started. The caller can make another call to read the rest of the
* range. A partial read can occur if some, but not all, of the pages
* are already in the buffer cache, or because the range crosses a
* segment boundary.
*
* XXX: Until we have async I/O, this just allocates the buffers in the
* buffer cache. And perhaps calls smgrprefetch(). The actual I/O
* happens in WaitReadBufferRange().
*/
bool
StartReadBufferRange(BufferManagerRelation bmr,
ForkNumber forkNum,
BlockNumber startBlk,
BlockNumber *endBlk,
BufferAccessStrategy strategy,
Buffer *buffers);
/*
* Wait for a read that was started earlier with StartReadBufferRange()
* to finish.
*
* XXX: Until we have async I/O, this is the function that actually
* performs
* the I/O. StartReadBufferRange already checked that the pages can be
* read in one preadv() syscall. However, it's possible that another
* backend performed the read for some of the pages in between. In that
* case this will perform multiple syscalls, after all.
*/
void
WaitReadBufferRange(Buffer *buffers, int nbuffers, bool zero_on_error);
/*
* Allocates a buffer for the given page in the buffer cache, and locks
* the page. No I/O is initiated. The caller must initialize it and mark
* the buffer dirty before releasing the lock.
*
* This is equivalent to ReadBuffer(RBM_ZERO_AND_LOCK) or
* ReadBuffer(RBM_ZERO_AND_CLEANUP_LOCK).
*/
Buffer
ZeroBuffer(BufferManagerRelation bmr,
ForkNumber forkNum,
BlockNumber blockNum,
BufferAccessStrategy strategy,
bool cleanup_lock);
This range-oriented API fits the callers pretty well: the streaming read
API works with block ranges already. There is no need for separate
Prepare and Start steps.
One weakness is that if StartReadBufferRange() finds that the range is
"chopped up", it needs to return and throw away the work it had to do to
look up the next buffer. So in the extreme case that every other block
in the range is in the buffer cache, each call would look up two buffers
in the buffer cache, startBlk and startBlk + 1, but only return one
buffer to the caller.
--
Heikki Linnakangas
Neon (https://neon.tech)
