On Mon, Sep 22, 2025 at 3:51 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I looked through the v5 patchset.
Thanks!
Here's a new set of patches. I've added a new 0001 at the beginning to
fix the bug you identified in SetExplainExtensionState; this will need
to be back-patched to v18 once the release freeze lifts. I've also
adjusted the previous patch, now 0002, to fix the equivalent problem
with the new Set*ExplainState functions,and I've attempted to fix the
other problems you mentioned, with this exception:
> Also, although you're not the first sinner, I really think
> this new argument to planner() should be documented. Maybe
> the rest too while we're at it.
I'm a little nervous about this. I fear that the comments are all
going to be of the form "to save a file, click the File menu, then
click Save," which doesn't actually help anyone. That might be a
slight exaggeration, but I feel like it's pretty obvious on visual
inspection that Query *parse is what we're planning, char
*query_string is the text version of that, cursorOptions is some kind
of flag mask, boundParams is the parameter values. It's fair to say
that ExplainState *es is a little less obvious than the others, but
saying "If this function is being invoked by EXPLAIN, then
ExplainState *es is the ExplainState, else it is NULL" doesn't really
seem all that helpful to me. I mean, what else would it be? My thought
here would be that if you want to write some comments that you
consider helpful for the existing arguments, I'll try to write a new
comment for this one in the same style (or you can suggest one) and
hold my nose if I don't find it helpful, or alternatively, we could
proceed with these patches without the comment and you can add
whatever comment text you want after-the-fact. If you want the comment
changes in this patch set, then I need a suggestion as to what that
should look like.
--
Robert Haas
EDB: http://www.enterprisedb.com
