Re: making EXPLAIN extensible

On Wed, Mar 5, 2025 at 1:53 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Here's some comments on 0001 and 0002; I didn't have time for
> 0003 today.  But in any case, I think you should move forward
> with committing 0001/0002 soon so other people can play around
> in this space.  0003 can be left for later.

Cool. Thank you very much for the review!

> GetExplainExtensionState and SetExplainExtensionState should probably
> have guards against extension_id < 0, even if it's just an Assert.
> Or make the id unsigned?

I like the Assert() idea better, so I added that. This way, an
extension can write "static int es_extension_id = -1;" or similar and
the Assert() will catch use-before-initialization errors.

> SetExplainExtensionState is repalloc'ing ExplainExtensionNameArray;
> why?  Wouldn't that invalidate any other pointers to the array?

Wow, ouch. That's a brown-paper-bag bug. It should be allocating and
then reallocating es->extension_state. The point is to make sure the
assignment at the tail end of the function isn't indexing off the
allocated portion of the array.

> RegisterExtensionExplainOption has "char *option_name", but I think
> that should be "const char *", and the function should have the same
> disclaimer as GetExplainExtensionId about that string needing to be
> constant-or-never-freed.

Added.

> This bit in explain.h seems non-idiomatic:
> explain_state.h has the same pattern:

Fixed, I hope.

> 0002:
>
> I'm fine with the order of additions being determined by module
> load order.  Users who are feeling picky about that can arrange
> the module load order as they wish.  If we put in a priority
> mechanism then the order would be determined by module authors,
> who probably don't want the responsibility, and not by the users
> whose results are actually affected.

Check.

> I'm quite confused by the #include additions in auto_explain.c and
> file_fdw.c, and I strongly object to the ones in explain_state.h.
> Surely those are unnecessary?

They are necessary but they should have been part of 0001. Because
0001 moves the definition of ExplainState to explain_state.h, files
that need to access to the members of that structure now need to
include that header file. As for the includes in explain_state.h, it
needs definitions for DefElem, ParseState, and PlannedStmt.

> Anyway, these are all very minor concerns; overall I think
> it's going in the right direction.

I am very happy to hear that. Thanks!

v3 attached.

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