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

Hi,

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:

1) Initialize slots to InvalidBuffer: Before calling StartReadBuffers,
it had to zero out buffer slots:
while (stream->initialized_buffers < buffer_index + nblocks)
    stream->buffers[stream->initialized_buffers++] = InvalidBuffer;

2) Scan for forwarded buffers: After the call, it had to scan the
array to count how many buffers were forwarded:
forwarded = 0;
while (nblocks + forwarded < requested_nblocks &&
       stream->buffers[buffer_index + nblocks + forwarded] != InvalidBuffer)
    forwarded++;
stream->forwarded_buffers = forwarded;

3) Clear consumed slots: When returning a buffer to the consumer, it
had to clear the slot:
stream->buffers[oldest_buffer_index] = InvalidBuffer;

This could be fragile because:

- read_stream.c must maintain the invariant that unused slots are InvalidBuffer.
- The scanning loop is O(n) and "layer-violating" (it's re-discovering
information that StartReadBuffers already knew).
- Missing a = InvalidBuffer assignment anywhere would cause silent corruption.

With the new interface -- Make npinned an explicit in/out parameter.

1) No initialization needed: StartReadBuffers doesn't look at
uninitialized slots. It only reads buffers[0..npinned-1].

2) No scanning needed: The count is returned directly: *npinned =
actual_npinned - *nblocks;  // Buffer manager tells you the count

3) No clearing needed: The "is it forwarded?" check is now i <
actual_npinned, not buffers[i] != InvalidBuffer.

As for the xxx questions, here're some thoughts:

XXX is the single-buffer specialization unaffected due to dead code
elimination, as expected?
- The single-buffer specialization stays unchanged: StartReadBuffer()
still passes npinned=0 with allow_forwarding=false, so the extra
parameter is dead code there and the compiler will inline/strip it
just as before.

XXX unused queue entries could potentially be electrified with
wipe_mem/VALGRIND_MAKE_MEM_NOACCESS, though existing bufmgr.c
assertions are very likely to reveal any fencepost bugs already
- The queue-entry sanitization step removed should not affect
behavior, since StartReadBuffers() now relies on npinned rather than
InvalidBuffer sentinels. Leaving stale values in the array is likely
harmless. If desired, using wipe_mem or VALGRIND_MAKE_MEM_NOACCESS
would simply provide additional debugging hardening.

XXX perhaps we don't need quite so many _npinned <= _nblocks assertions
- I think this makes sense. In my experience, reading through the
read_stream module already requires a fair amount of effort and focus.
There are many edge cases to watch, and the complexity is amplified by
managing two arrays, pointer arithmetic across function calls, and
numerous assertions to validate those operations. Removing assertions
that are not strictly necessary can make the code more readable and
easier to follow, without weakening its defensive properties.

// In read_stream_start_pending_read():
// REMOVE this pre-call assertion:
- Assert(stream->pending_read_npinned <= stream->pending_read_nblocks);

// Entry in StartReadBuffersImpl (bufmgr.c):
- Assert(*npinned <= *nblocks);

// KEEP this post-call assertion:
  Assert(stream->pending_read_nblocks >= stream->pending_read_npinned);

The pre-call assertion in read_stream_start_pending_read looks less
needed since the entry assertion in StartReadBuffersImpl catches
exactly a subset of bugs that the entry assertion catches, with no
intervening code that could obscure the failure or change program
state.


--
Best,
Xuneng