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

[Laurenz Albe]

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