On Fri, Mar 21, 2025 at 2:37 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I took a look through v8, and have just a couple of trivial nits:
Thank you!
> +static void overexplain_debug_handler(ExplainState *, DefElem *,
> + ParseState *);
> +static void overexplain_range_table_handler(ExplainState *, DefElem *,
> + ParseState *);
>
> I cordially hate this parameter-name-less style of function
> declaration, and consider it Stroustrup's single worst idea in C++.
> It rests on the assumption that parameter names convey zero
> information, which is wrongheaded for any function more complex than,
> say, addition. Also, even if you like this style, clang-tidy probably
> won't (cf for example 035ce1feb).
I don't hate the style but I didn't use it intentionally. Fixed now, I hope.
> pgoverexplain.sgml seems to have been formatted to fit in about an
> 85-column window. Please don't do that --- you might as well have
> made it 99 columns wide or any other random number, it still looks
> like heck in 80 columns.
Oops. Fixed. Also, I tried to disclaim stability and comprehensibility
a little better and fixed a dumb mistake in the page title.
> Other than that, there's room to debate exactly what to show.
> But as long as we're agreed that we won't hold this module to
> high cross-version compatibility standards, that doesn't seem
> like a problem. I'm okay with this as a starting point.
Great news, thanks again!
Here's v9, which also adds 'SET debug_parallel_query = off' to the
pg_overexplain tests, per CI, because the test results are not (and
cannot realistically be made) stable under under that option.
--
Robert Haas
EDB: http://www.enterprisedb.com
