public inbox for [email protected]  

help / color / mirror / Atom feed

From: Heikki Linnakangas <[email protected]>
To: Tom Lane <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Cc: David Geier <[email protected]>
Cc: Matthias van de Meent <[email protected]>
Cc: pgsql-hackers <[email protected]>
Subject: Re: Reduce build times of pg_trgm GIN indexes
Date: Thu, 16 Apr 2026 20:30:51 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<CAEze2WiUL9idZBbuUN+MuWqr6DcPr_-C91E9MTx=H62Xx5fHaQ@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

On 16/04/2026 17:37, Tom Lane wrote:
> Heikki Linnakangas <[email protected]> writes:
>> On 16/04/2026 11:45, Peter Eisentraut wrote:
>>> 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?
> 
> Good point.
> 
>>> 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().
> 
> I think there can be no doubt that most functions receiving a
> pass-by-ref Datum are not supposed to scribble on the pointed-to
> data.  So it makes sense to me that PointerGetDatum should carry
> an implication of const-ness, and then we need to invent a new
> notation to use in the small number of places where that's not
> appropriate.  I'd capitalize it as NonConstPointerGetDatum,
> but other than that nit that naming suggestion seems fine to me.

That makes sense. My worry is that we're changing the rules in a very 
subtle way: It used to be OK to use PointerGetDatum(), pass the 
resulting datum to something that modifies it. Now we say it's not OK, 
and you must use NonConstPointerGetDatum(). You don't get any compiler 
warnings if you use it wrong, except for this one coverity warning 
apparently, but it doesn't catch this reliably either.

> Of course, then the *real* question is why DatumGetPointer
> doesn't deliver a const pointer.  But I don't see how to get
> there without extremely invasive changes.

Good point.

>> We could have all three:
> 
> Not excited about making massive changes for this.

Having all three would be a very localized change in postgres.h.

> I remain far less certain than Peter is that this discussion has
> anything to do with why Coverity is complaining about
> ginExtractEntries.  I still think we should make some minimum-effort
> change to see if the complaint goes away before expending a lot of
> brain cells on choosing a final fix.

I think I'm going to commit my proposal to turn PointerGetDatum() back 
into a macro, and see if that makes Coverity happy. Then we'll know, and 
we can decide on the next steps. Any objections?

One open question is whether we should backpatch any of this. I guess 
compilers don't misoptimize this in practice, or we would've gotten more 
reports, but I really can't rationalize why not and a new compiler 
version might well start hitting this.

- Heikki

view thread (31+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Reduce build times of pg_trgm GIN indexes
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox