Date: Fri, 28 Oct 2022 17:59:04 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Laurenz Albe <laurenz.albe@cybertec.at>
Cc: "David G. Johnston" <david.g.johnston@gmail.com>,
	Nikolay Samokhvalov <samokhvalov@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: explain_regress, explain(MACHINE), and default to
 explain(BUFFERS) (was: BUFFERS enabled by default in EXPLAIN (ANALYZE))
Message-ID: <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>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="XJ5wUQMT/jAb5xue"
Content-Disposition: inline
In-Reply-To: <033ff31aeeb5d0bb49a8e85df32d34e032ab5729.camel@cybertec.at>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20221028225904.GJ16921%40telsasoft.com>
Precedence: bulk


--XJ5wUQMT/jAb5xue
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Tue, Oct 25, 2022 at 03:49:14PM +0200, Laurenz Albe wrote:
> On Thu, 2022-10-20 at 21:09 -0500, Justin Pryzby wrote:
> > Rebased.
> 
> I had a look at the patch set.

Thanks for looking

>   @@ -2288,6 +2288,7 @@ regression_main(int argc, char *argv[],
>           fputs("log_lock_waits = on\n", pg_conf);
>           fputs("log_temp_files = 128kB\n", pg_conf);
>           fputs("max_prepared_transactions = 2\n", pg_conf);
>   +       // fputs("explain_regress = yes\n", pg_conf);
>  
>           for (sl = temp_configs; sl != NULL; sl = sl->next)
>           {
> 
>   This code comment is a leftover and should go.

No - I was wondering whether the GUC should be specified by the
environment or by the file; I think it's better by file.  Then it also
needs to be in Cluster.pm.

> 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).

>   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).

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.

> 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.

-- 
Justin

--XJ5wUQMT/jAb5xue
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="0001-Add-GUC-explain_regress.patch"