Re: making EXPLAIN extensible - Mailing list pgsql-hackers

On Wed, Mar 5, 2025 at 4:38 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> v4 has addressed most of my nitpicks, but you still have typedefs
> for ExplainState in both header files.  My bet is that at least
> one buildfarm animal will complain about that.  I could be wrong
> though, maybe all such compilers are in disuse now.

Ugh, I suck at this, sorry. Adjusted in v5. It's hard to avoid the
conclusion that our IWYU configuration must be fairly lenient, because
every change seems to surface more source files that are depending on
indirect includes.

> Also I noted one more nitpick: I think SetExplainExtensionState
> needs to grow the array with repalloc0 not repalloc, else you
> risk GetExplainExtensionState returning garbage pointers for
> not-yet-set array entries.

Hardly a nitpick, thanks. I've tried to fix this.

> In short, I wonder if somebody might build code that depends
> on the IDs being the same across a cluster, and find that
> it mostly works but sometimes not on Windows.  Maybe we don't
> care given that we explicitly disclaimed them being the same.
> But it's probably worth a bit of thought.

I think that if the only thing a backend does with the ID returned by
GetExplainExtensionId is call GetExplainExtensionState() and
SetExplainExtensionState(), there's no problem here. So to me, the
question here is whether there's a reason to use that ID for anything
else.

If there is, maybe we should discard the integer IDs and just use
strings directly. I could make GetExplainExtensionState() and
SetExplainExtensionState() take const char *extension_name arguments
instead of int extension_id, and just get rid of GetExplainExtensionId
entirely. I chose the current design because I thought that the IDs
did not need to be shared and I thought it would be a bit extravagant
to have an EXPLAIN extension need to do a hash table lookup for every
plan node, but maybe it wouldn't really matter in practice.

But I wonder what else you think that the ID might get used for. If
you're trying to do some kind of general coordination across all
backends using shared memory, we have ShmemInitStruct() and
GetNamedDSMSegment() for that purpose, and those do indeed use names,
and an extension should probably use those interfaces for that kind of
work rather than trying to do something with the explain extension ID
that is not intended.

The best argument that I can see for maybe using that extension ID for
some other purpose is parallel query. The ExplainState is not shared
across backends and presumably never will be, so there's no immediate
problem. But you can perhaps imagine someone wanting to do more than
this infrastructure allows. In particular, I'm imagining that people
might want to run extensions that annotate parts of the plan with
extension-provided data. If such annotations were added at plan time,
and if they used the extension ID, that still wouldn't give rise to
any cross-backend use, but if they were added at execution time, then
parallel query could be happening, and if the extension ID were then
used, trouble could occur.

But to me that sounds a little too hypothetical for me to be concerned
about it. For one thing, we haven't actually agreed on doing anything
of that sort. More importantly, using integer extension IDs for this
feature does not require that we use the integer extension IDs for
that feature, if and when it gets implemented, and it certainly
doesn't require that we use the same ones. And I feel like it might be
slightly surprising if it did, because they seem like two different
features that maybe ought to work in different ways. Those
hypothetical future features seem like part of the planner or part of
the executor, respectively, so I can't really see a reason why they
would pull their main identifier out of the EXPLAIN system.

So I guess I can't really think of a convincing reason why the current
design would cause a problem for anybody. Of course, my imagination
might be lacking.

Regarding commit timeframe, you expressed a preference for 0001 and
0002 to be committed "soon," but I'm going to be leaving town for a
week on Saturday and would prefer not to put myself in a situation
where somebody is expecting me to fix the buildfarm while I am gone.
So I'm happy to commit them today or tomorrow if you don't mind being
responsible for anything that blows up while I'm gone, or I'm happy to
have you commit them whenever you want, but barring one of those
outcomes, I'm going to deal with it on or after March 17th.

--
Robert Haas
EDB: http://www.enterprisedb.com