Convert varatt.h macros to static inline functions

Peter Eisentraut <peter@eisentraut.org> writes:

I had this lying around as a draft patch, as part of my ongoing campaign
to convert many complicated macros to static inline functions. Since
the topic was mentioned in another thread [0], I cleaned up the patch so
that we can all look at it.

I had just finished doing exactly that, per my idea in the other
thread of providing both pointer and Datum variants where needed.
I'll look more closely at yours in a bit, but here's mine.

regards, tom lane

On Jul 31, 2025, at 10:06 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Peter Eisentraut <peter@eisentraut.org> writes:

I had this lying around as a draft patch, as part of my ongoing campaign 
to convert many complicated macros to static inline functions.  Since 
the topic was mentioned in another thread [0], I cleaned up the patch so 
that we can all look at it.

I had just finished doing exactly that, per my idea in the other
thread of providing both pointer and Datum variants where needed.
I'll look more closely at yours in a bit, but here's mine.

regards, tom lane

Thank you both for diving into this, I really appreciate the clarity
this brings to the code.  I've applied both patches and compared the two
approaches.  I build and tested Peter's patches on macOS (15.5, M3 Pro)
using meson and found no issues.  It's clear you both had the same idea
in mind, but with slight differences in application.

I find VARDATA(DatumGetPointer(key_datums[I])) to be more clear
than VARDATA_D(key_datums[I]).  Yes, the line length suffers a bit but I
find it much more readable.

I'm +1 for changes like this, and this one in particular.

Just my $0.02, best.

-greg

On Thu, Jul 31, 2025 at 10:06:02AM -0400, Tom Lane wrote:

Peter Eisentraut <peter@eisentraut.org> writes:

I had this lying around as a draft patch, as part of my ongoing campaign
to convert many complicated macros to static inline functions. Since
the topic was mentioned in another thread [0], I cleaned up the patch so
that we can all look at it.

I had just finished doing exactly that, per my idea in the other
thread of providing both pointer and Datum variants where needed.
I'll look more closely at yours in a bit, but here's mine.

I'm finding that your wip version is bit more complete as you have
introduced inline functions for these ones:
- VARATT_EXTERNAL_GET_EXTSIZE()
- VARATT_EXTERNAL_GET_COMPRESS_METHOD().

Another comment that can apply to all the patches presented on this
thread. Could it be worth splitting these inline functions into a
separate header that declares varatt.h, meaning that we'd need to
think a bit more about the structures themselves and all the
sub-macros like SET_VARSIZE_1B() & friends? There is a bit of chaos
that has accumulated in varatt.h over the ages, still there is a
hierarchy with the most "internal" structures and what gets used by
the code C code. So we could use this occasion to split things if
that brings some clarity..
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

Another comment that can apply to all the patches presented on this
thread. Could it be worth splitting these inline functions into a
separate header that declares varatt.h, meaning that we'd need to
think a bit more about the structures themselves and all the
sub-macros like SET_VARSIZE_1B() & friends?

IIRC we already moved these macros out of postgres.h, some years ago.

I don't really see how we could hide the lower-level macros from
callers' eyes while still having them available to inline functions.
The point of this exercise IMO is not information hiding: it is to
be clearer about the input and output datatypes of the functions,
and to eliminate multiple-evaluation hazards.

regards, tom lane

It looks like the majority vote is still in favor of writing out
DatumGetPointer instead of using "_D()" functions, so let's roll
with that approach.

I looked through our two versions of the varatt.h changes and
merged them. The attached is only cosmetically different from
yours, I think --- mostly, I kept the comments I'd written.

I've tested this atop 0001-0005 from [1]/messages/by-id/8246d7ff-f4b7-4363-913e-827dadfeb145@eisentraut.org, and it all seems good.
I'd like to move along with getting these changes committed, and
then I'll take another look at the 8-byte-datums-everywhere proposal.

regards, tom lane

[1]: /messages/by-id/8246d7ff-f4b7-4363-913e-827dadfeb145@eisentraut.org

On 03.08.25 22:20, Tom Lane wrote:

It looks like the majority vote is still in favor of writing out
DatumGetPointer instead of using "_D()" functions, so let's roll
with that approach.

I looked through our two versions of the varatt.h changes and
merged them. The attached is only cosmetically different from
yours, I think --- mostly, I kept the comments I'd written.

I've tested this atop 0001-0005 from [1], and it all seems good.
I'd like to move along with getting these changes committed, and
then I'll take another look at the 8-byte-datums-everywhere proposal.

I committed this with the required prerequisite patches. That concludes
this thread, I think. I'll follow up on the remaining work in the
"Datum as struct" thread, and the work in the "8 byte Datums" thread can
also continue.

Peter Eisentraut <peter@eisentraut.org> writes:

I committed this with the required prerequisite patches. That concludes
this thread, I think. I'll follow up on the remaining work in the
"Datum as struct" thread, and the work in the "8 byte Datums" thread can
also continue.

Thanks! The "8 byte Datums" work is still blocked on the remaining
two preliminary patches from "Datum as struct", though.

regards, tom lane

Hi,

On Tue, Aug 5, 2025 at 9:42 AM Peter Eisentraut <peter@eisentraut.org> wrote:

On 03.08.25 22:20, Tom Lane wrote:

It looks like the majority vote is still in favor of writing out
DatumGetPointer instead of using "_D()" functions, so let's roll
with that approach.

I looked through our two versions of the varatt.h changes and
merged them. The attached is only cosmetically different from
yours, I think --- mostly, I kept the comments I'd written.

I've tested this atop 0001-0005 from [1], and it all seems good.
I'd like to move along with getting these changes committed, and
then I'll take another look at the 8-byte-datums-everywhere proposal.

I committed this with the required prerequisite patches. That concludes
this thread, I think. I'll follow up on the remaining work in the
"Datum as struct" thread, and the work in the "8 byte Datums" thread can
also continue.

I got the following compiler warning:

% make -C src/backend/storage/large_object
inv_api.c: In function ‘inv_write’:
inv_api.c:565:29: warning: ‘workbuf’ may be used uninitialized
[-Wmaybe-uninitialized]
565 | char *workb = VARDATA(&workbuf.hdr);
| ^~~~~~~~~~~~~~~~~~~~~
In file included from ../../../../src/include/access/htup_details.h:22,
from ../../../../src/include/nodes/tidbitmap.h:25,
from ../../../../src/include/access/genam.h:20,
from inv_api.c:36:
../../../../src/include/varatt.h:305:1: note: by argument 1 of type
‘const void *’ to ‘VARDATA’ declared here
305 | VARDATA(const void *PTR)
| ^~~~~~~
inv_api.c:564:33: note: ‘workbuf’ declared here
564 | } workbuf;
| ^~~~~~~
inv_api.c: In function ‘inv_truncate’:
inv_api.c:756:29: warning: ‘workbuf’ may be used uninitialized
[-Wmaybe-uninitialized]
756 | char *workb = VARDATA(&workbuf.hdr);
| ^~~~~~~~~~~~~~~~~~~~~
../../../../src/include/varatt.h:305:1: note: by argument 1 of type
‘const void *’ to ‘VARDATA’ declared here
305 | VARDATA(const void *PTR)
| ^~~~~~~
inv_api.c:755:33: note: ‘workbuf’ declared here
755 | } workbuf;
| ^~~~~~~

I've not fully investigated the root cause but commit e035863c9a0
presumably is the culprit. FYI I'm using gcc 14.2.1.

The attached patch fixes the warning.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Masahiko Sawada <sawada.mshk@gmail.com> writes:

I got the following compiler warning:

% make -C src/backend/storage/large_object
inv_api.c: In function ‘inv_write’:
inv_api.c:565:29: warning: ‘workbuf’ may be used uninitialized
[-Wmaybe-uninitialized]
565 | char *workb = VARDATA(&workbuf.hdr);
| ^~~~~~~~~~~~~~~~~~~~~

I've not fully investigated the root cause but commit e035863c9a0
presumably is the culprit. FYI I'm using gcc 14.2.1.

Interesting. I did not see such warnings with gcc 14.3.1, 15.1.1,
nor older gcc versions. Must be something peculiar to 14.2.

The attached patch fixes the warning.

Theoretically this shouldn't be necessary, since VARDATA()
does not touch the contents of the pointed-to object.
Still, no objection.

regards, tom lane

On Tue, Aug 5, 2025 at 11:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Masahiko Sawada <sawada.mshk@gmail.com> writes:

I got the following compiler warning:

% make -C src/backend/storage/large_object
inv_api.c: In function ‘inv_write’:
inv_api.c:565:29: warning: ‘workbuf’ may be used uninitialized
[-Wmaybe-uninitialized]
565 | char *workb = VARDATA(&workbuf.hdr);
| ^~~~~~~~~~~~~~~~~~~~~

I've not fully investigated the root cause but commit e035863c9a0
presumably is the culprit. FYI I'm using gcc 14.2.1.

Interesting. I did not see such warnings with gcc 14.3.1, 15.1.1,
nor older gcc versions. Must be something peculiar to 14.2.

Hmm, I got the same warning with 14.3.1 (exact version shown below) so
probably something is strange on my end:

% gcc --version
gcc (GCC) 14.3.1 20250805

Theoretically this shouldn't be necessary, since VARDATA()
does not touch the contents of the pointed-to object.

Right. I'll do more research.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Masahiko Sawada <sawada.mshk@gmail.com> writes:

On Tue, Aug 5, 2025 at 11:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Interesting. I did not see such warnings with gcc 14.3.1, 15.1.1,
nor older gcc versions. Must be something peculiar to 14.2.

Hmm, I got the same warning with 14.3.1 (exact version shown below) so
probably something is strange on my end:

% gcc --version
gcc (GCC) 14.3.1 20250805

That's even more interesting. The specific late-model gcc versions
I checked were from Fedora 41:

$ gcc --version
gcc (GCC) 14.3.1 20250523 (Red Hat 14.3.1-1)

and Fedora 42:

gcc (GCC) 15.1.1 20250521 (Red Hat 15.1.1-2)

Maybe there's some strange cross-distro difference here, but
what I'm wondering is if there's a difference in CFLAGS.
My build used

CFLAGS = -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type -Wshadow=compatible-local -Wformat-security -Wmissing-variable-declarations -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wno-format-truncation -Wno-stringop-truncation -g -O2

regards, tom lane

On Tue, Aug 5, 2025 at 1:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Masahiko Sawada <sawada.mshk@gmail.com> writes:

On Tue, Aug 5, 2025 at 11:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Interesting. I did not see such warnings with gcc 14.3.1, 15.1.1,
nor older gcc versions. Must be something peculiar to 14.2.

Hmm, I got the same warning with 14.3.1 (exact version shown below) so
probably something is strange on my end:

% gcc --version
gcc (GCC) 14.3.1 20250805

That's even more interesting. The specific late-model gcc versions
I checked were from Fedora 41:

$ gcc --version
gcc (GCC) 14.3.1 20250523 (Red Hat 14.3.1-1)

and Fedora 42:

gcc (GCC) 15.1.1 20250521 (Red Hat 15.1.1-2)

Maybe there's some strange cross-distro difference here, but
what I'm wondering is if there's a difference in CFLAGS.
My build used

CFLAGS = -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type -Wshadow=compatible-local -Wformat-security -Wmissing-variable-declarations -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wno-format-truncation -Wno-stringop-truncation -g -O2

Yeah, interestingly I didn't see the warning with CFLAGS your build
used but got it if I use -O0 instead of -O2.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Masahiko Sawada <sawada.mshk@gmail.com> writes:

On Tue, Aug 5, 2025 at 1:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Maybe there's some strange cross-distro difference here, but
what I'm wondering is if there's a difference in CFLAGS.
My build used

CFLAGS = -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type -Wshadow=compatible-local -Wformat-security -Wmissing-variable-declarations -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wno-format-truncation -Wno-stringop-truncation -g -O2

Yeah, interestingly I didn't see the warning with CFLAGS your build
used but got it if I use -O0 instead of -O2.

I checked the buildfarm, and (so far) adder and flaviventris have
shown this warning, but nothing else has. adder is using gcc 14.2.0
with -O0, while flaviventris is using gcc 16.0.0 with -O0. Also
I tried -O0 with gcc 15.1.1 on my Fedora 42 box, and now it shows the
warning. So maybe the difference is just -O0? But I think there are
other buildfarm animals using that, so I'm not certain we've explained
the difference fully.

Anyway, based on that I think there's enough reason to go ahead
with your patch.

regards, tom lane

On Tue, Aug 5, 2025 at 2:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Masahiko Sawada <sawada.mshk@gmail.com> writes:

On Tue, Aug 5, 2025 at 1:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Maybe there's some strange cross-distro difference here, but
what I'm wondering is if there's a difference in CFLAGS.
My build used

CFLAGS = -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type -Wshadow=compatible-local -Wformat-security -Wmissing-variable-declarations -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wno-format-truncation -Wno-stringop-truncation -g -O2

Yeah, interestingly I didn't see the warning with CFLAGS your build
used but got it if I use -O0 instead of -O2.

I checked the buildfarm, and (so far) adder and flaviventris have
shown this warning, but nothing else has. adder is using gcc 14.2.0
with -O0, while flaviventris is using gcc 16.0.0 with -O0.

Indeed. Thank you for checking.

Also
I tried -O0 with gcc 15.1.1 on my Fedora 42 box, and now it shows the
warning. So maybe the difference is just -O0? But I think there are
other buildfarm animals using that, so I'm not certain we've explained
the difference fully.

Anyway, based on that I think there's enough reason to go ahead
with your patch.

Agreed. I've attached the patch. I'll push it, barring comments.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Tue, Aug 5, 2025 at 3:07 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Aug 5, 2025 at 2:39 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Masahiko Sawada <sawada.mshk@gmail.com> writes:

On Tue, Aug 5, 2025 at 1:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Maybe there's some strange cross-distro difference here, but
what I'm wondering is if there's a difference in CFLAGS.
My build used

CFLAGS = -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type -Wshadow=compatible-local -Wformat-security -Wmissing-variable-declarations -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wno-format-truncation -Wno-stringop-truncation -g -O2

Yeah, interestingly I didn't see the warning with CFLAGS your build
used but got it if I use -O0 instead of -O2.

I checked the buildfarm, and (so far) adder and flaviventris have
shown this warning, but nothing else has. adder is using gcc 14.2.0
with -O0, while flaviventris is using gcc 16.0.0 with -O0.

Indeed. Thank you for checking.

Also
I tried -O0 with gcc 15.1.1 on my Fedora 42 box, and now it shows the
warning. So maybe the difference is just -O0? But I think there are
other buildfarm animals using that, so I'm not certain we've explained
the difference fully.

Anyway, based on that I think there's enough reason to go ahead
with your patch.

Agreed. I've attached the patch. I'll push it, barring comments.

Pushed.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com