On Wed, Mar 5, 2025 at 1:53 PM Tom Lane <t...@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
v3-0002-Add-some-new-hooks-so-extensions-can-add-details-.patch
Description: Binary data
v3-0001-Make-it-possible-for-loadable-modules-to-add-EXPL.patch
Description: Binary data
v3-0003-pg_overexplain-Additional-EXPLAIN-options-for-deb.patch
Description: Binary data
