Re: New EXPLAIN option: ALL
On Tue, Jul 02, 2019 at 03:06:52PM +0100, Peter Eisentraut wrote: > On 2019-05-18 19:39, David Fetter wrote: > > On Wed, May 15, 2019 at 09:32:31AM -0400, Tom Lane wrote: > >> David Fetter writes: > >>> I hope the patch is a little easier to digest as now attached. > >> > >> To be blunt, I find 500K worth of changes in the regression test > >> outputs to be absolutely unacceptable, especially when said changes > >> are basically worthless from a diagnostic standpoint. There are at > >> least two reasons why this won't fly: > > > > Here's a patch set with a much smaller change. Will that fly? > > This appears to be the patch of record for this commit fest. > > I don't sense much enthusiasm for this change. Neither do I, so withdrawn. I do hope we can go with EXPLAIN and PROFILE, as opposed to EXPLAIN/EXPLAIN ANALYZE. Best, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Re: New EXPLAIN option: ALL
On 2019-05-18 19:39, David Fetter wrote: > On Wed, May 15, 2019 at 09:32:31AM -0400, Tom Lane wrote: >> David Fetter writes: >>> I hope the patch is a little easier to digest as now attached. >> >> To be blunt, I find 500K worth of changes in the regression test >> outputs to be absolutely unacceptable, especially when said changes >> are basically worthless from a diagnostic standpoint. There are at >> least two reasons why this won't fly: > > Here's a patch set with a much smaller change. Will that fly? This appears to be the patch of record for this commit fest. I don't sense much enthusiasm for this change. What is the exact rationale for this proposal? I think using a new keyword EXEC that is similar to an existing one EXECUTE will likely just introduce a new class of confusion. (ANALYZE EXEC EXECUTE ...?) -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: New EXPLAIN option: ALL
On Wed, Jun 19, 2019 at 02:08:21PM +0200, Daniel Gustafsson wrote: > > On 19 Jun 2019, at 08:15, Peter Eisentraut > > wrote: > > > > On 2019-06-18 23:15, David Fetter wrote: > >> Are you proposing something along the lines of this? > >> > >> PROFILE [statement]; /* Shows the plan */ > >> PROFILE RUN [statement]; /* Actually executes the query */ > > > > No, it would be > > > > EXPLAIN statement; /* Shows the plan */ > > PROFILE statement; /* Actually executes the query */ > > That makes a lot of sense. > > cheers ./daniel +1 Thanks for clarifying. Best, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Re: New EXPLAIN option: ALL
> On 19 Jun 2019, at 08:15, Peter Eisentraut > wrote: > > On 2019-06-18 23:15, David Fetter wrote: >> Are you proposing something along the lines of this? >> >> PROFILE [statement]; /* Shows the plan */ >> PROFILE RUN [statement]; /* Actually executes the query */ > > No, it would be > > EXPLAIN statement; /* Shows the plan */ > PROFILE statement; /* Actually executes the query */ That makes a lot of sense. cheers ./daniel
Re: New EXPLAIN option: ALL
On 19/06/2019 18:15, Peter Eisentraut wrote: On 2019-06-18 23:15, David Fetter wrote: Are you proposing something along the lines of this? PROFILE [statement]; /* Shows the plan */ PROFILE RUN [statement]; /* Actually executes the query */ No, it would be EXPLAIN statement; /* Shows the plan */ PROFILE statement; /* Actually executes the query */ I think that looks good, and the verbs seem well appropriate. IMnsHO
Re: New EXPLAIN option: ALL
On 2019-06-18 23:15, David Fetter wrote: > Are you proposing something along the lines of this? > > PROFILE [statement]; /* Shows the plan */ > PROFILE RUN [statement]; /* Actually executes the query */ No, it would be EXPLAIN statement; /* Shows the plan */ PROFILE statement; /* Actually executes the query */ -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: New EXPLAIN option: ALL
On 2019-05-15 19:58, Andres Freund wrote: > On 2019-05-15 13:53:26 -0400, Tom Lane wrote: >> FWIW, given the conflict against "EXPLAIN EXECUTE prepared_stmt_name", >> we should probably just drop the whole idea. It seemed like a great >> idea at the time, but it's going to confuse people not just Bison. > I'm not particularly invested in the idea of renaming ANALYZE - but I > think we might be able to come up with something less ambiguous than > EXECUTE. Even EXECUTION might be better. The GQL draft uses PROFILE as a separate top-level command, so it would be PROFILE SELECT ... That seems nice and clear. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: New EXPLAIN option: ALL
Andres Freund writes: > On 2019-05-21 19:38:57 +0200, David Fetter wrote: >> On Tue, May 21, 2019 at 12:32:21PM -0400, Robert Haas wrote: >>> Defaulting BUFFERS to ON is probably a reasonable change, thuogh. >> Would this be worth back-patching? I ask because adding it will cause >> fairly large (if mechanical) churn in the regression tests. > This is obviously a no. But I don't even know what large mechanical > churn you're talking about? There's not that many files with EXPLAIN > (ANALYZE) in the tests - we didn't have any until recently, when we > added SUMMARY OFF, to turn off non-deterministic details (f9b1a0dd4). partition_prune.sql has got kind of a lot of them though :-( src/test/regress/sql/tidscan.sql:3 src/test/regress/sql/partition_prune.sql:46 src/test/regress/sql/select_parallel.sql:3 src/test/regress/sql/select.sql:1 src/test/regress/sql/subselect.sql:1 Still, if we're adding BUFFERS OFF in the same places we have SUMMARY OFF, I agree that it won't create much new hazard for back-patching --- all those places already have a limit on how far they can be back-patched. regards, tom lane
Re: New EXPLAIN option: ALL
Hi,
On 2019-05-21 19:38:57 +0200, David Fetter wrote:
> On Tue, May 21, 2019 at 12:32:21PM -0400, Robert Haas wrote:
> > Defaulting BUFFERS to ON is probably a reasonable change, thuogh.
>
> Would this be worth back-patching? I ask because adding it will cause
> fairly large (if mechanical) churn in the regression tests.
This is obviously a no. But I don't even know what large mechanical
churn you're talking about? There's not that many files with EXPLAIN
(ANALYZE) in the tests - we didn't have any until recently, when we
added SUMMARY OFF, to turn off non-deterministic details (f9b1a0dd4).
$ grep -irl 'summary off' src/test/regress/{sql,input}
src/test/regress/sql/select.sql
src/test/regress/sql/partition_prune.sql
src/test/regress/sql/tidscan.sql
src/test/regress/sql/subselect.sql
src/test/regress/sql/select_parallel.sql
adding a bunch of BUFFERS OFF to those wouldn't be particularly
painful. And if we decided it somehow were painful, we could infer it
from COSTS or such.
Greetings,
Andres Freund
Re: New EXPLAIN option: ALL
On Tue, May 21, 2019 at 1:38 PM David Fetter wrote: > Would this be worth back-patching? I ask because adding it will cause > fairly large (if mechanical) churn in the regression tests. No. I can't believe you're even asking that question. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: New EXPLAIN option: ALL
David Fetter writes: > On Tue, May 21, 2019 at 12:32:21PM -0400, Robert Haas wrote: >> Defaulting BUFFERS to ON is probably a reasonable change, thuogh. > Would this be worth back-patching? I ask because adding it will cause > fairly large (if mechanical) churn in the regression tests. It really doesn't matter how much churn it causes in the regression tests. Back-patching a significant non-bug behavioral change like that is exactly the kind of thing we don't do, because it will cause our users pain. regards, tom lane
Re: New EXPLAIN option: ALL
On Tue, May 21, 2019 at 12:32:21PM -0400, Robert Haas wrote: > On Wed, May 15, 2019 at 1:53 PM Tom Lane wrote: > > >> That seems too confusing. > > > > > Ok. Are you voting for using EXEC as a keyword to replace ANALYZE? > > > > FWIW, given the conflict against "EXPLAIN EXECUTE prepared_stmt_name", > > we should probably just drop the whole idea. It seemed like a great > > idea at the time, but it's going to confuse people not just Bison. > > +1. I think trying to replace ANALYZE with something else is setting > ourselves up for years, possibly decades, worth of confusion. And > without any real benefit. > > Defaulting BUFFERS to ON is probably a reasonable change, thuogh. Would this be worth back-patching? I ask because adding it will cause fairly large (if mechanical) churn in the regression tests. Best, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Re: New EXPLAIN option: ALL
On Wed, May 15, 2019 at 1:53 PM Tom Lane wrote: > >> That seems too confusing. > > > Ok. Are you voting for using EXEC as a keyword to replace ANALYZE? > > FWIW, given the conflict against "EXPLAIN EXECUTE prepared_stmt_name", > we should probably just drop the whole idea. It seemed like a great > idea at the time, but it's going to confuse people not just Bison. +1. I think trying to replace ANALYZE with something else is setting ourselves up for years, possibly decades, worth of confusion. And without any real benefit. Defaulting BUFFERS to ON is probably a reasonable change, thuogh. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: New EXPLAIN option: ALL
On Wed, May 15, 2019 at 09:32:31AM -0400, Tom Lane wrote:
> David Fetter writes:
> > I hope the patch is a little easier to digest as now attached.
>
> To be blunt, I find 500K worth of changes in the regression test
> outputs to be absolutely unacceptable, especially when said changes
> are basically worthless from a diagnostic standpoint. There are at
> least two reasons why this won't fly:
Here's a patch set with a much smaller change. Will that fly?
Best,
David.
--
David Fetter http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
>From be95e9296a95eb5e6f19fe4aacd4df4ebc3fe533 Mon Sep 17 00:00:00 2001
From: David Fetter
Date: Sat, 18 May 2019 10:22:33 -0700
Subject: [PATCH v5 1/8] Changed EXPLAIN ANALYZE to EXPLAIN (EXEC)
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="2.21.0"
This is a multi-part message in MIME format.
--2.21.0
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index a6c6de78f1..8088926174 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -155,8 +155,8 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt, const char *queryString,
{
DefElem*opt = (DefElem *) lfirst(lc);
- if (strcmp(opt->defname, "analyze") == 0)
- es->analyze = defGetBoolean(opt);
+ if (strcmp(opt->defname, "exec") == 0)
+ es->exec = defGetBoolean(opt);
else if (strcmp(opt->defname, "verbose") == 0)
es->verbose = defGetBoolean(opt);
else if (strcmp(opt->defname, "costs") == 0)
@@ -202,22 +202,22 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt, const char *queryString,
parser_errposition(pstate, opt->location)));
}
- if (es->buffers && !es->analyze)
+ if (es->buffers && !es->exec)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("EXPLAIN option BUFFERS requires ANALYZE")));
+ errmsg("EXPLAIN option BUFFERS requires EXEC")));
/* 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->exec;
- /* check that timing is used with EXPLAIN ANALYZE */
- if (es->timing && !es->analyze)
+ /* check that timing is used with EXPLAIN EXEC */
+ if (es->timing && !es->exec)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("EXPLAIN option TIMING requires ANALYZE")));
+ errmsg("EXPLAIN option TIMING requires EXEC")));
/* 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->exec;
/*
* Parse analysis was done already, but we still have to run the rule
@@ -475,9 +475,9 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es,
Assert(plannedstmt->commandType != CMD_UTILITY);
- if (es->analyze && es->timing)
+ if (es->exec && es->timing)
instrument_option |= INSTRUMENT_TIMER;
- else if (es->analyze)
+ else if (es->exec)
instrument_option |= INSTRUMENT_ROWS;
if (es->buffers)
@@ -512,7 +512,7 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es,
dest, params, queryEnv, instrument_option);
/* Select execution options */
- if (es->analyze)
+ if (es->exec)
eflags = 0;/* default run-to-completion flags */
else
eflags = EXEC_FLAG_EXPLAIN_ONLY;
@@ -523,7 +523,7 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es,
ExecutorStart(queryDesc, eflags);
/* Execute the plan for statistics if asked for */
- if (es->analyze)
+ if (es->exec)
{
ScanDirection dir;
@@ -556,7 +556,7 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es,
}
/* Print info about runtime of triggers */
- if (es->analyze)
+ if (es->exec)
ExplainPrintTriggers(es, queryDesc);
/*
@@ -581,7 +581,7 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es,
PopActiveSnapshot();
/* We need a CCI just in case query expanded to multiple plans */
- if (es->analyze)
+ if (es->exec)
CommandCounterIncrement();
totaltime += elapsed_time();
@@ -592,7 +592,7 @@ ExplainOnePlan(PlannedStmt *plannedstmt, IntoClause *into, ExplainState *es,
* user can set SUMMARY OFF to not have the timing information included in
* the output). By default, ANALYZE sets SUMMARY to true.
*/
- if (es->summary && es->analyze)
+ if (es->summary && es->exec)
ExplainPropertyFloat("Execution Time", "ms", 1000.0 * totaltime, 3,
es);
@@ -834,7 +834,7 @@ ExplainPrintJIT(ExplainState *es, int jit_flags,
"Expressions", jit_flags & PGJIT_EXPR ? "true" : "false",
"Deforming", jit_flags & PGJIT_DEFORM ? "true" : "false");
- if (es->analyze && es->timing)
+ if (es->exec &&
Re: New EXPLAIN option: ALL
On Wed, May 15, 2019 at 10:46:39AM -0400, Tom Lane wrote: > Andres Freund writes: >> On May 15, 2019 7:20:34 AM PDT, David Fetter wrote: >>> Indeed. I think we should move our regression tests to TAP and >>> dispense with this. > >> -inconceivably much > > Yeah, that's not happening. +1 to the we-shall-not-move part. -- Michael signature.asc Description: PGP signature
Re: New EXPLAIN option: ALL
Hi,
On 2019-05-15 13:53:26 -0400, Tom Lane wrote:
> FWIW, given the conflict against "EXPLAIN EXECUTE prepared_stmt_name",
> we should probably just drop the whole idea. It seemed like a great
> idea at the time, but it's going to confuse people not just Bison.
I'm not particularly invested in the idea of renaming ANALYZE - but I
think we might be able to come up with something less ambiguous than
EXECUTE. Even EXECUTION might be better.
> So ... never mind that suggestion. Can we get anywhere with the
> rest of it?
Yes, please. I still think getting rid of
if (es->buffers && !es->analyze)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("EXPLAIN option BUFFERS requires
ANALYZE")));
and
/* check that timing is used with EXPLAIN ANALYZE */
if (es->timing && !es->analyze)
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("EXPLAIN option TIMING requires
ANALYZE")));
and then changing the default for BUFFERs would be good. I assume they'd
still only apply to query execution.
Althouh, in the case of BUFFERS, I more than once wished we'd track the
plan-time stats for buffers as well. But that's a significantly more
complicated change.
Greetings,
Andres Freund
Re: New EXPLAIN option: ALL
Alvaro Herrera writes: > On 2019-May-15, Andres Freund wrote: >> On 2019-05-15 11:05:31 -0400, Alvaro Herrera wrote: >>> After eyeballing the giant patch set you sent[1], I think EXEC is a >>> horrible keyword to use -- IMO it should either be the complete word >>> EXECUTE, or we should pick some other word. I realize that we do not >>> want to have different sets of keywords when using the legacy syntax (no >>> parens) vs. new-style (with parens), but maybe we should just not >>> support the EXECUTE keyword in the legacy syntax; there's already a >>> number of options we don't support in the legacy syntax (BUFFERS, >>> TIMING), so this isn't much of a stretch. >> That seems too confusing. > Ok. Are you voting for using EXEC as a keyword to replace ANALYZE? FWIW, given the conflict against "EXPLAIN EXECUTE prepared_stmt_name", we should probably just drop the whole idea. It seemed like a great idea at the time, but it's going to confuse people not just Bison. This is such a fundamental option that it doesn't make sense to not have it available in the simplified syntax. It also doesn't make sense to use different names for it in the simplified and extended syntaxes. And "EXEC", or other weird spellings, is in the end not an improvement on "ANALYZE". So ... never mind that suggestion. Can we get anywhere with the rest of it? regards, tom lane
Re: New EXPLAIN option: ALL
Hello, On 2019-May-15, Andres Freund wrote: > On 2019-05-15 11:05:31 -0400, Alvaro Herrera wrote: > > After eyeballing the giant patch set you sent[1], I think EXEC is a > > horrible keyword to use -- IMO it should either be the complete word > > EXECUTE, or we should pick some other word. I realize that we do not > > want to have different sets of keywords when using the legacy syntax (no > > parens) vs. new-style (with parens), but maybe we should just not > > support the EXECUTE keyword in the legacy syntax; there's already a > > number of options we don't support in the legacy syntax (BUFFERS, > > TIMING), so this isn't much of a stretch. > > That seems too confusing. Ok. Are you voting for using EXEC as a keyword to replace ANALYZE? -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: New EXPLAIN option: ALL
Hi, On 2019-05-15 11:05:31 -0400, Alvaro Herrera wrote: > After eyeballing the giant patch set you sent[1], I think EXEC is a > horrible keyword to use -- IMO it should either be the complete word > EXECUTE, or we should pick some other word. I realize that we do not > want to have different sets of keywords when using the legacy syntax (no > parens) vs. new-style (with parens), but maybe we should just not > support the EXECUTE keyword in the legacy syntax; there's already a > number of options we don't support in the legacy syntax (BUFFERS, > TIMING), so this isn't much of a stretch. That seems too confusing. > [1] I think if you just leave out the GUC print from the changes, it > becomes a reasonable patch series. Yea, it really should be small incremental changes instead of proposals to "just change everything". Greetings, Andres Freund
Re: New EXPLAIN option: ALL
On 2019-May-13, David Fetter wrote: > I tried changing it to EXEC (EXPLAIN EXECUTE is already a thing), but > got a giant flock of reduce-reduce conflicts along with a few > shift-reduce conflicts. After eyeballing the giant patch set you sent[1], I think EXEC is a horrible keyword to use -- IMO it should either be the complete word EXECUTE, or we should pick some other word. I realize that we do not want to have different sets of keywords when using the legacy syntax (no parens) vs. new-style (with parens), but maybe we should just not support the EXECUTE keyword in the legacy syntax; there's already a number of options we don't support in the legacy syntax (BUFFERS, TIMING), so this isn't much of a stretch. IOW if we want to change ANALYZE to EXECUTE, I propose we change it in the new-style syntax only and not the legacy one. So: EXPLAIN ANALYZE SELECT ... -- legacy syntax EXPLAIN (EXECUTE) SELECT ...-- new-style EXPLAIN (ANALYZE) SELECT ...-- we still support ANALYZE as an alias, for compatibility this should not cause a conflict with EXPLAIN EXECUTE, so these all should work: EXPLAIN ANALYZE EXECUTE ... EXPLAIN (EXECUTE) EXECUTE ... EXPLAIN (ANALYZE) EXECUTE ... [1] I think if you just leave out the GUC print from the changes, it becomes a reasonable patch series. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: New EXPLAIN option: ALL
Andres Freund writes: > On May 15, 2019 7:20:34 AM PDT, David Fetter wrote: >> Indeed. I think we should move our regression tests to TAP and >> dispense with this. > -inconceivably much Yeah, that's not happening. Just eyeing the patch again, it seems like most of the test-output churn is from a decision to make printing of planner options be on-by-default; which is also what creates the buildfarm-variant-options hazard. So I suggest reconsidering that. TBH, even without the regression test angle, I suspect that such a change would receive a lot of pushback. It's a pretty big delta in the verbosity of EXPLAIN, and it is frankly of no value to a lot of people a lot of the time. regards, tom lane
Re: New EXPLAIN option: ALL
Hi, On May 15, 2019 7:20:34 AM PDT, David Fetter wrote: >On Wed, May 15, 2019 at 09:32:31AM -0400, Tom Lane wrote: >> David Fetter writes: >> > I hope the patch is a little easier to digest as now attached. >> >> To be blunt, I find 500K worth of changes in the regression test >> outputs to be absolutely unacceptable, especially when said changes >> are basically worthless from a diagnostic standpoint. > >You're right, of course. The fundamental problem is that our >regression tests depend on (small sets of) fixed strings. TAP is an >alternative, and could test the structure of the output rather than >what really should be completely inconsequential changes in its form. >> There are >> at least two reasons why this won't fly: >> >> * Such a change would be a serious obstacle to back-patching >> regression test cases that involve explain output. >> >> * Some buildfarm members use nonstandard settings (notably >> force_parallel_mode, but I don't think that's the only one). >> We are *not* going to maintain variant output files to try to cope >> with all those combinations. It'd be even more disastrous for >> private forks that might have their own affected settings. > >Indeed. I think we should move our regression tests to TAP and >dispense with this. -inconceivably much The effort to write tap tests over our main tests is much much higher. And they're usually much slower. Of course tap is more powerful, so it's good to have the option. And it'd be many months if not years worth of work, and would make backpatching much harder. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: New EXPLAIN option: ALL
On Wed, May 15, 2019 at 09:32:31AM -0400, Tom Lane wrote: > David Fetter writes: > > I hope the patch is a little easier to digest as now attached. > > To be blunt, I find 500K worth of changes in the regression test > outputs to be absolutely unacceptable, especially when said changes > are basically worthless from a diagnostic standpoint. You're right, of course. The fundamental problem is that our regression tests depend on (small sets of) fixed strings. TAP is an alternative, and could test the structure of the output rather than what really should be completely inconsequential changes in its form. > There are > at least two reasons why this won't fly: > > * Such a change would be a serious obstacle to back-patching > regression test cases that involve explain output. > > * Some buildfarm members use nonstandard settings (notably > force_parallel_mode, but I don't think that's the only one). > We are *not* going to maintain variant output files to try to cope > with all those combinations. It'd be even more disastrous for > private forks that might have their own affected settings. Indeed. I think we should move our regression tests to TAP and dispense with this. > I don't know how to make progress towards the original goal without > having a regression-test disaster, but what we have here is one. This just highlights a disaster already in progress. I'm volunteering to help fix it. Best, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Re: New EXPLAIN option: ALL
David Fetter writes: > I hope the patch is a little easier to digest as now attached. To be blunt, I find 500K worth of changes in the regression test outputs to be absolutely unacceptable, especially when said changes are basically worthless from a diagnostic standpoint. There are at least two reasons why this won't fly: * Such a change would be a serious obstacle to back-patching regression test cases that involve explain output. * Some buildfarm members use nonstandard settings (notably force_parallel_mode, but I don't think that's the only one). We are *not* going to maintain variant output files to try to cope with all those combinations. It'd be even more disastrous for private forks that might have their own affected settings. I don't know how to make progress towards the original goal without having a regression-test disaster, but what we have here is one. regards, tom lane
Re: New EXPLAIN option: ALL
On Tue, May 07, 2019 at 06:25:12PM -0400, Tom Lane wrote: > Stephen Frost writes: > > I'm generally in favor of doing something like what Tom is suggesting > > with VERBOSE, but I also feel like it should be the default for formats > > like JSON. If you're asking for the output in JSON, then we really > > should include everything that a flag like VERBOSE would contain because > > you're pretty clearly planning to copy/paste that output into something > > else to read it anyway. > > Meh --- I don't especially care for non-orthogonal behaviors like that. > If you wanted JSON but *not* all of the additional info, how would you > specify that? (The implementation I had in mind would make VERBOSE OFF > more or less a no-op, so that wouldn't get you there.) > > >> I do feel that it's a good idea to keep ANALYZE separate. "Execute > >> the query or not" is a mighty fundamental thing. I've never liked > >> that name for the option though --- maybe we could deprecate it > >> in favor of EXECUTE? > > > Let's not fool ourselves by saying we'd 'deprecate' it because that > > implies, at least to me, that there's some intention of later on > > removing it > > True, the odds of ever actually removing it are small :-(. I meant > mostly changing all of our docs to use the other spelling, except > for some footnote. Maybe we could call ANALYZE a "legacy spelling" > of EXECUTE. I tried changing it to EXEC (EXPLAIN EXECUTE is already a thing), but got a giant flock of reduce-reduce conflicts along with a few shift-reduce conflicts. How do I fix this? Best, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate >From c8cdcf6ea0ee25b7159898a09210fc343b14e5e5 Mon Sep 17 00:00:00 2001 From: David Fetter Date: Sun, 12 May 2019 21:48:09 -0700 Subject: [PATCH v1] WIP (broken): Changed EXPLAIN ANALYZE to EXPLAIN EXEC To: hackers MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="2.21.0" This is a multi-part message in MIME format. --2.21.0 Content-Type: text/plain; charset=UTF-8; format=fixed Content-Transfer-Encoding: 8bit - In passing, changed a few of EXPLAIN's defaults diff --git a/doc/src/sgml/ref/explain.sgml b/doc/src/sgml/ref/explain.sgml index 385d10411f..63a3faede8 100644 --- a/doc/src/sgml/ref/explain.sgml +++ b/doc/src/sgml/ref/explain.sgml @@ -32,11 +32,11 @@ PostgreSQL documentation EXPLAIN [ ( option [, ...] ) ] statement -EXPLAIN [ ANALYZE ] [ VERBOSE ] statement +EXPLAIN [ EXEC ] [ VERBOSE ] statement where option can be one of: -ANALYZE [ boolean ] +EXEC [ boolean ] VERBOSE [ boolean ] COSTS [ boolean ] SETTINGS [ boolean ] @@ -76,36 +76,37 @@ EXPLAIN [ ANALYZE ] [ VERBOSE ] statement - The ANALYZE option causes the statement to be actually + The EXEC option causes the statement to be actually executed, not only planned. Then actual run time statistics are added to the display, including the total elapsed time expended within each plan node (in milliseconds) and the total number of rows it actually returned. This is useful for seeing whether the planner's estimates - are close to reality. + are close to reality. For historical reasons, ANALYZE and ANALYSE can be + used instead of EXEC. Keep in mind that the statement is actually executed when -the ANALYZE option is used. Although +the EXEC option is used. Although EXPLAIN will discard any output that a SELECT would return, other side effects of the statement will happen as usual. If you wish to use -EXPLAIN ANALYZE on an +EXPLAIN EXEC on an INSERT, UPDATE, DELETE, CREATE TABLE AS, or EXECUTE statement without letting the command affect your data, use this approach: BEGIN; -EXPLAIN ANALYZE ...; +EXPLAIN EXEC ...; ROLLBACK; - Only the ANALYZE and VERBOSE options + Only the EXEC and VERBOSE options can be specified, and only in that order, without surrounding the option list in parentheses. Prior to PostgreSQL 9.0, the unparenthesized syntax was the only one supported. It is expected that @@ -118,7 +119,7 @@ ROLLBACK; -ANALYZE +EXEC Carry out the command and show actual run times and other statistics. @@ -159,7 +160,7 @@ ROLLBACK; Include information on configuration parameters. Specifically, include options affecting query planning with value different from the built-in - default value. This parameter defaults to FALSE. + default value. This parameter defaults to TRUE. @@ -186,8 +187,8 @@ ROLLBACK; The number of blocks shown for an upper-level node includes those used by all its child nodes. In text format, only non-zero values are printed. This parameter may only be - used when ANALYZE is also enabled. It defaults to -
Re: New EXPLAIN option: ALL
> On May 8, 2019, at 4:22 PM, Vik Fearing wrote: > > On 07/05/2019 09:30, David Fetter wrote: >> Folks, >> >> It can get a little tedious turning on (or off) all the boolean >> options to EXPLAIN, so please find attached a shortcut. > > I would rather have a set of gucs such as default_explain_buffers, > default_explain_summary, and default_explain_format. > > Of course if you default BUFFERS to on(*) and don't do ANALYZE, that > should not result in an error. > > (*) Defaulting BUFFERS to on is something I want regardless of anything > else we do. I think this, plus Tom’s suggesting of changing what VERBOSE does, is the best way to handle this. Especially since VERBOSE is IMHO pretty useless... I’m +1 on trying to move away from ANALYZE as well, though I think it’s mostly orthogonal...
Re: New EXPLAIN option: ALL
On 07/05/2019 09:30, David Fetter wrote: > Folks, > > It can get a little tedious turning on (or off) all the boolean > options to EXPLAIN, so please find attached a shortcut. I would rather have a set of gucs such as default_explain_buffers, default_explain_summary, and default_explain_format. Of course if you default BUFFERS to on(*) and don't do ANALYZE, that should not result in an error. (*) Defaulting BUFFERS to on is something I want regardless of anything else we do. -- Vik Fearing +33 6 46 75 15 36 http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
Re: New EXPLAIN option: ALL
On Tue, May 7, 2019 at 12:31 PM David Fetter wrote: > If you're tuning a query interactively, it's a lot simpler to prepend, > for example, > > EXPLAIN (ALL, FORMAT JSON) > > to it than to prepend something along the lines of > > EXPLAIN(ANALYZE, VERBOSE, COSTS, BUFFERS, SETTINGS, TIMING, SUMMARY, > PARTRIDGE_IN_A_PEAR_TREE, FORMAT JSON) > > to it. This is something of an exaggeration of what could ever be necessary, because COSTS and TIMING default to TRUE and SUMMARY defaults to TRUE when ANALYZE is specified, and the PARTRIDGE_IN_A_PEAR_TREE option seems not to have made it into the tree this cycle. But you could need EXPLAIN (ANALYZE, VERBOSE, BUFFERS, SETTINGS, FORMAT JSON), which is not quite so long, but admittedly still somewhat long. Flipping some of the defaults seems like it might be the way to go. I think turning SETTINGS and BUFFERS on by default would be pretty sensible. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: New EXPLAIN option: ALL
On Tue, May 7, 2019 at 6:25 PM Tom Lane wrote: > Meh --- I don't especially care for non-orthogonal behaviors like that. > If you wanted JSON but *not* all of the additional info, how would you > specify that? (The implementation I had in mind would make VERBOSE OFF > more or less a no-op, so that wouldn't get you there.) +1. Assuming we know which information the user wants on the basis of their choice of output format seems like a bad idea. I mean, suppose we introduced a new option that gathered lots of additional detail but made the query run 3x slower. Would everyone want that enabled all the time any time they chose a non-text format? Probably not. If people want BUFFERS turned on essentially all the time, then let's just flip the default for that, so that EXPLAIN ANALYZE does the equivalent of what EXPLAIN (ANALYZE, BUFFERS) currently does, and make people say EXPLAIN (ANALYZE, BUFFERS OFF) if they don't want all that detail. I think that's more or less what Andres was suggesting upthread. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: New EXPLAIN option: ALL
Greetings, * Tom Lane (t...@sss.pgh.pa.us) wrote: > Stephen Frost writes: > > I'm generally in favor of doing something like what Tom is suggesting > > with VERBOSE, but I also feel like it should be the default for formats > > like JSON. If you're asking for the output in JSON, then we really > > should include everything that a flag like VERBOSE would contain because > > you're pretty clearly planning to copy/paste that output into something > > else to read it anyway. > > Meh --- I don't especially care for non-orthogonal behaviors like that. > If you wanted JSON but *not* all of the additional info, how would you > specify that? (The implementation I had in mind would make VERBOSE OFF > more or less a no-op, so that wouldn't get you there.) You'd do it the same way you proposed for verbose- eg: BUFFERS OFF, et al, but, really, the point here is that what you're doing with the JSON result is fundamentally different- you're going to paste it into some other tool and it should be that tool's job to manage the visualization of it and what's included or not in what you see. Passing the information about what should be seen in the json-based EXPLAIN viewer by way of omitting things from the JSON output strikes me as downright odd, and doesn't give that other tool the ability to show that data if the users ends up wanting it without rerunning the query. > >> I do feel that it's a good idea to keep ANALYZE separate. "Execute > >> the query or not" is a mighty fundamental thing. I've never liked > >> that name for the option though --- maybe we could deprecate it > >> in favor of EXECUTE? > > > Let's not fool ourselves by saying we'd 'deprecate' it because that > > implies, at least to me, that there's some intention of later on > > removing it > > True, the odds of ever actually removing it are small :-(. I meant > mostly changing all of our docs to use the other spelling, except > for some footnote. Maybe we could call ANALYZE a "legacy spelling" > of EXECUTE. Sure, that'd be fine too. Thanks, Stephen signature.asc Description: PGP signature
Re: New EXPLAIN option: ALL
Greetings, * David Fetter (da...@fetter.org) wrote: > On Tue, May 07, 2019 at 06:12:56PM -0400, Stephen Frost wrote: > > * Tom Lane (t...@sss.pgh.pa.us) wrote: > > > One idea that comes to mind is that VERBOSE could be redefined as > > > some sort of package of primitive options, including all of the > > > "additional information" options, with the ability to turn > > > individual ones off again if you wanted. So for example (VERBOSE, > > > BUFFERS OFF) would give you everything except buffer stats. We'd > > > need a separate flag/flags to control what VERBOSE originally did, > > > but that doesn't bother me --- it's an opportunity for more > > > clarity of definition, anyway. > > > > I'm generally in favor of doing something like what Tom is > > suggesting with VERBOSE, but I also feel like it should be the > > default for formats like JSON. If you're asking for the output in > > JSON, then we really should include everything that a flag like > > VERBOSE would contain because you're pretty clearly planning to > > copy/paste that output into something else to read it anyway. > > So basically, every format but text gets the full treatment for > (essentially) the functionality I wrapped up in ALL? That makes a lot > of sense. Something along those lines is what I was thinking, yes, and it's what at least some other projects do (admittedly, that's at least partially my fault because I'm thinking of the 'info' command for pgbackrest, but David Steele seemed to think it made sense also, at least). > > I'm all in favor of adding an alias for analyze called 'execute', as > > that makes a lot more sense and then updating our documentation to > > use it, with 'analyze is accepted as an alias' as a footnote. > > How about making ANALYZE a backward-compatibility feature in the sense > of replacing examples, docs, etc., with EXECUTE? If most of our users > are in the future, this makes those same users's better without > qualification, and helps some positive fraction of our current users. I'd rather not refer to it as a backwards-compatibility feature since we, thankfully, don't typically do that and I generally think that's the right way to go- but in some cases, like this one, having a 'legacy' spelling or an alias seems to be darn near free without opening the box of trying to provide backwards compatibility for everything. > On a slightly related topic, we haven't, to date, made any promises > about what EXPLAIN will put out, but as we make more machine-readable > versions, we should at least think about its schema and versioning > of same. Not really sure that I agree on this point. Do you see a reason to need versioning or schema when the schema is, essentially, included in each result since it's JSON or the other machine-readable formats? I can imagine that we might need a version if we decided to redefine some existing field in a non-compatible or non-sensible way, but is that possibility likely enough to warrent adding versioning and complicating everything downstream? I have a hard time seeing that. Thanks, Stephen signature.asc Description: PGP signature
Re: New EXPLAIN option: ALL
On Tue, May 07, 2019 at 06:12:56PM -0400, Stephen Frost wrote: > Greetings, > > * Tom Lane (t...@sss.pgh.pa.us) wrote: > > > > One idea that comes to mind is that VERBOSE could be redefined as > > some sort of package of primitive options, including all of the > > "additional information" options, with the ability to turn > > individual ones off again if you wanted. So for example (VERBOSE, > > BUFFERS OFF) would give you everything except buffer stats. We'd > > need a separate flag/flags to control what VERBOSE originally did, > > but that doesn't bother me --- it's an opportunity for more > > clarity of definition, anyway. > > I'm generally in favor of doing something like what Tom is > suggesting with VERBOSE, but I also feel like it should be the > default for formats like JSON. If you're asking for the output in > JSON, then we really should include everything that a flag like > VERBOSE would contain because you're pretty clearly planning to > copy/paste that output into something else to read it anyway. So basically, every format but text gets the full treatment for (essentially) the functionality I wrapped up in ALL? That makes a lot of sense. > > I do feel that it's a good idea to keep ANALYZE separate. "Execute > > the query or not" is a mighty fundamental thing. I've never liked > > that name for the option though --- maybe we could deprecate it > > in favor of EXECUTE? > > Let's not fool ourselves by saying we'd 'deprecate' it because that > implies, at least to me, that there's some intention of later on > removing it and people will potentially propose patches to do that, > which we will then almost certainly spend hours arguing about with the > result being that we don't actually remove it. Excellent point. > I'm all in favor of adding an alias for analyze called 'execute', as > that makes a lot more sense and then updating our documentation to > use it, with 'analyze is accepted as an alias' as a footnote. How about making ANALYZE a backward-compatibility feature in the sense of replacing examples, docs, etc., with EXECUTE? If most of our users are in the future, this makes those same users's better without qualification, and helps some positive fraction of our current users. On a slightly related topic, we haven't, to date, made any promises about what EXPLAIN will put out, but as we make more machine-readable versions, we should at least think about its schema and versioning of same. Best, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Re: New EXPLAIN option: ALL
Stephen Frost writes: > I'm generally in favor of doing something like what Tom is suggesting > with VERBOSE, but I also feel like it should be the default for formats > like JSON. If you're asking for the output in JSON, then we really > should include everything that a flag like VERBOSE would contain because > you're pretty clearly planning to copy/paste that output into something > else to read it anyway. Meh --- I don't especially care for non-orthogonal behaviors like that. If you wanted JSON but *not* all of the additional info, how would you specify that? (The implementation I had in mind would make VERBOSE OFF more or less a no-op, so that wouldn't get you there.) >> I do feel that it's a good idea to keep ANALYZE separate. "Execute >> the query or not" is a mighty fundamental thing. I've never liked >> that name for the option though --- maybe we could deprecate it >> in favor of EXECUTE? > Let's not fool ourselves by saying we'd 'deprecate' it because that > implies, at least to me, that there's some intention of later on > removing it True, the odds of ever actually removing it are small :-(. I meant mostly changing all of our docs to use the other spelling, except for some footnote. Maybe we could call ANALYZE a "legacy spelling" of EXECUTE. regards, tom lane
Re: New EXPLAIN option: ALL
Greetings, * Tom Lane (t...@sss.pgh.pa.us) wrote: > Andres Freund writes: > > As I said, I don't think ALL is a good idea under any name. Like it > > just makes no sense to have ANALYZE, SUMMARY, VERBOSE, BUFFERS, > > SETTINGS, FORMAT controlled by one option, unless you call it DWIM. It's > > several separate axis (query is executed or not (ANALYZE), verbosity > > (SUMMARY, VERBOSE), collecting additional information (BUFFERS, TIMING), > > output format). > > FWIW, I find this line of argument fairly convincing. There may well > be a case for rethinking just how EXPLAIN's options behave, but "ALL" > doesn't seem like a good conceptual model. > > One idea that comes to mind is that VERBOSE could be redefined as some > sort of package of primitive options, including all of the "additional > information" options, with the ability to turn individual ones off again > if you wanted. So for example (VERBOSE, BUFFERS OFF) would give you > everything except buffer stats. We'd need a separate flag/flags to > control what VERBOSE originally did, but that doesn't bother me --- > it's an opportunity for more clarity of definition, anyway. I'm generally in favor of doing something like what Tom is suggesting with VERBOSE, but I also feel like it should be the default for formats like JSON. If you're asking for the output in JSON, then we really should include everything that a flag like VERBOSE would contain because you're pretty clearly planning to copy/paste that output into something else to read it anyway. > I do feel that it's a good idea to keep ANALYZE separate. "Execute > the query or not" is a mighty fundamental thing. I've never liked > that name for the option though --- maybe we could deprecate it > in favor of EXECUTE? Let's not fool ourselves by saying we'd 'deprecate' it because that implies, at least to me, that there's some intention of later on removing it and people will potentially propose patches to do that, which we will then almost certainly spend hours arguing about with the result being that we don't actually remove it. I'm all in favor of adding an alias for analyze called 'execute', as that makes a lot more sense and then updating our documentation to use it, with 'analyze is accepted as an alias' as a footnote. Thanks, Stephen signature.asc Description: PGP signature
Re: New EXPLAIN option: ALL
Andres Freund writes: > As I said, I don't think ALL is a good idea under any name. Like it > just makes no sense to have ANALYZE, SUMMARY, VERBOSE, BUFFERS, > SETTINGS, FORMAT controlled by one option, unless you call it DWIM. It's > several separate axis (query is executed or not (ANALYZE), verbosity > (SUMMARY, VERBOSE), collecting additional information (BUFFERS, TIMING), > output format). FWIW, I find this line of argument fairly convincing. There may well be a case for rethinking just how EXPLAIN's options behave, but "ALL" doesn't seem like a good conceptual model. One idea that comes to mind is that VERBOSE could be redefined as some sort of package of primitive options, including all of the "additional information" options, with the ability to turn individual ones off again if you wanted. So for example (VERBOSE, BUFFERS OFF) would give you everything except buffer stats. We'd need a separate flag/flags to control what VERBOSE originally did, but that doesn't bother me --- it's an opportunity for more clarity of definition, anyway. I do feel that it's a good idea to keep ANALYZE separate. "Execute the query or not" is a mighty fundamental thing. I've never liked that name for the option though --- maybe we could deprecate it in favor of EXECUTE? regards, tom lane
Re: New EXPLAIN option: ALL
Hi, On 2019-05-07 23:23:55 +0200, David Fetter wrote: > Would you agree that there's a problem here as I described as > motivation for people who operate databases? Yea, but I don't think the solution is where you seek it. I think the problem is that our defaults for EXPLAIN, in particular EXPLAIN ANALYZE, are dumb. And that your desire for ALL stems from that, rather than it being desirable on its own. We really e.g. should just enable BUFFERS by default. The reason we can't is that right now we have checks like: EXPLAIN (BUFFERS) SELECT 1; ERROR: 22023: EXPLAIN option BUFFERS requires ANALYZE LOCATION: ExplainQuery, explain.c:206 but we ought to simply remove them. There's no benefit, and besides preventing from enabling BUFFERS by default it means that enabling/disabling ANALYZE is more work than necessary. > If so, do you have one or more abbreviations in mind that aren't > called ALL? I realize that Naming Things™ is one of two hard problems > in computer science, but it's still one we have to tackle. As I said, I don't think ALL is a good idea under any name. Like it just makes no sense to have ANALYZE, SUMMARY, VERBOSE, BUFFERS, SETTINGS, FORMAT controlled by one option, unless you call it DWIM. It's several separate axis (query is executed or not (ANALYZE), verbosity (SUMMARY, VERBOSE), collecting additional information (BUFFERS, TIMING), output format). Greetings, Andres Freund
Re: New EXPLAIN option: ALL
On Tue, May 7, 2019 at 9:31 AM David Fetter wrote: > If you're tuning a query interactively, it's a lot simpler to prepend, > for example, > > EXPLAIN (ALL, FORMAT JSON) > > to it than to prepend something along the lines of > > EXPLAIN(ANALYZE, VERBOSE, COSTS, BUFFERS, SETTINGS, TIMING, SUMMARY, > PARTRIDGE_IN_A_PEAR_TREE, FORMAT JSON) > > to it. FWIW, I have the following in my psqlrc: \set ea 'EXPLAIN (ANALYZE, SETTINGS, VERBOSE, BUFFERS) ' The idea behind that is that I can prepend ":ea" as needed, rather than doing a lot of typing each time, as in: :ea SELECT ... -- Peter Geoghegan
Re: New EXPLAIN option: ALL
On Tue, May 07, 2019 at 09:44:30AM -0700, Andres Freund wrote: > Hi, > > On 2019-05-07 18:34:11 +0200, David Fetter wrote: > > On Tue, May 07, 2019 at 08:41:47AM -0700, Andres Freund wrote: > > > On 2019-05-07 09:30:47 +0200, David Fetter wrote: > > > > It can get a little tedious turning on (or off) all the boolean > > > > options to EXPLAIN, so please find attached a shortcut. > > > > > > I'm not convinced this is a good idea - it seems likely that we'll > > > add conflicting options at some point, and then this will just be a > > > pain in the neck. > > > > I already left out FORMAT for a similar reason, namely that it's not a > > boolean, so it's not part of flipping on (or off) all the switches. > > Which is already somewhat hard to explain. > > Imagine if we had CPU_PROFILE = on (which'd be *extremely* > useful). Would you want that to be switched on automatically? How about > RECORD_IO_TRACE? How about DISTINCT_BUFFERS which'd be like BUFFERS > except that we'd track how many different buffers are accessed using HLL > or such? Would also be extremely useful. > > > > Are you seeing a point in the future where there'd be both mutually > > exclusive boolean options and no principled reason to choose among > > them? If so, what might it look like? > > Yes. CPU_PROFILE_PERF, CPU_PROFILE_VTUNE. And lots more. Thanks for clarifying. Would you agree that there's a problem here as I described as motivation for people who operate databases? If so, do you have one or more abbreviations in mind that aren't called ALL? I realize that Naming Things™ is one of two hard problems in computer science, but it's still one we have to tackle. Best, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Re: New EXPLAIN option: ALL
Hi, On 2019-05-07 18:34:11 +0200, David Fetter wrote: > On Tue, May 07, 2019 at 08:41:47AM -0700, Andres Freund wrote: > > On 2019-05-07 09:30:47 +0200, David Fetter wrote: > > > It can get a little tedious turning on (or off) all the boolean > > > options to EXPLAIN, so please find attached a shortcut. > > > > I'm not convinced this is a good idea - it seems likely that we'll > > add conflicting options at some point, and then this will just be a > > pain in the neck. > > I already left out FORMAT for a similar reason, namely that it's not a > boolean, so it's not part of flipping on (or off) all the switches. Which is already somewhat hard to explain. Imagine if we had CPU_PROFILE = on (which'd be *extremely* useful). Would you want that to be switched on automatically? How about RECORD_IO_TRACE? How about DISTINCT_BUFFERS which'd be like BUFFERS except that we'd track how many different buffers are accessed using HLL or such? Would also be extremely useful. > Are you seeing a point in the future where there'd be both mutually > exclusive boolean options and no principled reason to choose among > them? If so, what might it look like? Yes. CPU_PROFILE_PERF, CPU_PROFILE_VTUNE. And lots more. Greetings, Andres Freund
Re: New EXPLAIN option: ALL
On Tue, May 07, 2019 at 08:41:47AM -0700, Andres Freund wrote: > Hi, > > On 2019-05-07 09:30:47 +0200, David Fetter wrote: > > It can get a little tedious turning on (or off) all the boolean > > options to EXPLAIN, so please find attached a shortcut. > > I'm not convinced this is a good idea - it seems likely that we'll > add conflicting options at some point, and then this will just be a > pain in the neck. I already left out FORMAT for a similar reason, namely that it's not a boolean, so it's not part of flipping on (or off) all the switches. Are you seeing a point in the future where there'd be both mutually exclusive boolean options and no principled reason to choose among them? If so, what might it look like? Best, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Re: New EXPLAIN option: ALL
On Tue, May 07, 2019 at 11:03:23AM +0200, Rafia Sabih wrote: > On Tue, 7 May 2019 at 09:30, David Fetter wrote: > > > > Folks, > > > > It can get a little tedious turning on (or off) all the boolean > > options to EXPLAIN, so please find attached a shortcut. > > I don't understand this, do you mind explaining a bit may be with an > example on how you want it to work. If you're tuning a query interactively, it's a lot simpler to prepend, for example, EXPLAIN (ALL, FORMAT JSON) to it than to prepend something along the lines of EXPLAIN(ANALYZE, VERBOSE, COSTS, BUFFERS, SETTINGS, TIMING, SUMMARY, PARTRIDGE_IN_A_PEAR_TREE, FORMAT JSON) to it. Best, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Re: New EXPLAIN option: ALL
On Tue, May 07, 2019 at 11:13:15AM +0300, Sergei Kornilov wrote:
> Hi
>
> I liked this idea.
>
> + timing_set = true;
> + es->timing = defGetBoolean(opt);
> + summary_set = true;
> + es->timing = defGetBoolean(opt);
>
> second es->timing should be es->summary, right?
You are correct! Sorry about the copy-paste-o.
Best,
David.
--
David Fetter http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
>From 424d82c9ff519c02ae266700ed1f4b40ac9c495f Mon Sep 17 00:00:00 2001
From: David Fetter
Date: Tue, 7 May 2019 00:26:29 -0700
Subject: [PATCH v2] Add an ALL option to EXPLAIN
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="2.21.0"
This is a multi-part message in MIME format.
--2.21.0
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
which starts all boolean options at once.
diff --git a/doc/src/sgml/ref/explain.sgml b/doc/src/sgml/ref/explain.sgml
index 385d10411f..ade3793c5f 100644
--- a/doc/src/sgml/ref/explain.sgml
+++ b/doc/src/sgml/ref/explain.sgml
@@ -43,6 +43,7 @@ EXPLAIN [ ANALYZE ] [ VERBOSE ] statementboolean ]
TIMING [ boolean ]
SUMMARY [ boolean ]
+ALL [ boolean ]
FORMAT { TEXT | XML | JSON | YAML }
@@ -224,6 +225,16 @@ ROLLBACK;
+
+ALL
+
+
+ Include (or exclude) all of the above settings before examining the rest
+ of the options. It defaults to FALSE.
+
+
+
+
FORMAT
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index a6c6de78f1..f58b9d7290 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -151,6 +151,26 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt, const char *queryString,
bool summary_set = false;
/* Parse options list. */
+ /* First, see whether ALL is set */
+ foreach(lc, stmt->options)
+ {
+ DefElem *opt = (DefElem *) lfirst(lc);
+
+ if (strcmp(opt->defname, "all") == 0)
+ {
+ es->analyze = defGetBoolean(opt);
+ es->verbose = defGetBoolean(opt);
+ es->costs = defGetBoolean(opt);
+ es->buffers = defGetBoolean(opt);
+ es->settings = defGetBoolean(opt);
+ timing_set = true;
+ es->timing = defGetBoolean(opt);
+ summary_set = true;
+ es->summary_set = defGetBoolean(opt);
+ }
+ }
+
+ /* If you add another boolean option here, remember to add it above, too */
foreach(lc, stmt->options)
{
DefElem*opt = (DefElem *) lfirst(lc);
@@ -194,6 +214,8 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt, const char *queryString,
opt->defname, p),
parser_errposition(pstate, opt->location)));
}
+ else if (strcmp(opt->defname, "all") == 0)
+ ; /* Do nothing */
else
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 3dc0e8a4fb..4a69f9711c 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -10704,6 +10704,7 @@ explain_option_elem:
explain_option_name:
NonReservedWord { $$ = $1; }
| analyze_keyword { $$ = "analyze"; }
+ | ALL { $$ = "all"; }
;
explain_option_arg:
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index bcddc7601e..8177f9c6da 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2862,9 +2862,9 @@ psql_completion(const char *text, int start, int end)
* one word, so the above test is correct.
*/
if (ends_with(prev_wd, '(') || ends_with(prev_wd, ','))
- COMPLETE_WITH("ANALYZE", "VERBOSE", "COSTS", "BUFFERS",
+ COMPLETE_WITH("ALL", "ANALYZE", "VERBOSE", "COSTS", "BUFFERS",
"TIMING", "SUMMARY", "FORMAT");
- else if (TailMatches("ANALYZE|VERBOSE|COSTS|BUFFERS|TIMING|SUMMARY"))
+ else if (TailMatches("ALL|ANALYZE|VERBOSE|COSTS|BUFFERS|TIMING|SUMMARY"))
COMPLETE_WITH("ON", "OFF");
else if (TailMatches("FORMAT"))
COMPLETE_WITH("TEXT", "XML", "JSON", "YAML");
@@ -2874,7 +2874,8 @@ psql_completion(const char *text, int start, int end)
"VERBOSE");
else if (Matches("EXPLAIN", "(*)") ||
Matches("EXPLAIN", "VERBOSE") ||
- Matches("EXPLAIN", "ANALYZE", "VERBOSE"))
+ Matches("EXPLAIN", "ANALYZE", "VERBOSE") ||
+ Matches("EXPLAIN", "ALL"))
COMPLETE_WITH("SELECT", "INSERT", "DELETE", "UPDATE", "DECLARE");
/* FETCH && MOVE */
--2.21.0--
Re: New EXPLAIN option: ALL
Hi, On 2019-05-07 09:30:47 +0200, David Fetter wrote: > It can get a little tedious turning on (or off) all the boolean > options to EXPLAIN, so please find attached a shortcut. I'm not convinced this is a good idea - it seems likely that we'll add conflicting options at some point, and then this will just be a pain in the neck. Greetings, Andres Freund
Re: New EXPLAIN option: ALL
On Tue, 7 May 2019 at 09:30, David Fetter wrote: > > Folks, > > It can get a little tedious turning on (or off) all the boolean > options to EXPLAIN, so please find attached a shortcut. > I don't understand this, do you mind explaining a bit may be with an example on how you want it to work. -- Regards, Rafia Sabih
Re: New EXPLAIN option: ALL
Hi I liked this idea. + timing_set = true; + es->timing = defGetBoolean(opt); + summary_set = true; + es->timing = defGetBoolean(opt); second es->timing should be es->summary, right? regards, Sergei
Re: New EXPLAIN option: ALL
On Tue, May 07, 2019 at 09:30:47AM +0200, David Fetter wrote:
> Folks,
>
> It can get a little tedious turning on (or off) all the boolean
> options to EXPLAIN, so please find attached a shortcut.
>
> Best,
> David.
It helps to have a working patch for this.
Best,
David.
--
David Fetter http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
>From d62669c5b1d22c18949ad5d6ae3f6b23b49819e3 Mon Sep 17 00:00:00 2001
From: David Fetter
Date: Tue, 7 May 2019 00:26:29 -0700
Subject: [PATCH v2] Add an ALL option to EXPLAIN
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="2.21.0"
This is a multi-part message in MIME format.
--2.21.0
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
which starts all boolean options at once.
diff --git a/doc/src/sgml/ref/explain.sgml b/doc/src/sgml/ref/explain.sgml
index 385d10411f..ade3793c5f 100644
--- a/doc/src/sgml/ref/explain.sgml
+++ b/doc/src/sgml/ref/explain.sgml
@@ -43,6 +43,7 @@ EXPLAIN [ ANALYZE ] [ VERBOSE ] statementboolean ]
TIMING [ boolean ]
SUMMARY [ boolean ]
+ALL [ boolean ]
FORMAT { TEXT | XML | JSON | YAML }
@@ -224,6 +225,16 @@ ROLLBACK;
+
+ALL
+
+
+ Include (or exclude) all of the above settings before examining the rest
+ of the options. It defaults to FALSE.
+
+
+
+
FORMAT
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index a6c6de78f1..7ad73e4482 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -151,6 +151,26 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt, const char *queryString,
bool summary_set = false;
/* Parse options list. */
+ /* First, see whether ALL is set */
+ foreach(lc, stmt->options)
+ {
+ DefElem *opt = (DefElem *) lfirst(lc);
+
+ if (strcmp(opt->defname, "all") == 0)
+ {
+ es->analyze = defGetBoolean(opt);
+ es->verbose = defGetBoolean(opt);
+ es->costs = defGetBoolean(opt);
+ es->buffers = defGetBoolean(opt);
+ es->settings = defGetBoolean(opt);
+ timing_set = true;
+ es->timing = defGetBoolean(opt);
+ summary_set = true;
+ es->timing = defGetBoolean(opt);
+ }
+ }
+
+ /* If you add another boolean option here, remember to add it above, too */
foreach(lc, stmt->options)
{
DefElem*opt = (DefElem *) lfirst(lc);
@@ -194,6 +214,8 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt, const char *queryString,
opt->defname, p),
parser_errposition(pstate, opt->location)));
}
+ else if (strcmp(opt->defname, "all") == 0)
+ ; /* Do nothing */
else
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 3dc0e8a4fb..4a69f9711c 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -10704,6 +10704,7 @@ explain_option_elem:
explain_option_name:
NonReservedWord { $$ = $1; }
| analyze_keyword { $$ = "analyze"; }
+ | ALL { $$ = "all"; }
;
explain_option_arg:
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index bcddc7601e..8177f9c6da 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2862,9 +2862,9 @@ psql_completion(const char *text, int start, int end)
* one word, so the above test is correct.
*/
if (ends_with(prev_wd, '(') || ends_with(prev_wd, ','))
- COMPLETE_WITH("ANALYZE", "VERBOSE", "COSTS", "BUFFERS",
+ COMPLETE_WITH("ALL", "ANALYZE", "VERBOSE", "COSTS", "BUFFERS",
"TIMING", "SUMMARY", "FORMAT");
- else if (TailMatches("ANALYZE|VERBOSE|COSTS|BUFFERS|TIMING|SUMMARY"))
+ else if (TailMatches("ALL|ANALYZE|VERBOSE|COSTS|BUFFERS|TIMING|SUMMARY"))
COMPLETE_WITH("ON", "OFF");
else if (TailMatches("FORMAT"))
COMPLETE_WITH("TEXT", "XML", "JSON", "YAML");
@@ -2874,7 +2874,8 @@ psql_completion(const char *text, int start, int end)
"VERBOSE");
else if (Matches("EXPLAIN", "(*)") ||
Matches("EXPLAIN", "VERBOSE") ||
- Matches("EXPLAIN", "ANALYZE", "VERBOSE"))
+ Matches("EXPLAIN", "ANALYZE", "VERBOSE") ||
+ Matches("EXPLAIN", "ALL"))
COMPLETE_WITH("SELECT", "INSERT", "DELETE", "UPDATE", "DECLARE");
/* FETCH && MOVE */
--2.21.0--
New EXPLAIN option: ALL
Folks,
It can get a little tedious turning on (or off) all the boolean
options to EXPLAIN, so please find attached a shortcut.
Best,
David.
--
David Fetter http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
>From 8de75e81eb7161c712e2af97b0619a69f33c6b91 Mon Sep 17 00:00:00 2001
From: David Fetter
Date: Tue, 7 May 2019 00:26:29 -0700
Subject: [PATCH v1] Add an ALL option to EXPLAIN
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="2.21.0"
This is a multi-part message in MIME format.
--2.21.0
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
which starts all boolean options at once.
diff --git a/doc/src/sgml/ref/explain.sgml b/doc/src/sgml/ref/explain.sgml
index 385d10411f..ade3793c5f 100644
--- a/doc/src/sgml/ref/explain.sgml
+++ b/doc/src/sgml/ref/explain.sgml
@@ -43,6 +43,7 @@ EXPLAIN [ ANALYZE ] [ VERBOSE ] statementboolean ]
TIMING [ boolean ]
SUMMARY [ boolean ]
+ALL [ boolean ]
FORMAT { TEXT | XML | JSON | YAML }
@@ -224,6 +225,16 @@ ROLLBACK;
+
+ALL
+
+
+ Include (or exclude) all of the above settings before examining the rest
+ of the options. It defaults to FALSE.
+
+
+
+
FORMAT
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index a6c6de78f1..966f4566c7 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -151,6 +151,26 @@ ExplainQuery(ParseState *pstate, ExplainStmt *stmt, const char *queryString,
bool summary_set = false;
/* Parse options list. */
+ /* First, see whether ALL is set */
+ foreach(lc, stmt->options)
+ {
+ DefElem *opt = (DefElem *) lfirst(lc);
+
+ if (strcmp(opt->defname, "all") == 0)
+ {
+ es->analyze = defGetBoolean(opt);
+ es->verbose = defGetBoolean(opt);
+ es->costs = defGetBoolean(opt);
+ es->buffers = defGetBoolean(opt);
+ es->settings = defGetBoolean(opt);
+ timing_set = true;
+ es->timing = defGetBoolean(opt);
+ summary_set = true;
+ es->timing = defGetBoolean(opt);
+ }
+ }
+
+ /* If you add another boolean option here, remember to add it above, too */
foreach(lc, stmt->options)
{
DefElem*opt = (DefElem *) lfirst(lc);
--2.21.0--
