Re: Beautify read stream "per buffer data" APIs

From: Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com>
To: Melanie Plageman <melanieplageman(at)gmail(dot)com>
Cc: PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
Subject: Re: Beautify read stream "per buffer data" APIs
Date: 2026-04-01 08:02:48
Message-ID: CAN55FZ3EpkW4G_ein9XzHJV4Okek4Uv1DsK+xGdXMLdYZqbkpg@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

Thank you for looking into this!

On Tue, 31 Mar 2026 at 18:47, Melanie Plageman
<melanieplageman(at)gmail(dot)com> wrote:
>
> Thanks for rebasing, putting this together, and extracting from that
> other length thread!
>
> On Fri, Mar 27, 2026 at 8:02 AM Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com> wrote:
> >
> > The read_stream_put_value() macro doesn't accept literal constant
> > values, we need to pass a variable to it. Otherwise, the compilation
> > fails with:
> >
> > ```
> > ../../postgres/src/include/storage/read_stream.h:169:36: error: lvalue
> > required as unary ‘&’ operand
> > 169 | memcpy((per_buffer_data), &(value), sizeof(value)))
> > | ^
> > ../../postgres/src/backend/access/heap/vacuumlazy.c:1703:17: note: in
> > expansion of macro ‘read_stream_put_value’
> > 1703 | read_stream_put_value(stream, per_buffer_data, false);
> > ```
> >
> > If that is not intentional, I think it would be better if we can
> > convert read_stream_put_value() to a way that it accepts rvalues.
>
> I agree this would be much better from an API usability standpoint.
> I'm not sure we can do this in a way people would find acceptable in
> Postgres code, though. Apparently this works:
>
> read_stream_put_value(stream, per_buffer_data, (bool){false});
>
> but it is a little weird.

Yes, I don't recall seeing this usage elsewhere in the Postgres code.

> As we discussed off-list, I think the BitmapHeapScanNextBlock()
> read_stream_get_buffer_and_pointer() transition should be moved from
> 0002 to 0001 for clarity.

Fixed.

> I think this is unequivocally an improvement in the API. But it
> probably needs more time to bake now that you've started a new thread.
> It only helps in development, so we can commit it early in 20 and
> still get much of the same benefit. External users would have to wait.
> But it is probably better to wait than to have an API in 19 that
> changes again in 20.

I agree. These changes improve the type safety of read_stream_* calls,
but existing read stream users are not currently experiencing issues.

--
Regards,
Nazir Bilal Yavuz
Microsoft

Attachment Content-Type Size
v4-0001-Improve-API-for-retrieving-data-from-read-streams.patch text/x-patch 19.8 KB
v4-0002-Improve-API-for-storing-data-in-read-streams.patch text/x-patch 3.3 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Chao Li 2026-04-01 08:03:48 Re: ALTER TABLE: warn when actions do not recurse to partitions
Previous Message Chao Li 2026-04-01 08:01:56 Re: ALTER TABLE: warn when actions do not recurse to partitions