Re: making EXPLAIN extensible - Mailing list pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:
> OK. It sounds to me like there is a good amount of support for going
> forward with something like what I have, even though some people might
> also like other things. What I feel is currently lacking is some
> review of the actual code. Would anyone like to do that?

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.

0001:

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

SetExplainExtensionState is repalloc'ing ExplainExtensionNameArray;
why?  Wouldn't that invalidate any other pointers to the array?
It disturbs me that you have ExplainState carrying what could
easily become a stale pointer to that array; I'd try to get rid
of that.

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.

This bit in explain.h seems non-idiomatic:

+struct ExplainState;
+typedef struct ExplainState ExplainState;

AFAIK it's sufficient to write the typedef line.  Also I'd
strongly recommend a comment saying that the struct is
defined in explain_state.h.

explain_state.h has the same pattern:

+struct ExplainState;
+typedef struct ExplainState ExplainState;

This is a little more problematic, because I'm pretty sure some
compilers will kvetch when they see the duplicate typedefs.
You need to either (a) write the typedef in only one file,
and use "struct ExplainState" in the other file, or (b)
use some dance like

#ifndef EXPLAINSTATE_TYPEDEF_DEFINED
typedef struct ExplainState ExplainState;
#define EXPLAINSTATE_TYPEDEF_DEFINED 1
#endif

in both files.  If memory serves we do have some headers using
the (b) pattern, but I don't love it particularly.

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.

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?

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

            regards, tom lane