Horton

Thread

Commits

  1. Suppress maybe-uninitialized warning.

  2. Convert varatt.h access macros to static inline functions.

  3. Fix varatt versus Datum type confusions

  1. Convert varatt.h macros to static inline functions

    Peter Eisentraut <peter@eisentraut.org> — 2025-07-31T13:08:13Z

    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.

The titular change is to convert the macros in varatt.h to inline 
functions, so they are easier to read and use.  I only touched the ones 
for external use, not the internal ones, mainly because I don't have a 
way to test a big-endian build.

Part of the change is also figuring out exactly what the argument and 
return types should be.  In many cases, there were some inconsistencies, 
because the macros would just cast anything you give them into the shape 
they want, no matter whether it makes sense.  In particular, the callers 
were inconsistent about whether macros like VARDATA() and VARSIZE() 
should take struct varlena or Datum, or I guess both.  I cleaned this up 
by adding the required DatumGetPointer() calls in the first patch.  The 
thread [0] is now discussing some other ideas, but this is what I had, 
and this way it's at least consistent with other existing code.


[0]: 
https://www.postgresql.org/message-id/flat/1749799.1752797397%40sss.pgh.pa.us

  2. Re: Convert varatt.h macros to static inline functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-07-31T14:06:02Z

    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

  3. Re: Convert varatt.h macros to static inline functions

    Greg Burd <greg@burd.me> — 2025-07-31T14:44:22Z

    > 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

  4. Re: Convert varatt.h macros to static inline functions

    Michael Paquier <michael@paquier.xyz> — 2025-07-31T23:40:33Z

    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

  5. Re: Convert varatt.h macros to static inline functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-08-01T05:07:33Z

    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

  6. Re: Convert varatt.h macros to static inline functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-08-03T20:20:31Z

    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.

			regards, tom lane

[1] https://www.postgresql.org/message-id/8246d7ff-f4b7-4363-913e-827dadfeb145%40eisentraut.org

  7. Re: Convert varatt.h macros to static inline functions

    Peter Eisentraut <peter@eisentraut.org> — 2025-08-05T16:41:51Z

    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.

  8. Re: Convert varatt.h macros to static inline functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-08-05T16:48:29Z

    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

  9. Re: Convert varatt.h macros to static inline functions

    Masahiko Sawada <sawada.mshk@gmail.com> — 2025-08-05T17:33:47Z

    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

  10. Re: Convert varatt.h macros to static inline functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-08-05T18:34:44Z

    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

  11. Re: Convert varatt.h macros to static inline functions

    Masahiko Sawada <sawada.mshk@gmail.com> — 2025-08-05T20:31:52Z

    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

  12. Re: Convert varatt.h macros to static inline functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-08-05T20:59:14Z

    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

  13. Re: Convert varatt.h macros to static inline functions

    Masahiko Sawada <sawada.mshk@gmail.com> — 2025-08-05T21:21:57Z

    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

  14. Re: Convert varatt.h macros to static inline functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-08-05T21:39:33Z

    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

  15. Re: Convert varatt.h macros to static inline functions

    Masahiko Sawada <sawada.mshk@gmail.com> — 2025-08-05T22:07:25Z

    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

  16. Re: Convert varatt.h macros to static inline functions

    Masahiko Sawada <sawada.mshk@gmail.com> — 2025-08-05T22:54:58Z

    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