Re: explain_regress, explain(MACHINE), and default to explain(BUFFERS) (was: BUFFERS enabled by default in EXPLAIN (ANALYZE))
On Sat, Nov 05, 2022 at 10:43:07AM -0400, Tom Lane wrote: > Justin Pryzby writes: > > You set the patch to "waiting on author", which indicates that there's > > no need for further input or review. But, I think that's precisely > > what's needed - without input from more people, what could I do to > > progress the patch ? I don't think it's reasonable to put 001 first and > > change thousands (actually, 1338) of regression results. If nobody > > wants to discuss 001, then this patch series won't progress. > > Well ... > > 1. 0001 invents a new GUC but provides no documentation for it. > That certainly isn't committable, and it's discouraging the > discussion you seek because people have to read the whole patch > in detail to understand what is being proposed. > > 2. The same complaint for 0004, which labors under the additional I suggested that the discussion be limited to the early patches (004 is an optional, possible idea that I had, but it's evidently still causing confusion). > 3. I'm not really on board with the entire approach. Making > EXPLAIN work significantly differently for developers and test > critters than it does for everyone else seems likely to promote > confusion and hide bugs. I don't think getting rid of the need > for filter functions in test scripts is worth that. I'm open to suggestions how else to move towards the goal. Note that there's a few other things which have vaguely-similar behavior: HIDE_TABLEAM=on HIDE_TOAST_COMPRESSION=on compute_query_id = regress Another possibility is to make a new "explain" format, like | explain(FORMAT REGRESS, ...). ...but I don't see how that's would be less objectionable than what I've written. The patch record should probably be closed until someone proposes another way to implement what's necessary to enable explain(BUFFERS) by default. -- Justin
Re: explain_regress, explain(MACHINE), and default to explain(BUFFERS) (was: BUFFERS enabled by default in EXPLAIN (ANALYZE))
Justin Pryzby writes: > You set the patch to "waiting on author", which indicates that there's > no need for further input or review. But, I think that's precisely > what's needed - without input from more people, what could I do to > progress the patch ? I don't think it's reasonable to put 001 first and > change thousands (actually, 1338) of regression results. If nobody > wants to discuss 001, then this patch series won't progress. Well ... 1. 0001 invents a new GUC but provides no documentation for it. That certainly isn't committable, and it's discouraging the discussion you seek because people have to read the whole patch in detail to understand what is being proposed. 2. The same complaint for 0004, which labors under the additional problem that MACHINE is one of the worst option names I've ever seen proposed around here. It conveys *nothing* about what it does or is good for. The fact that it's got no relationship to the GUC name, and yet they're intimately connected, doesn't improve my opinion of it. 3. I'm not really on board with the entire approach. Making EXPLAIN work significantly differently for developers and test critters than it does for everyone else seems likely to promote confusion and hide bugs. I don't think getting rid of the need for filter functions in test scripts is worth that. 4. I don't see how the current patches could pass under "make installcheck" --- it looks to me like it only takes care of applying the GUC change in installations built by pg_regress or Cluster.pm. To make this actually work, you'd have to add "explain_regress = on" to the ALTER DATABASE SET commands issued for regression databases. That'd substantially increase the problem of "it works differently than what users see", at least for me --- I do a lot of stuff interactively in the regression database. 5. The change in postgres_fdw.c in 0001 concerns me too. Yeah, it will fix things if an updated postgres_fdw talks to an updated server, but what about an older postgres_fdw? Don't really want to see that case break; communication with servers of different major versions is one of postgres_fdw's main goals. As a side note, 0001 also creates a hard break in postgres_fdw's ability to work with pre-9.0 remote servers, which won't accept "EXPLAIN (COSTS)". Maybe we don't care about that anymore, given the policy we've adopted elsewhere that we won't test or support interoperability with versions before 9.2. But we should either make the command spelling conditional on remote version, or officially move that goalpost. regards, tom lane
Re: explain_regress, explain(MACHINE), and default to explain(BUFFERS) (was: BUFFERS enabled by default in EXPLAIN (ANALYZE))
On Fri, Nov 04, 2022 at 11:46:03AM +0100, Laurenz Albe wrote: > "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. But it doesn't cause backpatching to be harder; if a patch is meant to be backpatched, its author can still choose to write the old way, with (COSTS OFF, ...) > 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. I don't intend to encourage putting lots of "explain analyze" in the tests. Rather, this makes that a reasonable possibility rather than something that people are discouraged from doing by its difficulty. Using "explain analyze" still needs to be evaluated (for speed and otherwise), same as always. In any case, I suggested to defer review of patches 004 and beyond, which are optional, and it distracts from the essential discussion about 001. > 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. You set the patch to "waiting on author", which indicates that there's no need for further input or review. But, I think that's precisely what's needed - without input from more people, what could I do to progress the patch ? I don't think it's reasonable to put 001 first and change thousands (actually, 1338) of regression results. If nobody wants to discuss 001, then this patch series won't progress. -- Justin
Re: explain_regress, explain(MACHINE), and default to explain(BUFFERS) (was: BUFFERS enabled by default in EXPLAIN (ANALYZE))
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
Re: explain_regress, explain(MACHINE), and default to explain(BUFFERS) (was: BUFFERS enabled by default in EXPLAIN (ANALYZE))
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
>From c4527a8d878cfe48d2b62f6f631adbdf37ea83c2 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Sat, 22 Feb 2020 21:17:10 -0600
Subject: [PATCH 1/7] Add GUC: explain_regress
This changes the defaults for explain to: costs off, timing off, summary off.
It'd be reasonable to use this for new regression tests which are not intended
to be backpatched.
---
contrib/postgres_fdw/postgres_fdw.c | 2 +-
src/backend/commands/explain.c | 13 +++--
src/backend/utils/misc/guc_tables.c | 13 +
src/include/commands/explain.h | 2 ++
src/test/perl/PostgreSQL/Test/Cluster.pm | 1 +
src/test/regress/expected/explain.out| 3 +++
src/test/regress/expected/inherit.out| 2 +-
src/test/regress/expected/stats_ext.out | 2 +-
src/test/regress/pg_regress.c| 1 +
src/test/regress/sql/explain.sql | 4
src/test/regress/sql/inherit.sql | 2 +-
src/test/regress/sql/stats_ext.sql | 2 +-
12 files changed, 40 insertions(+), 7 deletions(-)
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 8d7500abfbd..3a4f56695b1 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -3138,7 +3138,7 @@ estimate_path_cost_size(PlannerInfo *root,
* values, so don't request params_list.
*/
initStringInfo();
- appendStringInfoString(, "EXPLAIN ");
+ appendStringInfoString(, "EXPLAIN (COSTS)");
deparseSelectStmtForRel(, root, foreignrel, fdw_scan_tlist,
remote_conds, pathkeys,
fpextra ? fpextra->has_final_sort : false,
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index f86983c6601..373fde4d498 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -154,6 +154,7 @@ static void ExplainJSONLineEnding(ExplainState *es);
static void ExplainYAMLLineStarting(ExplainState *es);
static void escape_yaml(StringInfo buf, const char *str);
+bool explain_regress = false; /* GUC */
/*
@@ -172,6 +173,7 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
ListCell *lc;
bool timing_set = false;
bool summary_set = false;
+ bool costs_set = false;
/* Parse options list. */
foreach(lc, stmt->options)
@@ -183,7 +185,11
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)';
-EXPLAIN has a BUFFERS option that
can be used with
-ANALYZE to get even more run time statistics:
+EXPLAIN ANALYZE has a BUFFERS
option which
+provides even more run time statistics:
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 Buffers: 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 BUFFERS option that can be
used with
EXPLAIN (ANALYZE), 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
Re: explain_regress, explain(MACHINE), and default to explain(BUFFERS) (was: BUFFERS enabled by default in EXPLAIN (ANALYZE))
Rebased.
BTW, I think it may be that the GUC should be marked PGDLLIMPORT ?
>From 12a605ca84bf21439e4ae51cc3f3a891b3cb4989 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Sat, 22 Feb 2020 21:17:10 -0600
Subject: [PATCH 1/7] Add GUC: explain_regress
This changes the defaults for explain to: costs off, timing off, summary off.
It'd be reasonable to use this for new regression tests which are not intended
to be backpatched.
---
contrib/postgres_fdw/postgres_fdw.c | 2 +-
src/backend/commands/explain.c | 13 +++--
src/backend/utils/misc/guc_tables.c | 13 +
src/include/commands/explain.h | 2 ++
src/test/regress/expected/explain.out | 3 +++
src/test/regress/expected/inherit.out | 2 +-
src/test/regress/expected/stats_ext.out | 2 +-
src/test/regress/pg_regress.c | 3 ++-
src/test/regress/sql/explain.sql| 4
src/test/regress/sql/inherit.sql| 2 +-
src/test/regress/sql/stats_ext.sql | 2 +-
11 files changed, 40 insertions(+), 8 deletions(-)
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 8d7500abfbd..3a4f56695b1 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -3138,7 +3138,7 @@ estimate_path_cost_size(PlannerInfo *root,
* values, so don't request params_list.
*/
initStringInfo();
- appendStringInfoString(, "EXPLAIN ");
+ appendStringInfoString(, "EXPLAIN (COSTS)");
deparseSelectStmtForRel(, root, foreignrel, fdw_scan_tlist,
remote_conds, pathkeys,
fpextra ? fpextra->has_final_sort : false,
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index f86983c6601..373fde4d498 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -154,6 +154,7 @@ static void ExplainJSONLineEnding(ExplainState *es);
static void ExplainYAMLLineStarting(ExplainState *es);
static void escape_yaml(StringInfo buf, const char *str);
+bool explain_regress = false; /* GUC */
/*
@@ -172,6 +173,7 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
ListCell *lc;
bool timing_set = false;
bool summary_set = false;
+ bool costs_set = false;
/* Parse options list. */
foreach(lc, stmt->options)
@@ -183,7 +185,11 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
else if (strcmp(opt->defname, "verbose") == 0)
es->verbose = defGetBoolean(opt);
else if (strcmp(opt->defname, "costs") == 0)
+ {
+ /* Need to keep track if it was explicitly set to ON */
+ costs_set = true;
es->costs = defGetBoolean(opt);
+ }
else if (strcmp(opt->defname, "buffers") == 0)
es->buffers = defGetBoolean(opt);
else if (strcmp(opt->defname, "wal") == 0)
@@ -227,13 +233,16 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
parser_errposition(pstate, opt->location)));
}
+ /* if the costs option was not set explicitly, set default value */
+ es->costs = (costs_set) ? es->costs : es->costs && !explain_regress;
+
if (es->wal && !es->analyze)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("EXPLAIN option WAL requires ANALYZE")));
/* if the timing was not set explicitly, set default value */
- es->timing = (timing_set) ? es->timing : es->analyze;
+ es->timing = (timing_set) ? es->timing : es->analyze && !explain_regress;
/* check that timing is used with EXPLAIN ANALYZE */
if (es->timing && !es->analyze)
@@ -242,7 +251,7 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
errmsg("EXPLAIN option TIMING requires ANALYZE")));
/* if the summary was not set explicitly, set default value */
- es->summary = (summary_set) ? es->summary : es->analyze;
+ es->summary = (summary_set) ? es->summary : es->analyze && !explain_regress;
query = castNode(Query, stmt->query);
if (IsQueryIdEnabled())
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 05ab087934c..1d34e6bdb7b 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -36,6 +36,7 @@
#include "catalog/namespace.h"
#include "catalog/storage.h"
#include "commands/async.h"
+#include "commands/explain.h"
#include "commands/tablespace.h"
#include "commands/trigger.h"
#include "commands/user.h"
@@ -967,6 +968,18 @@ struct config_bool ConfigureNamesBool[] =
true,
NULL, NULL, NULL
},
+
+ {
+ {"explain_regress", PGC_USERSET, DEVELOPER_OPTIONS,
+ gettext_noop("Change defaults of EXPLAIN to avoid unstable output."),
+ NULL,
+ GUC_NOT_IN_SAMPLE | GUC_EXPLAIN
+ },
+ _regress,
+ false,
+ NULL, NULL, NULL
+ },
+
{
{"geqo", PGC_USERSET, QUERY_TUNING_GEQO,
gettext_noop("Enables genetic query optimization."),
diff --git a/src/include/commands/explain.h b/src/include/commands/explain.h
index 9ebde089aed..912bc9484ef 100644
--- a/src/include/commands/explain.h
+++ b/src/include/commands/explain.h
@@ -61,6 +61,8 @@ typedef struct
Re: explain_regress, explain(MACHINE), and default to explain(BUFFERS) (was: BUFFERS enabled by default in EXPLAIN (ANALYZE))
On Tue, Jul 26, 2022 at 03:38:53PM -0700, David G. Johnston wrote:
> On Mon, Jan 24, 2022 at 9:54 AM Justin Pryzby wrote:
> > > Unfortunately, "COSTS OFF" breaks postgres_fdw remote_estimate. If
> > > specifying
> > > "COSTS ON" in postgres_fdw.c is considered to be a poor fix , then I
> > > suppose
> > > this patch series could do as suggested and enable buffers by default
> > > only when
> > > ANALYZE is specified. Then postgres_fdw is not affected, and the
> > > explain_regress GUC is optional: instead, we'd need to specify BUFFERS
> > > OFF in
> > > ~100 regression tests which use EXPLAIN ANALYZE.
>
> I'm not following the transition from the prior sentences about COSTS to
> this one regarding BUFFERS.
In this patch, SET explain_regress=on changes to defaults to
explain(COSTS OFF), so postgres_fdw now explicitly uses COSTS ON. The
patch also changes default to (BUFFERS OFF), which is what allows
enabling buffers by default. I think I was just saying that the
enabling buffers by default depends on somehow disabling it in
regression tests (preferably not by adding 100 instances of "BUFFERS
OFF").
> The following change in the machine commit seems out-of-place; are we
> fixing a bug here?
>
> explain.c:
>/* Show buffer usage in planning */
> - if (bufusage)
> + if (bufusage && es->buffers)
I think that was an extraneous change, maybe from a conflict while
re-ordering the patches. Removed it and rebased. Thanks for looking.
--
Justin
>From 0bf5296d9e71e136509557fa62c6479965c753e8 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Sat, 22 Feb 2020 21:17:10 -0600
Subject: [PATCH 1/7] Add GUC: explain_regress
This changes the defaults for explain to: costs off, timing off, summary off.
It'd be reasonable to use this for new regression tests which are not intended
to be backpatched.
---
contrib/postgres_fdw/postgres_fdw.c | 2 +-
src/backend/commands/explain.c | 13 +++--
src/backend/utils/misc/guc.c| 13 +
src/include/commands/explain.h | 2 ++
src/test/regress/expected/explain.out | 3 +++
src/test/regress/expected/inherit.out | 2 +-
src/test/regress/expected/stats_ext.out | 2 +-
src/test/regress/pg_regress.c | 3 ++-
src/test/regress/sql/explain.sql| 4
src/test/regress/sql/inherit.sql| 2 +-
src/test/regress/sql/stats_ext.sql | 2 +-
11 files changed, 40 insertions(+), 8 deletions(-)
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 16320170cee..68f3e97e13b 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -3131,7 +3131,7 @@ estimate_path_cost_size(PlannerInfo *root,
* values, so don't request params_list.
*/
initStringInfo();
- appendStringInfoString(, "EXPLAIN ");
+ appendStringInfoString(, "EXPLAIN (COSTS)");
deparseSelectStmtForRel(, root, foreignrel, fdw_scan_tlist,
remote_conds, pathkeys,
fpextra ? fpextra->has_final_sort : false,
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 053d2ca5ae4..f3ce3593f36 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -154,6 +154,7 @@ static void ExplainJSONLineEnding(ExplainState *es);
static void ExplainYAMLLineStarting(ExplainState *es);
static void escape_yaml(StringInfo buf, const char *str);
+bool explain_regress = false; /* GUC */
/*
@@ -172,6 +173,7 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
ListCell *lc;
bool timing_set = false;
bool summary_set = false;
+ bool costs_set = false;
/* Parse options list. */
foreach(lc, stmt->options)
@@ -183,7 +185,11 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
else if (strcmp(opt->defname, "verbose") == 0)
es->verbose = defGetBoolean(opt);
else if (strcmp(opt->defname, "costs") == 0)
+ {
+ /* Need to keep track if it was explicitly set to ON */
+ costs_set = true;
es->costs = defGetBoolean(opt);
+ }
else if (strcmp(opt->defname, "buffers") == 0)
es->buffers = defGetBoolean(opt);
else if (strcmp(opt->defname, "wal") == 0)
@@ -227,13 +233,16 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
parser_errposition(pstate, opt->location)));
}
+ /* if the costs option was not set explicitly, set default value */
+ es->costs = (costs_set) ? es->costs : es->costs && !explain_regress;
+
if (es->wal && !es->analyze)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("EXPLAIN option WAL requires ANALYZE")));
/* if the timing was not set explicitly, set default value */
- es->timing = (timing_set) ? es->timing : es->analyze;
+ es->timing = (timing_set) ? es->timing : es->analyze && !explain_regress;
/* check that timing is used with EXPLAIN ANALYZE */
if (es->timing && !es->analyze)
@@ -242,7 +251,7 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
errmsg("EXPLAIN option TIMING
Re: explain_regress, explain(MACHINE), and default to explain(BUFFERS) (was: BUFFERS enabled by default in EXPLAIN (ANALYZE))
On Mon, Jan 24, 2022 at 9:54 AM Justin Pryzby wrote:
> I'm renaming this thread for better visibility, since buffers is a small,
> optional part of the patches I sent.
>
> I made a CF entry here.
> https://commitfest.postgresql.org/36/3409/
>
> On Wed, Dec 01, 2021 at 06:58:20PM -0600, Justin Pryzby wrote:
> > On Mon, Nov 15, 2021 at 01:09:54PM -0600, Justin Pryzby wrote:
> > > Some time ago, I had a few relevant patches:
> > > 1) add explain(REGRESS) which is shorthand for (BUFFERS OFF, TIMING
> OFF, COSTS OFF, SUMMARY OFF)
> > > 2) add explain(MACHINE) which elides machine-specific output from
> explain;
> > >for example, Heap Fetches, sort spaceUsed, hash nbuckets, and
> tidbitmap stuff.
> > >
> > >
> https://www.postgresql.org/message-id/flat/20200306213310.gm...@telsasoft.com
> >
> > The attached patch series now looks like this (some minor patches are not
> > included in this list):
> >
> > 1. add GUC explain_regress, which disables TIMING, SUMMARY, COSTS;
>
> > 2. enable explain(BUFFERS) by default (but disabled by explain_regress);
>
+1
> > 3. Add explain(MACHINE) - which is disabled by explain_regress.
> > This elides various machine-specific output like Memory and Disk use.
> > Maybe it should be called something else like "QUIET" or
> "VERBOSE_MINUS_1"
> > or ??
>
INCIDENTALS ?
Aside from being 4 syllables and, for me at least, a pain to remember how
to spell, it seems to fit.
RUNTIME is probably easier, and we just need to point out the difficulty in
naming things since actual rows is still shown, and timings have their own
independent option.
>
> > The regression tests now automatically run with explain_regress=on,
> which is
> > shorthand for TIMING OFF, SUMMARY OFF, COSTS OFF, and then BUFFERS OFF.
> >
> > There's a further option called explain(MACHINE) which can be disabled
> to hide
> > portions of the output that are unstable, like Memory/Disk/Batches/
> > Heap Fetches/Heap Blocks. This allows "explain analyze" to be used more
> easily
> > in regression tests, and simplifies some existing tests that currently
> use
> > plpgsql functions to filter the output. But it doesn't handle all the
> > variations from parallel workers.
> >
> > (3) is optional, but simplifies some regression tests. The patch series
> could
> > be rephrased with (3) first.
>
I like the idea of encapsulating the logic about what to show into the
generation code instead of a post-processing routine.
I don't see any particular ordering of the three changes being most clear.
>
> > Unfortunately, "COSTS OFF" breaks postgres_fdw remote_estimate. If
> specifying
> > "COSTS ON" in postgres_fdw.c is considered to be a poor fix
Given that we have version tests scattered throughout the postgres_fdw.c
code I'd say protect it with version >= 9.0 and call it good.
But I'm assuming that we are somehow also sending over the GUC to the
remote server (which will be v16+) but I am unsure why that would be the
case. I'm done code reading for now so I'm leaving this an open
request/question.
, then I suppose
> > this patch series could do as suggested and enable buffers by default
> only when
> > ANALYZE is specified. Then postgres_fdw is not affected, and the
> > explain_regress GUC is optional: instead, we'd need to specify BUFFERS
> OFF in
> > ~100 regression tests which use EXPLAIN ANALYZE.
I'm not following the transition from the prior sentences about COSTS to
this one regarding BUFFERS.
(3) still seems useful on its
> > own.
>
It is an orthogonal feature with a different section of our user base being
catered to, so I'd agree by default.
Patch Review Comments:
The following change in the machine commit seems out-of-place; are we
fixing a bug here?
explain.c:
/* Show buffer usage in planning */
- if (bufusage)
+ if (bufusage && es->buffers)
Typo (trailing comment // without comment):
ExplainPropertyFloat("Actual Rows", NULL, rows, 0, es); //
I did a pretty thorough look-through of the locations of the various
changes and everything seems consistent with the established code in this
area (took me a bit to get to the level of comprehension though). Whether
there may be something missing I'm less able to judge.
>From reading the other main thread I'm not finding this particular choice
of GUC usage to be a problem anyone has raised and it does seem the
cleanest solution, both for us and for anyone out there with a similar
testing framework.
The machine output option covers an obvious need since we've already
written ad-hoc parsing code to deal with the problem.
And, as someone who sees first-hand the kinds of conversations that occur
surrounding beginner's questions, and getting more into performance
questions specifically, I'm sympathetic with the desire to have BUFFERS
something that is output by default. Given existing buy-in for that idea
I'd hope that at minimum the "update 100 .sql and expected output files,
then change the default" commit happens even if the rest
Re: explain_regress, explain(MACHINE), and default to explain(BUFFERS) (was: BUFFERS enabled by default in EXPLAIN (ANALYZE))
@cfbot: rebased
>From 099cb8cef38087917a060f86bdb06224d96c3f69 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Sat, 22 Feb 2020 21:17:10 -0600
Subject: [PATCH 1/7] Add GUC: explain_regress
This changes the defaults for explain to: costs off, timing off, summary off.
It'd be reasonable to use this for new regression tests which are not intended
to be backpatched.
---
contrib/postgres_fdw/postgres_fdw.c | 2 +-
src/backend/commands/explain.c | 13 +++--
src/backend/utils/misc/guc.c| 13 +
src/include/commands/explain.h | 2 ++
src/test/regress/expected/explain.out | 3 +++
src/test/regress/expected/inherit.out | 2 +-
src/test/regress/expected/stats_ext.out | 2 +-
src/test/regress/pg_regress.c | 3 ++-
src/test/regress/sql/explain.sql| 4
src/test/regress/sql/inherit.sql| 2 +-
src/test/regress/sql/stats_ext.sql | 2 +-
11 files changed, 40 insertions(+), 8 deletions(-)
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 955a428e3da..dc097f37112 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -3129,7 +3129,7 @@ estimate_path_cost_size(PlannerInfo *root,
* values, so don't request params_list.
*/
initStringInfo();
- appendStringInfoString(, "EXPLAIN ");
+ appendStringInfoString(, "EXPLAIN (COSTS)");
deparseSelectStmtForRel(, root, foreignrel, fdw_scan_tlist,
remote_conds, pathkeys,
fpextra ? fpextra->has_final_sort : false,
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index e29c2ae206f..0bdf1a4080d 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -154,6 +154,7 @@ static void ExplainJSONLineEnding(ExplainState *es);
static void ExplainYAMLLineStarting(ExplainState *es);
static void escape_yaml(StringInfo buf, const char *str);
+bool explain_regress = false; /* GUC */
/*
@@ -172,6 +173,7 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
ListCell *lc;
bool timing_set = false;
bool summary_set = false;
+ bool costs_set = false;
/* Parse options list. */
foreach(lc, stmt->options)
@@ -183,7 +185,11 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
else if (strcmp(opt->defname, "verbose") == 0)
es->verbose = defGetBoolean(opt);
else if (strcmp(opt->defname, "costs") == 0)
+ {
+ /* Need to keep track if it was explicitly set to ON */
+ costs_set = true;
es->costs = defGetBoolean(opt);
+ }
else if (strcmp(opt->defname, "buffers") == 0)
es->buffers = defGetBoolean(opt);
else if (strcmp(opt->defname, "wal") == 0)
@@ -227,13 +233,16 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
parser_errposition(pstate, opt->location)));
}
+ /* if the costs option was not set explicitly, set default value */
+ es->costs = (costs_set) ? es->costs : es->costs && !explain_regress;
+
if (es->wal && !es->analyze)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("EXPLAIN option WAL requires ANALYZE")));
/* if the timing was not set explicitly, set default value */
- es->timing = (timing_set) ? es->timing : es->analyze;
+ es->timing = (timing_set) ? es->timing : es->analyze && !explain_regress;
/* check that timing is used with EXPLAIN ANALYZE */
if (es->timing && !es->analyze)
@@ -242,7 +251,7 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
errmsg("EXPLAIN option TIMING requires ANALYZE")));
/* if the summary was not set explicitly, set default value */
- es->summary = (summary_set) ? es->summary : es->analyze;
+ es->summary = (summary_set) ? es->summary : es->analyze && !explain_regress;
query = castNode(Query, stmt->query);
if (IsQueryIdEnabled())
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 0328029d430..b4d63f2beb9 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -49,6 +49,7 @@
#include "catalog/pg_parameter_acl.h"
#include "catalog/storage.h"
#include "commands/async.h"
+#include "commands/explain.h"
#include "commands/prepare.h"
#include "commands/tablespace.h"
#include "commands/trigger.h"
@@ -1520,6 +1521,18 @@ static struct config_bool ConfigureNamesBool[] =
true,
NULL, NULL, NULL
},
+
+ {
+ {"explain_regress", PGC_USERSET, DEVELOPER_OPTIONS,
+ gettext_noop("Change defaults of EXPLAIN to avoid unstable output."),
+ NULL,
+ GUC_NOT_IN_SAMPLE | GUC_EXPLAIN
+ },
+ _regress,
+ false,
+ NULL, NULL, NULL
+ },
+
{
{"log_parser_stats", PGC_SUSET, STATS_MONITORING,
gettext_noop("Writes parser performance statistics to the server log."),
diff --git a/src/include/commands/explain.h b/src/include/commands/explain.h
index 9ebde089aed..912bc9484ef 100644
--- a/src/include/commands/explain.h
+++ b/src/include/commands/explain.h
@@ -61,6 +61,8 @@ typedef struct ExplainState
ExplainWorkersState
Re: explain_regress, explain(MACHINE), and default to explain(BUFFERS) (was: BUFFERS enabled by default in EXPLAIN (ANALYZE))
On Sat, Feb 26, 2022 at 03:07:20PM -0600, Justin Pryzby wrote:
> Rebased over ebf6c5249b7db525e59563fb149642665c88f747.
> It looks like that patch handles only query_id, and this patch also tries to
> handle a bunch of other stuff.
>
> If it's helpful, feel free to kick this patch to a future CF.
Rebased over MERGE.
>From 058bf205cbac68baa6ff539893d934115d9927f9 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Sat, 22 Feb 2020 21:17:10 -0600
Subject: [PATCH 1/6] Add GUC: explain_regress
This changes the defaults for explain to: costs off, timing off, summary off.
It'd be reasonable to use this for new regression tests which are not intended
to be backpatched.
---
contrib/postgres_fdw/postgres_fdw.c | 2 +-
src/backend/commands/explain.c | 13 +++--
src/backend/utils/misc/guc.c| 13 +
src/bin/psql/describe.c | 2 +-
src/include/commands/explain.h | 2 ++
src/test/regress/expected/explain.out | 3 +++
src/test/regress/expected/inherit.out | 2 +-
src/test/regress/expected/stats_ext.out | 2 +-
src/test/regress/pg_regress.c | 3 ++-
src/test/regress/sql/explain.sql| 4
src/test/regress/sql/inherit.sql| 2 +-
src/test/regress/sql/stats_ext.sql | 2 +-
12 files changed, 41 insertions(+), 9 deletions(-)
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 56654844e8f..54da40d6628 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -3124,7 +3124,7 @@ estimate_path_cost_size(PlannerInfo *root,
* values, so don't request params_list.
*/
initStringInfo();
- appendStringInfoString(, "EXPLAIN ");
+ appendStringInfoString(, "EXPLAIN (COSTS)");
deparseSelectStmtForRel(, root, foreignrel, fdw_scan_tlist,
remote_conds, pathkeys,
fpextra ? fpextra->has_final_sort : false,
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index cb13227db1f..de107ea5026 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -154,6 +154,7 @@ static void ExplainJSONLineEnding(ExplainState *es);
static void ExplainYAMLLineStarting(ExplainState *es);
static void escape_yaml(StringInfo buf, const char *str);
+bool explain_regress = false; /* GUC */
/*
@@ -172,6 +173,7 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
ListCell *lc;
bool timing_set = false;
bool summary_set = false;
+ bool costs_set = false;
/* Parse options list. */
foreach(lc, stmt->options)
@@ -183,7 +185,11 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
else if (strcmp(opt->defname, "verbose") == 0)
es->verbose = defGetBoolean(opt);
else if (strcmp(opt->defname, "costs") == 0)
+ {
+ /* Need to keep track if it was explicitly set to ON */
+ costs_set = true;
es->costs = defGetBoolean(opt);
+ }
else if (strcmp(opt->defname, "buffers") == 0)
es->buffers = defGetBoolean(opt);
else if (strcmp(opt->defname, "wal") == 0)
@@ -227,13 +233,16 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
parser_errposition(pstate, opt->location)));
}
+ /* if the costs option was not set explicitly, set default value */
+ es->costs = (costs_set) ? es->costs : es->costs && !explain_regress;
+
if (es->wal && !es->analyze)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("EXPLAIN option WAL requires ANALYZE")));
/* if the timing was not set explicitly, set default value */
- es->timing = (timing_set) ? es->timing : es->analyze;
+ es->timing = (timing_set) ? es->timing : es->analyze && !explain_regress;
/* check that timing is used with EXPLAIN ANALYZE */
if (es->timing && !es->analyze)
@@ -242,7 +251,7 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
errmsg("EXPLAIN option TIMING requires ANALYZE")));
/* if the summary was not set explicitly, set default value */
- es->summary = (summary_set) ? es->summary : es->analyze;
+ es->summary = (summary_set) ? es->summary : es->analyze && !explain_regress;
query = castNode(Query, stmt->query);
if (IsQueryIdEnabled())
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index eb3a03b9762..acd32fc229c 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -47,6 +47,7 @@
#include "catalog/pg_authid.h"
#include "catalog/storage.h"
#include "commands/async.h"
+#include "commands/explain.h"
#include "commands/prepare.h"
#include "commands/tablespace.h"
#include "commands/trigger.h"
@@ -1478,6 +1479,18 @@ static struct config_bool ConfigureNamesBool[] =
true,
NULL, NULL, NULL
},
+
+ {
+ {"explain_regress", PGC_USERSET, DEVELOPER_OPTIONS,
+ gettext_noop("Change defaults of EXPLAIN to avoid unstable output."),
+ NULL,
+ GUC_NOT_IN_SAMPLE | GUC_EXPLAIN
+ },
+ _regress,
+ false,
+ NULL, NULL, NULL
+ },
+
{
{"log_parser_stats", PGC_SUSET, STATS_MONITORING,
Re: explain_regress, explain(MACHINE), and default to explain(BUFFERS) (was: BUFFERS enabled by default in EXPLAIN (ANALYZE))
Rebased over ebf6c5249b7db525e59563fb149642665c88f747.
It looks like that patch handles only query_id, and this patch also tries to
handle a bunch of other stuff.
If it's helpful, feel free to kick this patch to a future CF.
>From e58fffedc6f1cf471228fb3234faba35898678c3 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Sat, 22 Feb 2020 21:17:10 -0600
Subject: [PATCH 1/6] Add GUC: explain_regress
This changes the defaults for explain to: costs off, timing off, summary off.
It'd be reasonable to use this for new regression tests which are not intended
to be backpatched.
---
contrib/postgres_fdw/postgres_fdw.c | 2 +-
src/backend/commands/explain.c | 13 +++--
src/backend/utils/misc/guc.c| 13 +
src/bin/psql/describe.c | 2 +-
src/include/commands/explain.h | 2 ++
src/test/regress/expected/explain.out | 3 +++
src/test/regress/expected/inherit.out | 2 +-
src/test/regress/expected/stats_ext.out | 2 +-
src/test/regress/pg_regress.c | 3 ++-
src/test/regress/sql/explain.sql| 4
src/test/regress/sql/inherit.sql| 2 +-
src/test/regress/sql/stats_ext.sql | 2 +-
12 files changed, 41 insertions(+), 9 deletions(-)
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 56654844e8..54da40d662 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -3124,7 +3124,7 @@ estimate_path_cost_size(PlannerInfo *root,
* values, so don't request params_list.
*/
initStringInfo();
- appendStringInfoString(, "EXPLAIN ");
+ appendStringInfoString(, "EXPLAIN (COSTS)");
deparseSelectStmtForRel(, root, foreignrel, fdw_scan_tlist,
remote_conds, pathkeys,
fpextra ? fpextra->has_final_sort : false,
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index de81379da3..22a3cf6349 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -154,6 +154,7 @@ static void ExplainJSONLineEnding(ExplainState *es);
static void ExplainYAMLLineStarting(ExplainState *es);
static void escape_yaml(StringInfo buf, const char *str);
+bool explain_regress = false; /* GUC */
/*
@@ -172,6 +173,7 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
ListCell *lc;
bool timing_set = false;
bool summary_set = false;
+ bool costs_set = false;
/* Parse options list. */
foreach(lc, stmt->options)
@@ -183,7 +185,11 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
else if (strcmp(opt->defname, "verbose") == 0)
es->verbose = defGetBoolean(opt);
else if (strcmp(opt->defname, "costs") == 0)
+ {
+ /* Need to keep track if it was explicitly set to ON */
+ costs_set = true;
es->costs = defGetBoolean(opt);
+ }
else if (strcmp(opt->defname, "buffers") == 0)
es->buffers = defGetBoolean(opt);
else if (strcmp(opt->defname, "wal") == 0)
@@ -227,13 +233,16 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
parser_errposition(pstate, opt->location)));
}
+ /* if the costs option was not set explicitly, set default value */
+ es->costs = (costs_set) ? es->costs : es->costs && !explain_regress;
+
if (es->wal && !es->analyze)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("EXPLAIN option WAL requires ANALYZE")));
/* if the timing was not set explicitly, set default value */
- es->timing = (timing_set) ? es->timing : es->analyze;
+ es->timing = (timing_set) ? es->timing : es->analyze && !explain_regress;
/* check that timing is used with EXPLAIN ANALYZE */
if (es->timing && !es->analyze)
@@ -242,7 +251,7 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
errmsg("EXPLAIN option TIMING requires ANALYZE")));
/* if the summary was not set explicitly, set default value */
- es->summary = (summary_set) ? es->summary : es->analyze;
+ es->summary = (summary_set) ? es->summary : es->analyze && !explain_regress;
query = castNode(Query, stmt->query);
if (IsQueryIdEnabled())
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 1e3650184b..87266cf7bd 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -46,6 +46,7 @@
#include "catalog/pg_authid.h"
#include "catalog/storage.h"
#include "commands/async.h"
+#include "commands/explain.h"
#include "commands/prepare.h"
#include "commands/tablespace.h"
#include "commands/trigger.h"
@@ -1474,6 +1475,18 @@ static struct config_bool ConfigureNamesBool[] =
true,
NULL, NULL, NULL
},
+
+ {
+ {"explain_regress", PGC_USERSET, DEVELOPER_OPTIONS,
+ gettext_noop("Change defaults of EXPLAIN to avoid unstable output."),
+ NULL,
+ GUC_NOT_IN_SAMPLE | GUC_EXPLAIN
+ },
+ _regress,
+ false,
+ NULL, NULL, NULL
+ },
+
{
{"log_parser_stats", PGC_SUSET, STATS_MONITORING,
gettext_noop("Writes parser performance statistics to the server log."),
diff --git
explain_regress, explain(MACHINE), and default to explain(BUFFERS) (was: BUFFERS enabled by default in EXPLAIN (ANALYZE))
I'm renaming this thread for better visibility, since buffers is a small,
optional part of the patches I sent.
I made a CF entry here.
https://commitfest.postgresql.org/36/3409/
On Wed, Dec 01, 2021 at 06:58:20PM -0600, Justin Pryzby wrote:
> On Mon, Nov 15, 2021 at 01:09:54PM -0600, Justin Pryzby wrote:
> > Some time ago, I had a few relevant patches:
> > 1) add explain(REGRESS) which is shorthand for (BUFFERS OFF, TIMING OFF,
> > COSTS OFF, SUMMARY OFF)
> > 2) add explain(MACHINE) which elides machine-specific output from explain;
> >for example, Heap Fetches, sort spaceUsed, hash nbuckets, and tidbitmap
> > stuff.
> >
> > https://www.postgresql.org/message-id/flat/20200306213310.gm...@telsasoft.com
>
> The attached patch series now looks like this (some minor patches are not
> included in this list):
>
> 1. add GUC explain_regress, which disables TIMING, SUMMARY, COSTS;
> 2. enable explain(BUFFERS) by default (but disabled by explain_regress);
> 3. Add explain(MACHINE) - which is disabled by explain_regress.
> This elides various machine-specific output like Memory and Disk use.
> Maybe it should be called something else like "QUIET" or "VERBOSE_MINUS_1"
> or ??
>
> The regression tests now automatically run with explain_regress=on, which is
> shorthand for TIMING OFF, SUMMARY OFF, COSTS OFF, and then BUFFERS OFF.
>
> There's a further option called explain(MACHINE) which can be disabled to hide
> portions of the output that are unstable, like Memory/Disk/Batches/
> Heap Fetches/Heap Blocks. This allows "explain analyze" to be used more
> easily
> in regression tests, and simplifies some existing tests that currently use
> plpgsql functions to filter the output. But it doesn't handle all the
> variations from parallel workers.
>
> (3) is optional, but simplifies some regression tests. The patch series could
> be rephrased with (3) first.
>
> Unfortunately, "COSTS OFF" breaks postgres_fdw remote_estimate. If specifying
> "COSTS ON" in postgres_fdw.c is considered to be a poor fix , then I suppose
> this patch series could do as suggested and enable buffers by default only
> when
> ANALYZE is specified. Then postgres_fdw is not affected, and the
> explain_regress GUC is optional: instead, we'd need to specify BUFFERS OFF in
> ~100 regression tests which use EXPLAIN ANALYZE. (3) still seems useful on
> its
> own.
>From 747cb7a979cf96f99403a005053e635f3bf8c3f0 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Sat, 22 Feb 2020 21:17:10 -0600
Subject: [PATCH 1/7] Add GUC: explain_regress
This changes the defaults for explain to: costs off, timing off, summary off.
It'd be reasonable to use this for new regression tests which are not intended
to be backpatched.
---
contrib/postgres_fdw/postgres_fdw.c | 2 +-
src/backend/commands/explain.c | 13 +++--
src/backend/utils/misc/guc.c| 13 +
src/include/commands/explain.h | 2 ++
src/test/regress/expected/explain.out | 3 +++
src/test/regress/expected/inherit.out | 2 +-
src/test/regress/expected/stats_ext.out | 2 +-
src/test/regress/pg_regress.c | 3 ++-
src/test/regress/sql/explain.sql| 4
src/test/regress/sql/inherit.sql| 2 +-
src/test/regress/sql/stats_ext.sql | 2 +-
11 files changed, 40 insertions(+), 8 deletions(-)
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index bf3f3d9e26e..71f5b145984 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -3112,7 +3112,7 @@ estimate_path_cost_size(PlannerInfo *root,
* values, so don't request params_list.
*/
initStringInfo();
- appendStringInfoString(, "EXPLAIN ");
+ appendStringInfoString(, "EXPLAIN (COSTS)");
deparseSelectStmtForRel(, root, foreignrel, fdw_scan_tlist,
remote_conds, pathkeys,
fpextra ? fpextra->has_final_sort : false,
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index b970997c346..43e8bc78778 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -154,6 +154,7 @@ static void ExplainJSONLineEnding(ExplainState *es);
static void ExplainYAMLLineStarting(ExplainState *es);
static void escape_yaml(StringInfo buf, const char *str);
+bool explain_regress = false; /* GUC */
/*
@@ -172,6 +173,7 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
ListCell *lc;
bool timing_set = false;
bool summary_set = false;
+ bool costs_set = false;
/* Parse options list. */
foreach(lc, stmt->options)
@@ -183,7 +185,11 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt,
else if (strcmp(opt->defname, "verbose") == 0)
es->verbose = defGetBoolean(opt);
else if (strcmp(opt->defname, "costs") == 0)
+ {
+ /* Need to keep track if it was explicitly set to ON */
+ costs_set = true;
es->costs = defGetBoolean(opt);
+ }
else if (strcmp(opt->defname, "buffers") ==
