Horton

Thread

Commits

  1. Some stylistic improvements in toast_save_datum()

  2. Silence compiler warnings on clang 21

  3. Convert *GetDatum() and DatumGet*() macros to inline functions

  1. new warnings with clang-21 / how const is Datum

    Peter Eisentraut <peter@eisentraut.org> — 2025-09-01T06:47:17Z

    clang-21 shows some new warnings:

../src/backend/access/common/toast_internals.c:296:33: error: variable 
'chunk_data' is uninitialized when passed as a const pointer argument 
here [-Werror,-Wuninitialized-const-pointer]
   296 |         t_values[2] = PointerGetDatum(&chunk_data);

../src/backend/access/gist/gistutil.c:207:28: error: variable 'attrsize' 
is uninitialized when passed as a const pointer argument here 
[-Werror,-Wuninitialized-const-pointer]
   207 | 
                 PointerGetDatum(&attrsize));
       | 
                                  ^~~~~~~~
../src/backend/access/gist/gistutil.c:276:27: error: variable 'dstsize' 
is uninitialized when passed as a const pointer argument here 
[-Werror,-Wuninitialized-const-pointer]
   276 | 
  PointerGetDatum(&dstsize));
       | 
                   ^~~~~~~


The first one is easily fixed by re-arranging the code a bit.

The other two indicate more fundamental problems.  The gist API uses 
these arguments to pass back information; these pointers do not in fact 
point to a const object.

This possibly means that the change

commit c8b2ef05f48
Author: Peter Eisentraut <peter@eisentraut.org>
Date:   Tue Sep 27 20:47:07 2022

     Convert *GetDatum() and DatumGet*() macros to inline functions

that made PointerGetDatum() look like

     static inline Datum
     PointerGetDatum(const void *X)

was mistaken in adding the const qualifier.

We could remove that qualifier (this would propagate to several other 
functions built on top of PointerGetDatum()), but then there will be 
complaints from the other side:

     static const TableAmRoutine heapam_methods = {

     PG_RETURN_POINTER(&heapam_methods);

Then the question is, which one of these should be considered the outlier?

We could remove the const qualifications from the API and stick an 
unconstify() around &heapam_methods.

Or we could maybe make a new function PointerNonConstGetDatum() that we 
use for exceptional cases like the gist API.

There is a related issue that I have been researching for some time.  It 
seems intuitively correct that the string passed into a data type input 
function should not be modified by that function.  And so the relevant 
functions could use const qualifiers like

extern Datum InputFunctionCall(FmgrInfo *flinfo, const char *str, ...);

which they currently do not.

There are a some places in the code where const strings get passed into 
some *InputFunctionCall() functions and have their const qualifications 
rudely cast away, which seems unsatisfactory.

Overall, the question to what extent fmgr functions are allowed to 
modify objects pointed to by their input arguments doesn't seem to be 
addressed anywhere.

  2. Re: new warnings with clang-21 / how const is Datum

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-09-01T15:05:49Z

    Peter Eisentraut <peter@eisentraut.org> writes:
> Overall, the question to what extent fmgr functions are allowed to 
> modify objects pointed to by their input arguments doesn't seem to be 
> addressed anywhere.

I think it's generally understood that an fmgr function must not
modify pass-by-reference inputs, because they could easily be
pointing directly into a heap tuple in a shared buffer.  The
exception is functions that participate in custom APIs where the
presence of output argument(s) is explicitly documented.

One question that statement leaves unanswered is whether datatype
input functions qualify as a "custom API".  I'd vote not (ie they
shouldn't modify their input strings) unless we find exceptions.

So that suggests that the use of non-const pointer Datums should be
the outlier.

			regards, tom lane

  3. Re: new warnings with clang-21 / how const is Datum

    Peter Eisentraut <peter@eisentraut.org> — 2025-09-08T12:01:20Z

    On 01.09.25 08:47, Peter Eisentraut wrote:
> clang-21 shows some new warnings:
> 
> ../src/backend/access/common/toast_internals.c:296:33: error: variable 
> 'chunk_data' is uninitialized when passed as a const pointer argument 
> here [-Werror,-Wuninitialized-const-pointer]
>    296 |         t_values[2] = PointerGetDatum(&chunk_data);
> 
> ../src/backend/access/gist/gistutil.c:207:28: error: variable 'attrsize' 
> is uninitialized when passed as a const pointer argument here [-Werror,- 
> Wuninitialized-const-pointer]
>    207 |                 PointerGetDatum(&attrsize));
>        |                                  ^~~~~~~~
> ../src/backend/access/gist/gistutil.c:276:27: error: variable 'dstsize' 
> is uninitialized when passed as a const pointer argument here [-Werror,- 
> Wuninitialized-const-pointer]
>    276 |  PointerGetDatum(&dstsize));
>        |                   ^~~~~~~

Here is a quick-fix patch for this.  It silences these warnings by 
initializing the respective variables first.  This is already done 
similarly in nearby code.  This can be backpatched to PG16, where these 
warnings began.

The second patch is a bit of a more extensive code rearrangement to make 
the need for the workaround in the first patch go away.  This would be 
for master only.