| 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 |
| 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 |