Message-ID: <790e175d16cca11244907d3366a6fa376c33e882.camel@cybertec.at>
Subject: Re: explain_regress, explain(MACHINE), and default to
 explain(BUFFERS) (was: BUFFERS enabled by default in EXPLAIN (ANALYZE))
From: Laurenz Albe <laurenz.albe@cybertec.at>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: "David G. Johnston" <david.g.johnston@gmail.com>, Nikolay Samokhvalov
	 <samokhvalov@gmail.com>, pgsql-hackers@lists.postgresql.org
Date: Fri, 04 Nov 2022 11:46:03 +0100
In-Reply-To: <20221028225904.GJ16921@telsasoft.com>
References: 
	<CANNMO++=LrJ4upoeydZhbmpd_ZgZjrTLueKSrivn6xmb=yFwQw@mail.gmail.com>
	 <20211115190954.GO17618@telsasoft.com>
	 <20211202005820.GJ17618@telsasoft.com>
	 <20220124173548.GV23027@telsasoft.com>
	 <CAKFQuwbnT_B9i4NFWXX0V=xdcsTi0XTHvm6NVcr8XHi39O_A7w@mail.gmail.com>
	 <20220905112031.GF31833@telsasoft.com>
	 <20221021020934.GA13372@telsasoft.com>
	 <033ff31aeeb5d0bb49a8e85df32d34e032ab5729.camel@cybertec.at>
	 <20221028225904.GJ16921@telsasoft.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.4 (3.42.4-2.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/790e175d16cca11244907d3366a6fa376c33e882.camel%40cybertec.at>
Precedence: bulk

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