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

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 &lt; 100 AND unique2 &gt; 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