Re: explain_regress, explain(MACHINE), and default to explain(BUFFERS) (was: BUFFERS enabled by default in EXPLAIN (ANALYZE))

Thanks for the updated patch set!

On Fri, 2022-10-28 at 17:59 -0500, Justin Pryzby wrote:
>
> > 0004, 0005, 0006, 0007: EXPLAIN (MACHINE)
> >
> >   I think it is confusing that these are included in this patch set.
> >   EXPLAIN (MACHINE OFF) is similar to "explain_regress = on", only it goes even further:
> >   no query ID, no Heap Fetches, no Sort details, ...
> >   Why not add this functionality to the GUC?
>
> Good question, and one I've asked myself.
>
> explain_regress affects pre-existing uses of explain in the regression
> tests, aiming to simplify current (or maybe only future) uses.  And
> is obviously used to allow enabling BUFFERS by default.
>
> explain(MACHINE off) is mostly about allowing "explain ANALYZE" to be
> used in regression tests.  Which, right now, is rare.  Maybe I shouldn't
> include those patches until the earlier patches progress (or, maybe we
> should just defer their discussion).

Essentially, "explain_regress" is to simplify the current regression tests,
and MACHINE OFF is to simplify future regression tests. That does not seem
like a meaningful distinction to me. Both are only useful for the regression
tests, and I see no need for two different knobs.

My opinion is now like this:

+1 on enabling BUFFERS by default for EXPLAIN (ANALYZE)

+0.2 on "explain_regress"

-1 on EXPLAIN (MACHINE) in addition to "explain_regress"

-0.1 on adding the MACHINE OFF functionality to "explain_regress"

"explain_regress" reduces the EXPLAIN options you need for regression tests.
This is somewhat useful, but not a big win. Also, it will make backpatching
regression tests slightly harder for the next 5 years.

Hence the +0.2 for "explain_regress".

For me, an important question is: do we really want more EXPLAIN (ANALYZE)
in the regression tests? It will probably slow the tests down, and I wonder
if there is a lot of added value, if we have to remove all the machine-specific
output anyway.

Hence the -0.1.

> >   0005 suppresses "rows removed by filter", but how is that machine dependent?
>
> It can vary with the number of parallel workers (see 13e8b2ee8), which
> may not be "machine dependent" but the point of that patch is to allow
> predictable output of EXPLAIN ANALYZE.  Maybe it needs a better name (or
> maybe it should be folded into the first patch).

Now it makes sense to me. I just didn't think of that.

> I'm not actually sure if this patch should try to address parallel
> workers at all, or (if so) if what it's doing now is adequate.
>
> > > BTW, I think it may be that the GUC should be marked PGDLLIMPORT ?
> >
> > I think it is project policy to apply this mark wherever it is needed.  Do you think
> > that third-party extensions might need to use this in C code?
>
> Since v15 (8ec569479), the policy is:
> > On Windows, export all the server's global variables using PGDLLIMPORT markers (Robert Haas)
>
> I'm convinced now that's what's intended here.

You convinced me too.

> > This is not enough.  The patch would have to update all the examples
> > that use EXPLAIN ANALYZE.
>
> Fair enough.  I've left a comment to handle this later if the patch
> gains any traction and 001 is progressed.

I agree with that.

I would really like to see 0003 go in, but it currently depends on 0001, which
I am not so sure about.
I understand that you did that so that "explain_regress" can turn off BUFFERS
and there is no extra churn in the regression tests.

Still, it would be a shame if resistance against "explain_regress" would
be a show-stopper for 0003.

If I could get my way, I'd want two separate patches: first, one to turn
BUFFERS on, and second one for "explain_regress" with its current functionality
on top of that.

Yours,
Laurenz Albe