Re: Reduce build times of pg_trgm GIN indexes - Mailing list pgsql-hackers

On 16/04/2026 11:45, Peter Eisentraut wrote:
> On 15.04.26 23:25, Tom Lane wrote:
>> Peter Eisentraut <peter@eisentraut.org> writes:
>>> On 15.04.26 13:06, Heikki Linnakangas wrote:
>>>> This was briefly discussed when PointerGetDatum() was changed from a
>>>> macro to a static inline function [1]. On that email, Peter pointed out
>>>> that the compiler was doing the same deduction that Coverity did now,
>>>> i.e. that if you pass the Datum returned by PointerGetDatum(&foo) to a
>>>> function, it cannot change *foo. I'm surprised we dismissed that worry
>>>> so quickly. If the compiler optimizes based on that assumption, you can
>>>> get incorrect code.
>>
>>> I don't think this is in evidence.  AFAICT, it's just Coverity that is
>>> complaining here, which is its right, but the code is not incorrect.
>>
>> Are you sure?  This seems like the sort of thing that will bite us on
>> the rear sometime in the future, as the compiler geeks put in more and
>> more aggressive optimizations.
>>
>> I think we should at least test the theory that changing
>> PointerGetDatum to remove the const cast would silence Coverity's
>> complaint.  If it does not then we're attributing too much
>> intelligence to Coverity.  But if it does, then we've correctly
>> identified why it's complaining, and we should take seriously the
>> idea that they aren't the only ones making this sort of deduction
>> (or won't be for long).
> 
> I think it's quite clear to me that Coverity is complaining about this 
> correctly, in its view of the world.  Compilers sometimes complain about 
> this, too, although in this case they apparently don't look quite as 
> deeply to do this analysis.
> 
> What I'm missing here is, essentially where the previous thread stopped: 
> What is the overall message that we want to communicate with the API?
> 
> If the default assumption is that what pointers converted to Datums 
> point to should not be modified on the other side (where the Datum is 
> converted back to a pointer), then the current declaration of 
> PointerGetDatum() is suitable, and the GIN code can be considered an 
> exception and we make a special API for that.  The previous thread 
> proposed NonconstPointerGetDatum().
> 
> (If this is the resolution, I also have half a patch somewhere that 
> makes the string input argument for the InputFunctionCall family of 
> functions const, which also seems intuitively sensible.)
> 
> If, on the other hand, the decision is that there is in fact no such 
> guarantee, that consumers of Datums are free to modify whatever they 
> seem fit, then we should drop the const of PointerGetDatum and fix the 
> fallout up the call stack.
> 
> The macro proposed by Heikki, I don't know, still doesn't actually 
> answer this question, just (possibly) makes these warnings go away in a 
> slightly mysterious way.

My intention was that if you do:

     const foo *ptr;
     ...
     datum = PointerGetDatum(ptr);

Then you're not allowed to modify the contents of *ptr through the 
'datum'. But if you do:

     foo *ptr;
     ...
     datum = PointerGetDatum(ptr);

Then it is allowed.

I don't know how to tell the compiler exactly that. I tried various 
hacks with _Generic(), but couldn't make it work. The macro casts away 
the 'const' in the first case, which might disable some optimizations 
that the compiler could otherwise do.

We could have all three:

ConstPointerGetDatum(const void *): Promises that the returned Datum is 
not used to modify

NonConstPointerGetDatum(void *): No such promise.

PointerGetDatum(): Macro as I proposed. Like NonConstPointerGetDatum(), 
but for backwards-compatibility it doesn't emit a warning if you pass a 
const pointer to it.

- Heikki