public inbox for [email protected]
help / color / mirror / Atom feedFrom: Laurenz Albe <[email protected]>
To: Justin Pryzby <[email protected]>
To: 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: Tue, 25 Oct 2022 15:49:14 +0200
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]>
On Thu, 2022-10-20 at 21:09 -0500, Justin Pryzby wrote:
> Rebased.
I had a look at the patch set.
It applies and builds cleanly and passes the regression tests.
0001: Add GUC: explain_regress
I like the idea of the "explain_regress" GUC. That should simplify
the regression tests.
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -625,7 +625,7 @@ initialize_environment(void)
* user's ability to set other variables through that.
*/
{
- const char *my_pgoptions = "-c intervalstyle=postgres_verbose";
+ const char *my_pgoptions = "-c intervalstyle=postgres_verbose -c explain_regress=on";
const char *old_pgoptions = getenv("PGOPTIONS");
char *new_pgoptions;
@@ -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.
0002: exercise explain_regress
This is the promised simplification. Looks good.
0003: Make explain default to BUFFERS TRUE
Yes, please!
This patch is independent from the previous patches.
I'd like this to be applied, even if the GUC is rejected.
(I understand that that would cause more changes in the regression
test output if we changed that without introducing "explain_regress".)
The patch changes the default value of "auto_explain.log_buffers"
to "on", which makes sense. However, the documentation in
doc/src/sgml/auto-explain.sgml should be updated to reflect that.
--- a/doc/src/sgml/perform.sgml
+++ b/doc/src/sgml/perform.sgml
@@ -731,8 +731,8 @@ EXPLAIN ANALYZE SELECT * FROM polygon_tbl WHERE f1 @> polygon '(0.5,2.0)';
</para>
<para>
- <command>EXPLAIN</command> has a <literal>BUFFERS</literal> option that can be used with
- <literal>ANALYZE</literal> to get even more run time statistics:
+ <command>EXPLAIN ANALYZE</command> has a <literal>BUFFERS</literal> option which
+ provides even more run time statistics:
<screen>
EXPLAIN (ANALYZE, BUFFERS) SELECT * FROM tenk1 WHERE unique1 < 100 AND unique2 > 9000;
This is not enough. The patch would have to update all the examples that use EXPLAIN ANALYZE.
I see two options:
1. Change the output of all the examples and move this explanation to the first example
with EXPLAIN ANALYZE:
The numbers in the <literal>Buffers:</literal> line help to identify which parts
of the query are the most I/O-intensive.
2. Change all examples that currently do *not* use BUFFERS to EXPLAIN (BUFFERS OFF).
Then you could change the example that shows BUFFERS to something like
If you do not suppress the <literal>BUFFERS</literal> option that can be used with
<command>EXPLAIN (ANALYZE)</command>, you get even more run time statistics:
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?
0005 suppresses "rows removed by filter", but how is that machine dependent?
> 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?
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