Default gucs for EXPLAIN

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
34 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Default gucs for EXPLAIN

Vik Fearing-6
Here is a patch to provide default gucs for EXPLAIN options.

I have two goals with this patch.  The first is that I personally
*always* want BUFFERS turned on, so this would allow me to do it without
typing it every time.

The second is it would make it easier to change the actual default for
settings if we choose to do so because users would be able to switch it
back if they prefer.

The patch is based off of a995b371ae.
--
Vik Fearing

0001-Add-default-settings-for-EXPLAIN-options.patch (18K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Default gucs for EXPLAIN

Justin Pryzby
On Sat, May 23, 2020 at 11:14:05AM +0200, Vik Fearing wrote:

> Here is a patch to provide default gucs for EXPLAIN options.
>
> I have two goals with this patch.  The first is that I personally
> *always* want BUFFERS turned on, so this would allow me to do it without
> typing it every time.
>
> The second is it would make it easier to change the actual default for
> settings if we choose to do so because users would be able to switch it
> back if they prefer.
>
> The patch is based off of a995b371ae.

The patch adds new GUCs for each explain() option.

Would it be better to make a GUC called default_explain_options which might say
"COSTS ON, ANALYZE ON, VERBOSE OFF, BUFFERS TBD, FORMAT TEXT, ..."
..and parsed using the same thing that parses the existing options (which would
need to be factored out of ExplainQuery()).

Do we really want default_explain_analyze ?
It sounds like bad news that EXPLAIN DELETE might or might not remove rows
depending on the state of a variable.

I think this should be split into two patches:
One to make the default explain options configurable, and a separate patch to
change the defaults.

+ /* Set defaults. */
+ es->analyze = default_explain_analyze;
+ es->buffers = default_explain_buffers;
+ es->costs = default_explain_costs;
...

I think you could avoid eight booleans and nine DefElems by making
default_explain_* a struct, maybe ExplainState.  Maybe all the defaults should
just be handled in NewExplainState() ?

--
Justin


Reply | Threaded
Open this post in threaded view
|

Re: Default gucs for EXPLAIN

Pavel Stehule
In reply to this post by Vik Fearing-6


so 23. 5. 2020 v 11:14 odesílatel Vik Fearing <[hidden email]> napsal:
Here is a patch to provide default gucs for EXPLAIN options.

I have two goals with this patch.  The first is that I personally
*always* want BUFFERS turned on, so this would allow me to do it without
typing it every time.

The second is it would make it easier to change the actual default for
settings if we choose to do so because users would be able to switch it
back if they prefer.

The patch is based off of a995b371ae.

It's lot of new GUC variables. Isn't better only one that allows list of values?

Regards

Pavel
 
--
Vik Fearing
Reply | Threaded
Open this post in threaded view
|

Re: Default gucs for EXPLAIN

Vik Fearing-6
In reply to this post by Justin Pryzby
On 5/23/20 6:12 PM, Justin Pryzby wrote:

> The patch adds new GUCs for each explain() option.

Thank you for looking at it!

> Would it be better to make a GUC called default_explain_options which might say
> "COSTS ON, ANALYZE ON, VERBOSE OFF, BUFFERS TBD, FORMAT TEXT, ..."
> ..and parsed using the same thing that parses the existing options (which would
> need to be factored out of ExplainQuery()).
I do not think that would be better, no.

> Do we really want default_explain_analyze ?
> It sounds like bad news that EXPLAIN DELETE might or might not remove rows
> depending on the state of a variable.

I have had sessions where not using ANALYZE was the exception, not the
rule.  I would much prefer to type  EXPLAIN (ANALYZE OFF)  in those cases.

> I think this should be split into two patches:
> One to make the default explain options configurable, and a separate patch to
> change the defaults.

This patch does not change the defaults, so I'm not sure what you mean here?
--
Vik Fearing


Reply | Threaded
Open this post in threaded view
|

Re: Default gucs for EXPLAIN

Vik Fearing-6
In reply to this post by Pavel Stehule
On 5/23/20 6:23 PM, Pavel Stehule wrote:

> It's lot of new GUC variables. Isn't better only one that allows list of
> values?
I like this way better.
--
Vik Fearing


Reply | Threaded
Open this post in threaded view
|

Re: Default gucs for EXPLAIN

Nikolay Samokhvalov
In reply to this post by Vik Fearing-6
This is a very good improvement! Using information about buffers is my favorite way to optimize queries.

Not having BUFFERS enabled by default means that in most cases, when asking for help, people send execution plans without buffers info.

And it's simply in on event to type "(ANALYZE, BUFFERS)" all the time.

So I strongly support this change, thank you, Vik.

On Sat, May 23 2020 at 02:14, Vik Fearing <[hidden email]> wrote:

Here is a patch to provide default gucs for EXPLAIN options.

I have two goals with this patch. The first is that I personally
*always* want BUFFERS turned on, so this would allow me to do it without typing it every time.

The second is it would make it easier to change the actual default for settings if we choose to do so because users would be able to switch it back if they prefer.

The patch is based off of a995b371ae.
--
Vik Fearing

Reply | Threaded
Open this post in threaded view
|

Re: Default gucs for EXPLAIN

Fabien COELHO-3
In reply to this post by Vik Fearing-6

Bonjour Vik,

>> Do we really want default_explain_analyze ?
>> It sounds like bad news that EXPLAIN DELETE might or might not remove rows
>> depending on the state of a variable.
>
> I have had sessions where not using ANALYZE was the exception, not the
> rule.  I would much prefer to type  EXPLAIN (ANALYZE OFF)  in those cases.

I concur with Justin that having EXPLAIN DELETE/UPDATE actually executing
the query can be too much a bit of a surprise for a user attempting it.

A typical scenario would be "this DELETE/UPDATE query is too slow", admin
connects to production and try safe EXPLAIN on some random sample, and get
bitten because the default was changed.

A way out could be having 3 states for analyse (off, read-only, on) which
would block updates/deletes by making the transaction/operation read-only
to prevent side effects, unless explicitely asked for? I'm not sure if
this can be easily implemented, though. Or maybe run the query in a
separate transaction which is then coldly rollbacked? Hmmm, I'm not really
convincing myself on this one… The safe option seems not allowing to
change ANALYZE option default.

While testing the issue, I'm surprised at the syntax:

  EXPLAIN [ ( option [, ...] ) ] statement
  EXPLAIN [ ANALYZE ] [ VERBOSE ] statement

Why not allowing the following:

  EXPLAIN [ ANALYZE ] [ VERBOSE ] [ ( option [, ...] ) ] statement

--
Fabien.
Reply | Threaded
Open this post in threaded view
|

Re: Default gucs for EXPLAIN

Fabien COELHO-3

> The safe option seems not allowing to change ANALYZE option default.

> EXPLAIN [ ANALYZE ] [ VERBOSE ] statement

Some more thoughts:

An argument for keeping it that way is that there is already a special
syntax to enable ANALYSE explicitely, which as far as I am concerned I
only ever attempt after having tried a "EXPLAIN query" first.

Moreover, having to just add the ANALYSE keyword is kind of cheap, while
having to type "(some list of options)" is pretty cumbersome.

--
Fabien.


Reply | Threaded
Open this post in threaded view
|

Re: Default gucs for EXPLAIN

Vik Fearing-6
In reply to this post by Fabien COELHO-3
On 5/24/20 9:31 AM, Fabien COELHO wrote:
> While testing the issue, I'm surprised at the syntax:
>
>  EXPLAIN [ ( option [, ...] ) ] statement
>  EXPLAIN [ ANALYZE ] [ VERBOSE ] statement
>
> Why not allowing the following:
>
>  EXPLAIN [ ANALYZE ] [ VERBOSE ] [ ( option [, ...] ) ] statement

That has nothing to do with this patch.
--
Vik Fearing


Reply | Threaded
Open this post in threaded view
|

Re: Default gucs for EXPLAIN

Fabien COELHO-4

>> Why not allowing the following:
>>
>>  EXPLAIN [ ANALYZE ] [ VERBOSE ] [ ( option [, ...] ) ] statement
>
> That has nothing to do with this patch.

Sure, it was just in passing, I was surprised by this restriction.

--
Fabien.
Reply | Threaded
Open this post in threaded view
|

Re: Default gucs for EXPLAIN

Bruce Momjian
In reply to this post by Nikolay Samokhvalov
On Sat, May 23, 2020 at 06:16:25PM +0000, Nikolay Samokhvalov wrote:
> This is a very good improvement! Using information about buffers is my favorite
> way to optimize queries.
>
> Not having BUFFERS enabled by default means that in most cases, when asking for
> help, people send execution plans without buffers info.
>
> And it's simply in on event to type "(ANALYZE, BUFFERS)" all the time.
>
> So I strongly support this change, thank you, Vik.

I am not excited about this new feature.  Why do it only for EXPLAIN?
That is a log of GUCs.  I can see this becoming a feature creep
disaster.

--
  Bruce Momjian  <[hidden email]>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +


Reply | Threaded
Open this post in threaded view
|

Re: Default gucs for EXPLAIN

Michael Paquier-2
On Mon, May 25, 2020 at 09:36:50PM -0400, Bruce Momjian wrote:
> I am not excited about this new feature.  Why do it only for EXPLAIN?
> That is a log of GUCs.  I can see this becoming a feature creep
> disaster.

FWIW, Neither am I.  This would create an extra maintenance cost, and
I would not want such stuff to spread to other commands either, say
CLUSTER, VACUUM, REINDEX, etc.  And note that it is always possible to
do that with a small extension using the utility hook and some
pre-loaded user-settable GUCs.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Default gucs for EXPLAIN

Stephen Frost
Greetings,

* Michael Paquier ([hidden email]) wrote:
> On Mon, May 25, 2020 at 09:36:50PM -0400, Bruce Momjian wrote:
> > I am not excited about this new feature.  Why do it only for EXPLAIN?

Would probably help to understand what your thinking is here regarding
how it could be done for everything...?  In particular, what else are
you thinking it'd be sensible for?

> > That is a log of GUCs.  I can see this becoming a feature creep
> > disaster.

I'd only view it as a feature creep disaster if we end up extending it
to things that don't make any sense..  I don't see any particular reason
why we'd have to do that though.  On the other hand, if there's a clean
way to do it for everything, that'd be pretty neat.

> FWIW, Neither am I.  This would create an extra maintenance cost, and
> I would not want such stuff to spread to other commands either, say
> CLUSTER, VACUUM, REINDEX, etc.  And note that it is always possible to
> do that with a small extension using the utility hook and some
> pre-loaded user-settable GUCs.

The suggestion to "go write C code that will be loaded via a utility
hook" is really entirely inappropriate here.

This strikes me as a pretty reasonable 'creature comfort' kind of idea.
Inventing GUCs to handle it is maybe not the best approach, but we
haven't really got anything better right at hand- psql can't parse
general SQL, today, and hasn't got it's own idea of "how to run
explain".  On the other hand, I could easily see a similar feature
being included in pgAdmin4 where running explain is clicking on a button
instead of typing 'explain'.

To that end- what if this was done client-side with '\explain' or
similar?  Basically, it'd work like \watch or \g but we'd have options
under pset like "explain_analyze t/f" and such.  I feel like that'd also
largely address the concerns about how this might 'feature creep' to
other commands- because those other commands don't work with a query
buffer, so it wouldn't really make sense for them.

As for the concerns wrt explain UPDATE or explain DETELE actually
running the query, that's what transactions are for, and if you don't
feel comfortable using transactions or using these options- then don't.

Thanks,

Stephen

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Default gucs for EXPLAIN

Nikolay Samokhvalov
In reply to this post by Bruce Momjian
On Mon, May 25, 2020 at 6:36 PM, Bruce Momjian <[hidden email]> wrote:

I am not excited about this new feature. Why do it only for EXPLAIN? That is a log of GUCs. I can see this becoming a feature creep disaster.


How about changing the default behavior, making BUFFERS enabled by default? Those who don't need it, always can say BUFFERS OFF — the say as for TIMING.


Reply | Threaded
Open this post in threaded view
|

Re: Default gucs for EXPLAIN

Laurenz Albe
On Tue, 2020-05-26 at 02:49 +0000, Nikolay Samokhvalov wrote:
> > I am not excited about this new feature. Why do it only for EXPLAIN? That is a log of GUCs. I can see this becoming a feature creep disaster.
> >
>
> How about changing the default behavior, making BUFFERS enabled by default? Those who don't need it, always can say BUFFERS OFF — the say as for TIMING.

+1

Yours,
Laurenz Albe



Reply | Threaded
Open this post in threaded view
|

Re: Default gucs for EXPLAIN

Guillaume Lelarge-3
In reply to this post by Stephen Frost
Le mar. 26 mai 2020 à 04:27, Stephen Frost <[hidden email]> a écrit :
Greetings,

* Michael Paquier ([hidden email]) wrote:
> On Mon, May 25, 2020 at 09:36:50PM -0400, Bruce Momjian wrote:
> > I am not excited about this new feature.  Why do it only for EXPLAIN?

Would probably help to understand what your thinking is here regarding
how it could be done for everything...?  In particular, what else are
you thinking it'd be sensible for?

> > That is a log of GUCs.  I can see this becoming a feature creep
> > disaster.

I'd only view it as a feature creep disaster if we end up extending it
to things that don't make any sense..  I don't see any particular reason
why we'd have to do that though.  On the other hand, if there's a clean
way to do it for everything, that'd be pretty neat.

> FWIW, Neither am I.  This would create an extra maintenance cost, and
> I would not want such stuff to spread to other commands either, say
> CLUSTER, VACUUM, REINDEX, etc.  And note that it is always possible to
> do that with a small extension using the utility hook and some
> pre-loaded user-settable GUCs.

The suggestion to "go write C code that will be loaded via a utility
hook" is really entirely inappropriate here.

This strikes me as a pretty reasonable 'creature comfort' kind of idea.
Inventing GUCs to handle it is maybe not the best approach, but we
haven't really got anything better right at hand- psql can't parse
general SQL, today, and hasn't got it's own idea of "how to run
explain".  On the other hand, I could easily see a similar feature
being included in pgAdmin4 where running explain is clicking on a button
instead of typing 'explain'.

To that end- what if this was done client-side with '\explain' or
similar?  Basically, it'd work like \watch or \g but we'd have options
under pset like "explain_analyze t/f" and such.  I feel like that'd also
largely address the concerns about how this might 'feature creep' to
other commands- because those other commands don't work with a query
buffer, so it wouldn't really make sense for them.

As for the concerns wrt explain UPDATE or explain DETELE actually
running the query, that's what transactions are for, and if you don't
feel comfortable using transactions or using these options- then don't.


This means you'll always have to check if the new GUCs are set up in a way it will actually execute the query or to open a transaction for the same reason. This is a huge behaviour change where people might lose data.

I really don't like this proposal (the new GUCs).


--
Guillaume.
Reply | Threaded
Open this post in threaded view
|

Re: Default gucs for EXPLAIN

David G Johnston
In reply to this post by Vik Fearing-6
On Saturday, May 23, 2020, Vik Fearing <[hidden email]> wrote:

> Do we really want default_explain_analyze ?
> It sounds like bad news that EXPLAIN DELETE might or might not remove rows
> depending on the state of a variable.

I have had sessions where not using ANALYZE was the exception, not the
rule.  I would much prefer to type  EXPLAIN (ANALYZE OFF)  in those cases.

Not sure about the feature as a whole but i’m strongly against having a GUC exist that conditions whether a query is actually executed.

David J.
Reply | Threaded
Open this post in threaded view
|

Re: Default gucs for EXPLAIN

David G Johnston
In reply to this post by Stephen Frost
On Monday, May 25, 2020, Stephen Frost <[hidden email]> wrote:
Greetings,

* Michael Paquier ([hidden email]) wrote:
> On Mon, May 25, 2020 at 09:36:50PM -0400, Bruce Momjian wrote:
> > I am not excited about this new feature.  Why do it only for EXPLAIN?

Would probably help to understand what your thinking is here regarding
how it could be done for everything...?  In particular, what else are
you thinking it'd be sensible for?

COPY comes to mind immediately.

David J. 
Reply | Threaded
Open this post in threaded view
|

Re: Default gucs for EXPLAIN

David Rowley
In reply to this post by Bruce Momjian
On Tue, 26 May 2020 at 13:36, Bruce Momjian <[hidden email]> wrote:

>
> On Sat, May 23, 2020 at 06:16:25PM +0000, Nikolay Samokhvalov wrote:
> > This is a very good improvement! Using information about buffers is my favorite
> > way to optimize queries.
> >
> > Not having BUFFERS enabled by default means that in most cases, when asking for
> > help, people send execution plans without buffers info.
> >
> > And it's simply in on event to type "(ANALYZE, BUFFERS)" all the time.
> >
> > So I strongly support this change, thank you, Vik.
>
> I am not excited about this new feature.

I'm against adding GUCs to control what EXPLAIN does by default.

A few current GUCs come to mind which gives external control to a
command's behaviour are:

standard_conforming_strings
backslash_quote
bytea_output

It's pretty difficult for application authors to write code that will
just work due to these GUCs. We end up with GUCs like
escape_string_warning to try and help application authors find areas
which may be problematic.

It's not an easy thing to search for in the archives, but we've
rejected GUCs that have proposed new ways which can break applications
in this way. For example [1]. You can see some arguments against that
in [2].

Now, there are certainly far fewer applications out there that will
execute an EXPLAIN, but the number is still above zero. I imagine the
authors of those applications might get upset if we create something
outside of the command that controls what the command does. Perhaps
the idea here is not quite as bad as that as applications could still
override the options by mentioning each EXPLAIN option in the command
they send to the server. However, we're not done adding new options
yet, so by doing this we'd be pretty much insisting that applications
that use EXPLAIN know about all EXPLAIN options for the server version
they're connected to. That seems like a big demand given that we've
been careful to still support the old
EXPLAIN syntax after we added the new way to specify the options in parenthesis.

[1] https://www.postgresql.org/message-id/flat/ACF85C502E55A143AB9F4ECFE960660A17282D@...
[2] https://www.postgresql.org/message-id/17880.1482648516%40sss.pgh.pa.us

David


Reply | Threaded
Open this post in threaded view
|

Re: Default gucs for EXPLAIN

Vik Fearing-6
On 5/26/20 1:30 PM, David Rowley wrote:

> On Tue, 26 May 2020 at 13:36, Bruce Momjian <[hidden email]> wrote:
>>
>> On Sat, May 23, 2020 at 06:16:25PM +0000, Nikolay Samokhvalov wrote:
>>> This is a very good improvement! Using information about buffers is my favorite
>>> way to optimize queries.
>>>
>>> Not having BUFFERS enabled by default means that in most cases, when asking for
>>> help, people send execution plans without buffers info.
>>>
>>> And it's simply in on event to type "(ANALYZE, BUFFERS)" all the time.
>>>
>>> So I strongly support this change, thank you, Vik.
>>
>> I am not excited about this new feature.
>
> I'm against adding GUCs to control what EXPLAIN does by default.
>
> A few current GUCs come to mind which gives external control to a
> command's behaviour are:
>
> standard_conforming_strings
> backslash_quote
> bytea_output
>
> It's pretty difficult for application authors to write code that will
> just work due to these GUCs. We end up with GUCs like
> escape_string_warning to try and help application authors find areas
> which may be problematic.
>
> It's not an easy thing to search for in the archives, but we've
> rejected GUCs that have proposed new ways which can break applications
> in this way. For example [1]. You can see some arguments against that
> in [2].
>
> Now, there are certainly far fewer applications out there that will
> execute an EXPLAIN, but the number is still above zero. I imagine the
> authors of those applications might get upset if we create something
> outside of the command that controls what the command does. Perhaps
> the idea here is not quite as bad as that as applications could still
> override the options by mentioning each EXPLAIN option in the command
> they send to the server. However, we're not done adding new options
> yet, so by doing this we'd be pretty much insisting that applications
> that use EXPLAIN know about all EXPLAIN options for the server version
> they're connected to. That seems like a big demand given that we've
> been careful to still support the old
> EXPLAIN syntax after we added the new way to specify the options in parenthesis.


Nah, this argument doesn't hold.  If an app wants something on or off,
it should say so.  If it doesn't care, then it doesn't care.

Are you saying we should have all new EXPLAIN options off forever into
the future because apps won't know about the new data?  I guess we
should also not ever introduce new plan nodes because those won't be
known either.

I'm not buying that.
--
Vik Fearing


12