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

public inbox for [email protected]  
help / color / mirror / Atom feed

From: Laurenz Albe <[email protected]>
To: Justin Pryzby <[email protected]>
Cc: David G. Johnston <[email protected]>
Cc: Nikolay Samokhvalov <[email protected]>
Cc: [email protected]
Subject: Re: explain_regress, explain(MACHINE), and default to explain(BUFFERS) (was: BUFFERS enabled by default in EXPLAIN (ANALYZE))
Date: Fri, 04 Nov 2022 11:46:03 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <CANNMO++=LrJ4upoeydZhbmpd_ZgZjrTLueKSrivn6xmb=yFwQw@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<CAKFQuwbnT_B9i4NFWXX0V=xdcsTi0XTHvm6NVcr8XHi39O_A7w@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

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

view thread (14+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: explain_regress, explain(MACHINE), and default to explain(BUFFERS) (was: BUFFERS enabled by default in EXPLAIN (ANALYZE))
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox