Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Mar 5, 2025 at 4:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Got it. So does that mean we can remove any #include's from explain.h
>> after moving the struct definition?
> Good question. It looks like "lib/stringinfo.h" can come out but the
> other two are still needed, so there is not much gain. But I've made
> that change in the attached v4.
OK.
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.
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.
In perhaps-less-nitpicky territory, I wonder how well the
"per backend" notion of extension and option IDs plays with
extensions that assign those during _PG_init, as I see you've
done in 0003. What I'm concerned about is what happens when
the extension is loaded using shared_preload_libraries. I
think what will happen is
(1) IDs will be assigned in the postmaster, and nothing bad
happens as long as TopMemoryContext already exists, which it will.
(2) Such IDs propagate to backends via fork(), and thus are
effectively system-wide.
(3) But ... in an EXEC_BACKEND build, it doesn't work like
that. Child processes will repeat the postmaster's processing
of shared_preload_libraries. Probably they will load the
modules in the same order and get the same ID assignments,
but I'm not quite sure that's guaranteed.
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.
regards, tom lane
