Thread: 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 <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
Attachment
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 <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
Attachment
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
On Tue, 7 May 2019 at 09:30, David Fetter <david@fetter.org> 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
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
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 <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
Attachment
On Tue, May 07, 2019 at 11:03:23AM +0200, Rafia Sabih wrote:
> On Tue, 7 May 2019 at 09:30, David Fetter <david@fetter.org> 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 <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 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 <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
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
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 <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 Tue, May 7, 2019 at 9:31 AM David Fetter <david@fetter.org> 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
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
Andres Freund <andres@anarazel.de> 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
Greetings, * Tom Lane (tgl@sss.pgh.pa.us) wrote: > Andres Freund <andres@anarazel.de> 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
Attachment
Stephen Frost <sfrost@snowman.net> 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
On Tue, May 07, 2019 at 06:12:56PM -0400, Stephen Frost wrote: > Greetings, > > * Tom Lane (tgl@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 <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
Greetings, * Tom Lane (tgl@sss.pgh.pa.us) wrote: > Stephen Frost <sfrost@snowman.net> 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
Attachment
Greetings, * David Fetter (david@fetter.org) wrote: > On Tue, May 07, 2019 at 06:12:56PM -0400, Stephen Frost wrote: > > * Tom Lane (tgl@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
Attachment
On Tue, May 7, 2019 at 6:25 PM Tom Lane <tgl@sss.pgh.pa.us> 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
On Tue, May 7, 2019 at 12:31 PM David Fetter <david@fetter.org> 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
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
> On May 8, 2019, at 4:22 PM, Vik Fearing <vik.fearing@2ndquadrant.com> 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 VERBOSEis IMHO pretty useless... I’m +1 on trying to move away from ANALYZE as well, though I think it’s mostly orthogonal...
On Tue, May 07, 2019 at 06:25:12PM -0400, Tom Lane wrote: > Stephen Frost <sfrost@snowman.net> 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 <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
Attachment
On Mon, May 13, 2019 at 07:51:12AM +0200, David Fetter wrote: > On Tue, May 07, 2019 at 06:25:12PM -0400, Tom Lane wrote: > > Stephen Frost <sfrost@snowman.net> 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? Fixed it. I hope the patch is a little easier to digest as now attached. 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
Attachment
- v4-0001-Changed-EXPLAIN-ANALYZE-to-EXPLAIN-EXEC.patch
- v4-0002-Documentation.patch
- v4-0003-psql-support.patch
- v4-0004-Changed-examples-in-the-documentation-to-use-EXEC.patch
- v4-0005-Changed-code-comments-to-reflect-the-new-default-.patch
- v4-0006-Mechanical-changes-to-regression-tests-and-their-.patch
- v4-0007-Propagate-changes-to-auto_explain.patch
- v4-0008-Mechanical-changes-in-file_fdw-and-postgres_fdw.patch
David Fetter <david@fetter.org> 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
On Wed, May 15, 2019 at 09:32:31AM -0400, Tom Lane wrote: > David Fetter <david@fetter.org> 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 <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
Hi, On May 15, 2019 7:20:34 AM PDT, David Fetter <david@fetter.org> wrote: >On Wed, May 15, 2019 at 09:32:31AM -0400, Tom Lane wrote: >> David Fetter <david@fetter.org> 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 ismore 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.
Andres Freund <andres@anarazel.de> writes:
> On May 15, 2019 7:20:34 AM PDT, David Fetter <david@fetter.org> 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
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 Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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
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 Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> 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
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
On Wed, May 15, 2019 at 10:46:39AM -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: >> On May 15, 2019 7:20:34 AM PDT, David Fetter <david@fetter.org> 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
Attachment
On Wed, May 15, 2019 at 09:32:31AM -0400, Tom Lane wrote: > David Fetter <david@fetter.org> 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 <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
Attachment
- v5-0001-Changed-EXPLAIN-ANALYZE-to-EXPLAIN-EXEC.patch
- v5-0002-Documentation.patch
- v5-0003-psql-support.patch
- v5-0004-Examples-in-the-documentation-now-use-EXEC.patch
- v5-0005-Code-comments-now-use-EXPLAIN-EXEC.patch
- v5-0006-Mechanical-changes-in-file_fdw-and-postgres_fdw.patch
- v5-0007-Propagate-changes-to-auto_explain.patch
- v5-0008-Mechanical-changes-to-regression-tests-and-output.patch
On Wed, May 15, 2019 at 1:53 PM Tom Lane <tgl@sss.pgh.pa.us> 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
On Tue, May 21, 2019 at 12:32:21PM -0400, Robert Haas wrote: > On Wed, May 15, 2019 at 1:53 PM Tom Lane <tgl@sss.pgh.pa.us> 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 <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
David Fetter <david@fetter.org> 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
On Tue, May 21, 2019 at 1:38 PM David Fetter <david@fetter.org> 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
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
Andres Freund <andres@anarazel.de> 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
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
On Tue, Jun 18, 2019 at 11:08:31PM +0200, Peter Eisentraut wrote: > 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. Are you proposing something along the lines of this? PROFILE [statement]; /* Shows the plan */ PROFILE RUN [statement]; /* Actually executes the query */ 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 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
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
> On 19 Jun 2019, at 08:15, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
On Wed, Jun 19, 2019 at 02:08:21PM +0200, Daniel Gustafsson wrote: > > On 19 Jun 2019, at 08:15, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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 <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 2019-05-18 19:39, David Fetter wrote: > On Wed, May 15, 2019 at 09:32:31AM -0400, Tom Lane wrote: >> David Fetter <david@fetter.org> 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
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 <david@fetter.org> 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 <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