Re: Some read stream improvements - Mailing list pgsql-hackers

From Andres Freund
Subject Re: Some read stream improvements
Date
Msg-id ewvz3cbtlhrwqk7h6ca6cctiqh7r64ol3pzb3iyjycn2r5nxk5@tnhw3a5zatlr
Whole thread Raw
In response to Re: Some read stream improvements  (Thomas Munro <thomas.munro@gmail.com>)
Responses Re: Some read stream improvements
List pgsql-hackers
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



pgsql-hackers by date:

Previous
From: Masahiko Sawada
Date:
Subject: Re: Restrict copying of invalidated replication slots
Next
From: David Rowley
Date:
Subject: Re: BTScanOpaqueData size slows down tests