Re: BUG #19006: Assert(BufferIsPinned) in BufferGetBlockNumber() is triggered for forwarded buffer

On Thu, Dec 11, 2025 at 5:16 PM Xuneng Zhou <xunengzhou(at)gmail(dot)com> wrote:
> I've reviewed patch v2 carefully and found no correctness problems.
> Although the performance benefit it brings might be intangible, the
> new interface is more robust and simpler than the current one. It does
> save a lot of bookkeeping:

Thanks Xuneng for all your work reviewing and testing this back in December.

Unfortunately this fell through the cracks (sorry) and I didn't push
it before the freeze. Any objections to pushing it now? I can live
with deferring it until master reopens if that's the call (CC RMT),
but it would be nice to tidy up this design wart if we can.

A short restatement of the situation (but see earlier if you want a
... really long one):

* it's already the case that the StartReadBuffers() sometimes stores
and "forwards" already-pinned buffers in its in/out buffers[] argument
* that currently means the caller has to fill it with InvalidBuffer so
they can be distinguished
* it forwards buffers when it decides to boot them out of the current
IO operation and into the next one (that it assumes the caller will
usually make)
* an example reason to split is when an overlapping already
in-progress IO is encountered after it has the pins
* in the current coding, read_stream.c has to initialise its internal
queue with InvalidBuffer and clear them after use, and count them
after each calls
* in the 18 beta phase a bug in an edge case of that accounting was
fixed by b4212231

This patch shifts some of that responsibility to a more natural place:

* it now seems obvious that StartReadBuffers() should just allow an
in/out npinned counter to travel along with the in/out buffers array
* read_stream.c still needs to know how many there are for pin limit purposes
* it also needs to know in the unusual case that the stream ends
earlier and it has to unpin them
* other than that, it's StartReadBuffers()'s private business to manage them
* StartReadBuffers() can do that with trivial arithmetic, no need to
distinguish and count the buffers
* the end result is much simpler and more robust

(Heikki may recall that we wrestled with versions of this
how-to-avoid-unpinning-across-split-IOs question back in the v17 cycle
when he was reviewing the first vectored I/O + read_stream.c patches,
and there were "two phase" and the "three phase" API ideas, which
eventually came back as "forwarded buffers" in v18, and this would be
a simplification for v19, "forwarded buffers + a counter".)