Обсуждение: Default gucs for EXPLAIN
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
Вложения
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
so 23. 5. 2020 v 11:14 odesílatel Vik Fearing <vik@postgresfriends.org> 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
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
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
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 <vik@postgresfriends.org> 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
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.
> 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.
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
>> 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.
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 <bruce@momjian.us> 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 +
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
Вложения
Greetings, * Michael Paquier (michael@paquier.xyz) 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
Вложения
On Mon, May 25, 2020 at 6:36 PM, Bruce Momjian <bruce@momjian.us> 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.
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 afeature creep disaster. > > > > How about changing the default behavior, making BUFFERS enabled by default? Those who don't need it, always can say BUFFERSOFF — the say as for TIMING. +1 Yours, Laurenz Albe
Le mar. 26 mai 2020 à 04:27, Stephen Frost <sfrost@snowman.net> a écrit :
Greetings,
* Michael Paquier (michael@paquier.xyz) 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.
On Saturday, May 23, 2020, Vik Fearing <vik@postgresfriends.org> 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.
On Monday, May 25, 2020, Stephen Frost <sfrost@snowman.net> wrote:
Greetings,
* Michael Paquier (michael@paquier.xyz) 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.
On Tue, 26 May 2020 at 13:36, Bruce Momjian <bruce@momjian.us> 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@mailserver2.local.mstarlabs.com [2] https://www.postgresql.org/message-id/17880.1482648516%40sss.pgh.pa.us David
On 5/26/20 1:30 PM, David Rowley wrote: > On Tue, 26 May 2020 at 13:36, Bruce Momjian <bruce@momjian.us> 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
út 26. 5. 2020 v 4:27 odesílatel Stephen Frost <sfrost@snowman.net> napsal:
Greetings,
* Michael Paquier (michael@paquier.xyz) 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.
the partial solution can be custom psql statements. Now, it can be just workaround
\set explain 'explain (analyze, buffers)'
:explain select * from pg_class ;
and anybody can prepare customized statements how he likes
Regards
Pavel
Thanks,
Stephen
Greetings, * Guillaume Lelarge (guillaume@lelarge.info) wrote: > Le mar. 26 mai 2020 à 04:27, Stephen Frost <sfrost@snowman.net> a écrit : > > 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. It's only a behaviour change if you enable it.. and the suggestion I made specifically wouldn't even be a regular 'explain', you'd be using '\explain' in psql, a new command. > I really don't like this proposal (the new GUCs). The proposal you're commenting on (seemingly mine, anyway) didn't include adding any new GUCs. Thanks, Stephen
Вложения
Greetings, * David G. Johnston (david.g.johnston@gmail.com) wrote: > On Monday, May 25, 2020, Stephen Frost <sfrost@snowman.net> wrote: > > * Michael Paquier (michael@paquier.xyz) 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. Indeed... and we have a \copy already, so following my proposal, at least, it seems like we could naturally add in options to have defaults to be used with \copy is used in psql. That might end up being a bit more interesting since we didn't contempalte that idea when \copy was first written and therefore we might need to change the syntax that the backend COPY commands to make this work (maybe adopting a similar syntax to explain, in addition to the existing WITH options after the COPY command, and then deciding which to prefer when both exist, or thorw an error in such a case). Thanks, Stephen
Вложения
Greetings, * Pavel Stehule (pavel.stehule@gmail.com) wrote: > the partial solution can be custom psql statements. Now, it can be just > workaround > > \set explain 'explain (analyze, buffers)' > :explain select * from pg_class ; > > and anybody can prepare customized statements how he likes Yeah, it's really very rudimentary though, unfortunately. A proper language in psql would be *really* nice with good ways to reference variables and such.. I don't view this as really being a good justification to not have a \explain type of command. Thanks, Stephen
Вложения
Le mar. 26 mai 2020 à 16:25, Stephen Frost <sfrost@snowman.net> a écrit :
Greetings,
* Guillaume Lelarge (guillaume@lelarge.info) wrote:
> Le mar. 26 mai 2020 à 04:27, Stephen Frost <sfrost@snowman.net> a écrit :
> > 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.
It's only a behaviour change if you enable it.. and the suggestion I
made specifically wouldn't even be a regular 'explain', you'd be using
'\explain' in psql, a new command.
> I really don't like this proposal (the new GUCs).
The proposal you're commenting on (seemingly mine, anyway) didn't
include adding any new GUCs.
My bad. I didn't read your email properly, sorry.
I wouldn't complain about a \explain metacommand. The proposal I (still) dislike is Vik's.
--
Guillaume.
On Sat, May 23, 2020 at 06:33:48PM +0200, Vik Fearing 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. I suggest that such sessions are themselves exceptional. > > 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? Sorry, ignore that; I wrote it before digesting the patch. On Sat, May 23, 2020 at 06:16:25PM +0000, Nikolay Samokhvalov wrote: > Not having BUFFERS enabled by default means that in most cases, when asking > for help, people send execution plans without buffers info. I also presumed that's where this patch was going to lead, but it doesn't actually change the default. So doesn't address that, except that if someone reports a performance problem, we can tell them to run: |alter system set explain_buffers=on; SELECT pg_reload_conf() ..which is no better, except that it would also affect any *additional* problem reports which might be made from that cluster. If you want to change the default, I think that should be a separate patch/thread. -- Justin
On 5/26/20 10:08 PM, Justin Pryzby wrote: > If you want to change the default, I think that should be a separate patch/thread. Yes, it will be. -- Vik Fearing
On Tue, 26 May 2020 at 23:59, Vik Fearing <vik@postgresfriends.org> wrote: > > On 5/26/20 1:30 PM, David Rowley wrote: > > On Tue, 26 May 2020 at 13:36, Bruce Momjian <bruce@momjian.us> 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 don't think this is a particularly good counter argument. If we add a new executor node then that's something that the server will send to the client. The client does not need knowledge about which version of PostreSQL it is connected to. If it receives details about some new node type in an EXPLAIN then it can be fairly certain that the server supports that node type. What we're talking about here is the opposite direction. The client is sending the command to the server, and the command it'll need to send is going to have to be specific to the server version. Now perhaps all such tools already have good infrastructure to change behaviour based on version, after all, these tools do also tend to query catalogue tables from time to time and those change between versions. Perhaps it would be good to hear from authors of such tools and get their input. If they all agree that it's not a problem then that certainly weakens my argument, but if they don't then perhaps you should reconsider. David
On Tue, May 26, 2020 at 4:30 AM David Rowley <dgrowleyml@gmail.com> wrote:
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.
I admittedly haven't tried to write an explain output parser but I'm doubting the conclusion that it is necessary to know the values of the various options in order to properly parse the output.
The output format type is knowable by observing the actual structure (first few characters probably) of the output and for everything else (all of the booleans) any parser worth its salt is going to be able to parse output where every possible setting is set to on.
I'm inclined to go with having everything except ANALYZE be something that has a GUC default override.
David J.
On Tue, May 26, 2020 at 4:53 PM David Rowley <dgrowleyml@gmail.com> wrote:
If we add
a new executor node then that's something that the server will send to
the client. The client does not need knowledge about which version of
PostreSQL it is connected to. If it receives details about some new
node type in an EXPLAIN then it can be fairly certain that the server
supports that node type.
The above is basically how I imagine explain handling software works today - if it sees a specific structure in the output it processes it. It has zero expectations about whether a feature with a option knob is set to true or false. And its deals with the one non-boolean option by examining the output text.
What we're talking about here is the opposite direction. The client is
sending the command to the server, and the command it'll need to send
is going to have to be specific to the server version. Now perhaps
all such tools already have good infrastructure to change behaviour
based on version, after all, these tools do also tend to query
catalogue tables from time to time and those change between versions.
I don't see how adding these optional GUCs impacts that materially. If the client provides a custom UI to the user and then writes an explain command itself it will need to possibly understand version differences whether these GUCs exist or not.
To hammer the point home if that client software is memorizing the choices made for the various options and then conditions its output based upon those choices then it should be specifying every one of them explicitly, in which case the GUCs wouldn't matter. If it is somehow depending upon the existing defaults and user choices to figure out the option values then, yes, the GUCs would be hidden information that may possibly confuse it if, say, a user has a GUC BUFFERS on but didn't make a choice in the client UI which defaulted to FALSE mimicking our default and because the default was chosen didn't output BUFFER off but left the option unspecified and now the buffers appear, which it for some reason isn't expecting and thus blows up. I could care less about that client and certainly wouldn't let its possible existence hold me back from adding a feature that bare-bones client users who send their own explain queries would find useful.
David J.
On Tue, 26 May 2020 at 23:59, Vik Fearing <vik@postgresfriends.org> wrote: > 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. Another argument against this is that it creates dependency among the new GUCs. Many of the options are not compatible with each other. e.g. postgres=# explain (timing on) select 1; ERROR: EXPLAIN option TIMING requires ANALYZE Would you propose we just error out in that case, or should we silently enable the required option, or disable the conflicting option? David
On Tuesday, May 26, 2020, David Rowley <dgrowleyml@gmail.com> wrote:
On Tue, 26 May 2020 at 23:59, Vik Fearing <vik@postgresfriends.org> wrote:
> 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.
Another argument against this is that it creates dependency among the
new GUCs. Many of the options are not compatible with each other. e.g.
postgres=# explain (timing on) select 1;
ERROR: EXPLAIN option TIMING requires ANALYZE
Would you propose we just error out in that case, or should we
silently enable the required option, or disable the conflicting
option?
The same thing we do today...ignore options that require analyze if analyze is not specified. There are no other options documented that are dependent with options besides than analyze. The docs say timing defaults to on, its only when explicitly specified instead of being treated as a default that the user message appears. All the GUCs are doing is changing the default.
David J.
On 5/27/20 7:27 AM, David G. Johnston wrote: > On Tuesday, May 26, 2020, David Rowley <dgrowleyml@gmail.com> wrote: > >> On Tue, 26 May 2020 at 23:59, Vik Fearing <vik@postgresfriends.org> wrote: >>> 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. >> >> Another argument against this is that it creates dependency among the >> new GUCs. Many of the options are not compatible with each other. e.g. >> >> postgres=# explain (timing on) select 1; >> ERROR: EXPLAIN option TIMING requires ANALYZE >> >> Would you propose we just error out in that case, or should we >> silently enable the required option, or disable the conflicting >> option? >> >> > The same thing we do today...ignore options that require analyze if analyze > is not specified. There are no other options documented that are dependent > with options besides than analyze. The docs say timing defaults to on, its > only when explicitly specified instead of being treated as a default that > the user message appears. All the GUCs are doing is changing the default. Yes, the patch handles this case the way you describe. In fact, the patch doesn't (or shouldn't) change any behavior at all. -- Vik Fearing
On Tue, May 26, 2020 at 7:30 AM David Rowley <dgrowleyml@gmail.com> wrote: > 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. I agree with this concern, as well as with what David says later, namely that the concern is less here than in some other cases but still not zero. I do think the idea of changing the default for BUFFERS from OFF to ON is a pretty good one, though. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, May 26, 2020 at 02:49:46AM +0000, Nikolay Samokhvalov wrote: > On Mon, May 25, 2020 at 6:36 PM, Bruce Momjian < bruce@momjian.us > 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 for changing the default of BUFFERS to ON. Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On Wed, May 27, 2020 at 11:10:35AM +0200, Vik Fearing wrote: > On 5/27/20 7:27 AM, David G. Johnston wrote: > >> Would you propose we just error out in that case, or should we > >> silently enable the required option, or disable the conflicting > >> option? > >> > > The same thing we do today...ignore options that require analyze if analyze > > is not specified. There are no other options documented that are dependent > > with options besides than analyze. The docs say timing defaults to on, its > > only when explicitly specified instead of being treated as a default that > > the user message appears. All the GUCs are doing is changing the default. > > > Yes, the patch handles this case the way you describe. In fact, the > patch doesn't (or shouldn't) change any behavior at all. I think it would have been helpful if an email explaining this idea for discussion would have been posted before a patch was generated and posted. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
On Tue, Jun 2, 2020 at 10:25 AM Bruce Momjian <bruce@momjian.us> wrote:
On Wed, May 27, 2020 at 11:10:35AM +0200, Vik Fearing wrote:
> On 5/27/20 7:27 AM, David G. Johnston wrote:
> >> Would you propose we just error out in that case, or should we
> >> silently enable the required option, or disable the conflicting
> >> option?
> >>
> > The same thing we do today...ignore options that require analyze if analyze
> > is not specified. There are no other options documented that are dependent
> > with options besides than analyze. The docs say timing defaults to on, its
> > only when explicitly specified instead of being treated as a default that
> > the user message appears. All the GUCs are doing is changing the default.
>
>
> Yes, the patch handles this case the way you describe. In fact, the
> patch doesn't (or shouldn't) change any behavior at all.
I think it would have been helpful if an email explaining this idea for
discussion would have been posted before a patch was generated and
posted.
I can see where it would have saved Vik some effort but I'm not seeing how an email without a patch is better for the rest of us than having a concrete change to discuss.
At this point, given the original goal of the patch was to try and grease a smoother path to changing the default for BUFFERS, and that people seem OK with doing just that without having this patch, I'd say we should just change the default and forget this patch. There hasn't been any other demand from our users for this capability and I also doubt that having BUFFERS on by default is going to bother people.
However, the one default on option, TIMING, also has a nice blurb about why having it enabled can be problematic and to consider turning it off. Is there a similar "oh by the way" with BUFFERS that I just haven't come across that would making having it on cause more problems than it solves?
David J.
On 6/2/20 7:54 PM, David G. Johnston wrote: > At this point, given the original goal of the patch was to try and grease a > smoother path to changing the default for BUFFERS, and that people seem OK > with doing just that without having this patch, I'd say we should just > change the default and forget this patch. There hasn't been any other > demand from our users for this capability and I also doubt that having > BUFFERS on by default is going to bother people. What about WAL? Can we turn that one one by default, too? I often find having VERBOSE on helps when people don't qualify their columns and I don't know the schema. We should turn that on by default, as well. -- Vik Fearing
On 6/2/20 7:25 PM, Bruce Momjian wrote: > On Wed, May 27, 2020 at 11:10:35AM +0200, Vik Fearing wrote: >> On 5/27/20 7:27 AM, David G. Johnston wrote: >>>> Would you propose we just error out in that case, or should we >>>> silently enable the required option, or disable the conflicting >>>> option? >>>> >>> The same thing we do today...ignore options that require analyze if analyze >>> is not specified. There are no other options documented that are dependent >>> with options besides than analyze. The docs say timing defaults to on, its >>> only when explicitly specified instead of being treated as a default that >>> the user message appears. All the GUCs are doing is changing the default. >> >> >> Yes, the patch handles this case the way you describe. In fact, the >> patch doesn't (or shouldn't) change any behavior at all. > > I think it would have been helpful if an email explaining this idea for > discussion would have been posted before a patch was generated and > posted. Why? -- Vik Fearing
On Tue, Jun 2, 2020 at 09:29:09PM +0200, Vik Fearing wrote:
> On 6/2/20 7:25 PM, Bruce Momjian wrote:
> > On Wed, May 27, 2020 at 11:10:35AM +0200, Vik Fearing wrote:
> >> On 5/27/20 7:27 AM, David G. Johnston wrote:
> >>>> Would you propose we just error out in that case, or should we
> >>>> silently enable the required option, or disable the conflicting
> >>>> option?
> >>>>
> >>> The same thing we do today...ignore options that require analyze if analyze
> >>> is not specified. There are no other options documented that are dependent
> >>> with options besides than analyze. The docs say timing defaults to on, its
> >>> only when explicitly specified instead of being treated as a default that
> >>> the user message appears. All the GUCs are doing is changing the default.
> >>
> >>
> >> Yes, the patch handles this case the way you describe. In fact, the
> >> patch doesn't (or shouldn't) change any behavior at all.
> >
> > I think it would have been helpful if an email explaining this idea for
> > discussion would have been posted before a patch was generated and
> > posted.
>
> Why?
Because you often have to go backwards to religitate things in the
patch, rather than opening with the design issues. Our TODO list is
very clear about this:
https://wiki.postgresql.org/wiki/Todo
Desirability -> Design -> Implement -> Test -> Review -> Commit
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EnterpriseDB https://enterprisedb.com
The usefulness of a cup is in its emptiness, Bruce Lee
On 6/2/20 10:51 PM, Bruce Momjian wrote: > On Tue, Jun 2, 2020 at 09:29:09PM +0200, Vik Fearing wrote: >> On 6/2/20 7:25 PM, Bruce Momjian wrote: >>> I think it would have been helpful if an email explaining this idea for >>> discussion would have been posted before a patch was generated and >>> posted. >> >> Why? > > Because you often have to go backwards to religitate things in the > patch, rather than opening with the design issues. Surely that's my problem; and it looks like the only thing I need to change in this patch is to remove the guc for ANALYZE. > Our TODO list is > very clear about this: > > https://wiki.postgresql.org/wiki/Todo > Desirability -> Design -> Implement -> Test -> Review -> Commit I can't read everything on this list (far from it), but I don't recall any other spontaneous patch being chastised for not having the bikeshedders-at-large do the first two steps before the implementer. -- Vik Fearing
On Tue, Jun 2, 2020 at 11:58:51PM +0200, Vik Fearing wrote: > On 6/2/20 10:51 PM, Bruce Momjian wrote: > > On Tue, Jun 2, 2020 at 09:29:09PM +0200, Vik Fearing wrote: > >> On 6/2/20 7:25 PM, Bruce Momjian wrote: > >>> I think it would have been helpful if an email explaining this idea for > >>> discussion would have been posted before a patch was generated and > >>> posted. > >> > >> Why? > > > > Because you often have to go backwards to religitate things in the > > patch, rather than opening with the design issues. > > > Surely that's my problem; and it looks like the only thing I need to > change in this patch is to remove the guc for ANALYZE. > > > > Our TODO list is > > very clear about this: > > > > https://wiki.postgresql.org/wiki/Todo > > Desirability -> Design -> Implement -> Test -> Review -> Commit > > > I can't read everything on this list (far from it), but I don't recall > any other spontaneous patch being chastised for not having the > bikeshedders-at-large do the first two steps before the implementer. Well, you have been around a long time, so I assumed you would know this, and have seen this in practice. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
On Tue, Jun 02, 2020 at 09:28:48PM +0200, Vik Fearing wrote: > On 6/2/20 7:54 PM, David G. Johnston wrote: > > At this point, given the original goal of the patch was to try and > > grease a smoother path to changing the default for BUFFERS, and > > that people seem OK with doing just that without having this > > patch, I'd say we should just change the default and forget this > > patch. There hasn't been any other demand from our users for this > > capability and I also doubt that having BUFFERS on by default is > > going to bother people. > > What about WAL? Can we turn that one one by default, too? > > I often find having VERBOSE on helps when people don't qualify their > columns and I don't know the schema. We should turn that on by > default, as well. +1 for all on (except ANALYZE because it would be a foot cannon) by default. For those few to whom it really matters, there'd be OFF switches. Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Le mer. 3 juin 2020 à 04:16, David Fetter <david@fetter.org> a écrit :
On Tue, Jun 02, 2020 at 09:28:48PM +0200, Vik Fearing wrote:
> On 6/2/20 7:54 PM, David G. Johnston wrote:
> > At this point, given the original goal of the patch was to try and
> > grease a smoother path to changing the default for BUFFERS, and
> > that people seem OK with doing just that without having this
> > patch, I'd say we should just change the default and forget this
> > patch. There hasn't been any other demand from our users for this
> > capability and I also doubt that having BUFFERS on by default is
> > going to bother people.
>
> What about WAL? Can we turn that one one by default, too?
>
> I often find having VERBOSE on helps when people don't qualify their
> columns and I don't know the schema. We should turn that on by
> default, as well.
+1 for all on (except ANALYZE because it would be a foot cannon) by
default. For those few to whom it really matters, there'd be OFF
switches.
+1 for all on, except ANALYZE (foot cannon as David says) and VERBOSE (verbose is something you ask for when the usual display isn't enough).
-1 for GUCs, we already have too many of them.
Guillaume.
I think this got ample review, so I've set it to "Waiting". -- Justin
On 2020-05-23 11:14, 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. There was a lot of opposition to the approach taken by this patch, but there was a lot of support turning BUFFERS on by default. Would you like to submit a patch for that? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> On 10 Jul 2020, at 15:30, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > > On 2020-05-23 11:14, 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. > > There was a lot of opposition to the approach taken by this patch, but there was a lot of support turning BUFFERS on bydefault. Would you like to submit a patch for that? My reading of this thread and the above that the patch, and CF entry, as it stands should be rejected - but that a separate patch for turning BUFFERS on by default would be highly appreciated. Unless objections I'll go do that in the CF app for 2020-07. cheers ./daniel
> On 2 Aug 2020, at 00:12, Justin Pryzby <pryzby@telsasoft.com> wrote: > On Sun, Aug 02, 2020 at 12:00:56AM +0200, Daniel Gustafsson wrote: >> My reading of this thread and the above that the patch, and CF entry, as it >> stands should be rejected - but that a separate patch for turning BUFFERS on by >> default would be highly appreciated. Unless objections I'll go do that in the >> CF app for 2020-07. > > Sounds right. Done that way. > I have a patch to enable buffers by default Please attach this thread as well to the CF entry for that patch once registered. cheers ./daniel