Re: Some read stream improvements

Hi,

On 2025-03-14 22:03:15 +1300, Thomas Munro wrote:
> I have pushed the new pin limit patches, after some more testing and
> copy editing. I dropped an unnecessary hunk (in read_stream_reset(), a
> change I'd like to make but it didn't belong in this commit) and
> dropped the word "Soft" from GetSoftPinLimit() as it wasn't helping
> comprehension and isn't even true for the local buffer version (which
> I still think might be an issue, will come back to that, but again it
> seemed independent).

I found an, easy to fix, issue in read_stream.c. If the limit returned by
GetAdditionalPinLimit() is very large, the buffer_limit variable in
read_stream_start_pending_read() can overflow.

While the code is careful to limit buffer_limit PG_INT16_MAX, we subsequently
add the number of forwarded buffers:

if (stream->temporary)
buffer_limit = Min(GetAdditionalLocalPinLimit(), PG_INT16_MAX);
else
buffer_limit = Min(GetAdditionalPinLimit(), PG_INT16_MAX);
Assert(stream->forwarded_buffers <= stream->pending_read_nblocks);
buffer_limit += stream->forwarded_buffers;

I think there's a second, theoretical, overflow issue shortly after:

/* Shrink distance: no more look-ahead until buffers are released. */
new_distance = stream->pinned_buffers + buffer_limit;
if (stream->distance > new_distance)
stream->distance = new_distance;

while that was hit in the case I was looking at, it was only hit because
buffer_limit had already wrapped around into the negative. I don't think
nblocks can be big enough to really hit this.

I don't actually see any reason for buffer_limit to be a 16bit quantity? It's
just to clamp things down, right?

Greetings,

Andres Freund