FAQ
Here is an updated version of my "generic options for explain" patch.
Previous version here:

http://archives.postgresql.org/pgsql-hackers/2009-06/msg00866.php

This patch requires the "explain refactoring v4" patch, which you can
find here, to be applied first:

http://archives.postgresql.org/pgsql-hackers/2009-06/msg00865.php

In this version, I've taken the liberty of adding a "COSTS" option
which defaults to "ON", so that you can say: EXPLAIN (COSTS OFF) ...
to abolish display of the costs information, per my previous
suggestion. I was initially thinking of waiting to submit this as a
follow-on patch, but nobody seemed to object to the idea much, so I've
gone ahead and added it here. It remains to be seen whether someone
can develop a workable set of regression tests based on this
functionality, but it's pretty clear that it CAN'T be done without
this functionality, so this seems like a step in the right direction
at any rate.

The other major update in this patch is that it adds documentation. I
was not completely sure what the best way to document this was, so
it's very possible that what I've done here can be improved upon.

I will send updated versions of the "machine-readable explain output"
patches soon.

...Robert

Search Discussions

  • Andres Freund at Jul 19, 2009 at 1:15 am
    Hi Robert, Hi All,

    Patch applies with some offset changes, code changes look sensible, I
    personally like the new syntax and the features it may allow in future. One,
    possibly big, gripe remains though:
    The formerly valid statement which cannot be written without the parentheses
    and stay semantically equivalent:
    EXPLAIN (SELECT 1 ORDER BY 1) UNION ALL (SELECT 2 ORDER BY 1);
    is now not valid anymore (The added %prec UMINUS causes the first '(' to be
    recognize as start of the option list as intended).
    This currently can only be resolved by using an option list like:
    EXPLAIN (VERBOSE OFF) ...
    Its also currently impossible to use an empty set of parentheses to resolve
    this - this could easily be changed though.

    I have to admit I don't see a nice solution here except living with the
    incompatibility... Perhaps somebody has a better idea?

    Andres

    PS: The 'offset corrected' version I tested with is attached
  • Martijn van Oosterhout at Jul 19, 2009 at 12:39 pm

    On Sun, Jul 19, 2009 at 03:15:38AM +0200, Andres Freund wrote:
    Hi Robert, Hi All,

    Patch applies with some offset changes, code changes look sensible, I
    personally like the new syntax and the features it may allow in future. One,
    possibly big, gripe remains though:
    The formerly valid statement which cannot be written without the parentheses
    and stay semantically equivalent:
    EXPLAIN (SELECT 1 ORDER BY 1) UNION ALL (SELECT 2 ORDER BY 1);
    is now not valid anymore (The added %prec UMINUS causes the first '(' to be
    recognize as start of the option list as intended).
    This currently can only be resolved by using an option list like:
    EXPLAIN (VERBOSE OFF) ...
    Its also currently impossible to use an empty set of parentheses to resolve
    this - this could easily be changed though.

    I have to admit I don't see a nice solution here except living with the
    incompatibility... Perhaps somebody has a better idea?
    I think another possibility might be to allow the syntax:

    EXPLAIN VERBOSE ANALYSE (options) SELECt ...;

    Sure, it's a bit ugly, but in the grammer you could then do:
    ExplainStmt: EXPLAIN opt_analyze opt_verbose ExplainableStmt
    EXPLAIN opt_analyze opt_verbose '(' explain_option_list ')' ExplainableStmt
    Which means that (I think) bison can use the token *after* the '(' to
    disambiguate, and since SELECT is a reserved word I think the problem
    may be solved.

    (The point being that then Bison can reduce the opt_analyze for both
    cases).

    Hope this helps,
    --
    Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/
    Please line up in a tree and maintain the heap invariant while
    boarding. Thank you for flying nlogn airlines.
  • Andres Freund at Jul 19, 2009 at 1:47 pm

    On Sunday 19 July 2009 14:39:33 Martijn van Oosterhout wrote:
    On Sun, Jul 19, 2009 at 03:15:38AM +0200, Andres Freund wrote:
    Hi Robert, Hi All,

    Patch applies with some offset changes, code changes look sensible, I
    personally like the new syntax and the features it may allow in future.
    One, possibly big, gripe remains though:
    The formerly valid statement which cannot be written without the
    parentheses and stay semantically equivalent:
    EXPLAIN (SELECT 1 ORDER BY 1) UNION ALL (SELECT 2 ORDER BY 1);
    is now not valid anymore (The added %prec UMINUS causes the first '(' to
    be recognize as start of the option list as intended).
    This currently can only be resolved by using an option list like:
    EXPLAIN (VERBOSE OFF) ...
    Its also currently impossible to use an empty set of parentheses to
    resolve this - this could easily be changed though.

    I have to admit I don't see a nice solution here except living with the
    incompatibility... Perhaps somebody has a better idea?
    I think another possibility might be to allow the syntax:

    EXPLAIN VERBOSE ANALYSE (options) SELECt ...;

    Sure, it's a bit ugly, but in the grammer you could then do:
    ExplainStmt: EXPLAIN opt_analyze opt_verbose ExplainableStmt
    EXPLAIN opt_analyze opt_verbose '(' explain_option_list ')'
    ExplainableStmt
    Which means that (I think) bison can use the token *after* the '(' to
    disambiguate, and since SELECT is a reserved word I think the problem
    may be solved.
    I think that does not work since explain_option_name has to include keywords
    to be able to use ANALYZE and VERBOSE.

    Its solvable by not allowing all keywords there but only ANALYZE and VERBOSE.
    Involves some duplication though...

    Patch attached.

    Andres
  • Robert Haas at Jul 19, 2009 at 9:55 pm

    On Sun, Jul 19, 2009 at 9:47 AM, Andres Freundwrote:
    On Sunday 19 July 2009 14:39:33 Martijn van Oosterhout wrote:
    On Sun, Jul 19, 2009 at 03:15:38AM +0200, Andres Freund wrote:
    Hi Robert, Hi All,

    Patch applies with some offset changes, code changes look sensible, I
    personally like the new syntax and the features it may allow in future.
    One, possibly big, gripe remains though:
    The formerly valid statement which cannot be written without the
    parentheses and stay semantically equivalent:
    EXPLAIN (SELECT 1 ORDER BY 1) UNION ALL (SELECT 2 ORDER BY 1);
    is now not valid anymore (The added %prec UMINUS causes the first '(' to
    be recognize as start of the option list as intended).
    This currently can only be resolved by using an option list like:
    EXPLAIN (VERBOSE OFF) ...
    Its also currently impossible to use an empty set of parentheses to
    resolve this - this could easily be changed though.

    I have to admit I don't see a nice solution here except living with the
    incompatibility... Perhaps somebody has a better idea?
    I think another possibility might be to allow the syntax:

    EXPLAIN VERBOSE ANALYSE (options) SELECt ...;

    Sure, it's a bit ugly, but in the grammer you could then do:
    ExplainStmt: EXPLAIN opt_analyze opt_verbose ExplainableStmt
    EXPLAIN opt_analyze opt_verbose '(' explain_option_list ')'
    ExplainableStmt
    Which means that (I think) bison can use the token *after* the '(' to
    disambiguate, and since SELECT is a reserved word I think the problem
    may be solved.
    I think that does not work since explain_option_name has to include keywords
    to be able to use ANALYZE and VERBOSE.

    Its solvable by not allowing all keywords there but only ANALYZE and VERBOSE.
    Involves some duplication though...

    Patch attached.
    Hmm, good idea. I will update and resubmit.

    ...Robert
  • Tom Lane at Jul 21, 2009 at 11:48 pm

    Robert Haas writes:
    Here is an updated version of my "generic options for explain" patch.
    What is the rationale for essentially duplicating defGetBoolean()?

    Also, I'd suggest changing the ExplainStmt struct to have a list of
    DefElem options instead of hard-wiring the option set at that level.

    regards, tom lane
  • Robert Haas at Jul 22, 2009 at 1:22 am

    On Tue, Jul 21, 2009 at 7:47 PM, Tom Lanewrote:
    Robert Haas <robertmhaas@gmail.com> writes:
    Here is an updated version of my "generic options for explain" patch.
    What is the rationale for essentially duplicating defGetBoolean()?
    I just didn't realize we already had something along those lines.
    Updated patch attached, to which I've also applied Andres Freund's
    parser changes, suggested here:

    http://archives.postgresql.org/pgsql-hackers/2009-07/msg01213.php
    Also, I'd suggest changing the ExplainStmt struct to have a list of
    DefElem options instead of hard-wiring the option set at that level.
    What is the advantage of that?

    ...Robert
  • Tom Lane at Jul 22, 2009 at 2:06 am

    Robert Haas writes:
    On Tue, Jul 21, 2009 at 7:47 PM, Tom Lanewrote:
    Also, I'd suggest changing the ExplainStmt struct to have a list of
    DefElem options instead of hard-wiring the option set at that level.
    What is the advantage of that?
    Fewer places to change when you add a new option; in particular, not
    copyfuncs or equalfuncs. Also, the way you are doing it is gratuitously
    unlike every other command that has similar issues to deal with.
    Everybody else parses their DefElem list at execution time. I think
    you should have the legacy ANALYZE and VERBOSE syntax elements generate
    DefElem list members that get examined at execution.

    BTW, I see that your "explain refactoring" patch is marked ready
    for committer, but is it actually sane to apply it before the other
    two?

    regards, tom lane
  • Robert Haas at Jul 22, 2009 at 2:29 am

    On Tue, Jul 21, 2009 at 10:05 PM, Tom Lanewrote:
    Robert Haas <robertmhaas@gmail.com> writes:
    On Tue, Jul 21, 2009 at 7:47 PM, Tom Lanewrote:
    Also, I'd suggest changing the ExplainStmt struct to have a list of
    DefElem options instead of hard-wiring the option set at that level.
    What is the advantage of that?
    Fewer places to change when you add a new option; in particular, not
    copyfuncs or equalfuncs.  Also, the way you are doing it is gratuitously
    unlike every other command that has similar issues to deal with.
    Everybody else parses their DefElem list at execution time.  I think
    you should have the legacy ANALYZE and VERBOSE syntax elements generate
    DefElem list members that get examined at execution.
    Not having to touch copyfuncs or equalfuncs for future options is a
    definite plus, so I'll rework along these lines.
    BTW, I see that your "explain refactoring" patch is marked ready
    for committer, but is it actually sane to apply it before the other
    two?
    I think so. It's all code cleanup, with no behavioral changes, and is
    intended to contain only the stuff that seemed to me as thought it
    would still be worth doing even if the rest of the patch set were
    rejected.

    ...Robert
  • Robert Haas at Jul 23, 2009 at 3:16 am

    On Tue, Jul 21, 2009 at 10:29 PM, Robert Haaswrote:
    On Tue, Jul 21, 2009 at 10:05 PM, Tom Lanewrote:
    Robert Haas <robertmhaas@gmail.com> writes:
    On Tue, Jul 21, 2009 at 7:47 PM, Tom Lanewrote:
    Also, I'd suggest changing the ExplainStmt struct to have a list of
    DefElem options instead of hard-wiring the option set at that level.
    What is the advantage of that?
    Fewer places to change when you add a new option; in particular, not
    copyfuncs or equalfuncs.  Also, the way you are doing it is gratuitously
    unlike every other command that has similar issues to deal with.
    Everybody else parses their DefElem list at execution time.  I think
    you should have the legacy ANALYZE and VERBOSE syntax elements generate
    DefElem list members that get examined at execution.
    Not having to touch copyfuncs or equalfuncs for future options is a
    definite plus, so I'll rework along these lines.
    Ugh. I took a look at this and it turns out that there are some
    tentacles. It doesn't seem very sane to actually do anything with a
    list of DefElem nodes, so we really need to parse that list and
    convert it to a more sensible format right away (this also seems
    important for proper error checking).

    The natural place to do this would be in ExplainPrintPlan(), which is
    already copying the relevant fields from the ExplainStmt over into an
    ExplainState, but that's too far down the call tree, which (for a
    non-utility statement when ExplainOneQuery_hook is null) looks like
    this:

    ExplainQuery -> ExplainOneQuery -> ExplainOnePlan -> ExplainPrintPlan

    The obvious solution to that is to create the ExplainState sooner,
    back up at the ExplainQuery level. If we do that, though, then
    ExplainState will need to become a public API, because
    contrib/auto_explain calls ExplainPrintPlan(). And if we do that,
    then probably we should declare it in include/nodes/execnodes.h and
    make it a node type... and if we do that then we'll be back to a
    copyfuncs/equalfuncs change every time we add a flag.

    Now that's not to say there's no advantage in the proposed refactoring
    - it's still more consistent with the way things are done elsewhere.
    But since it's going to be a fair amount of work and fail to achieve
    one of the two goals you set forth for it, I'd like to get
    confirmation before proceeding if possible, and any suggestions you
    may have for how to make it as clean as possible.

    Thanks,

    ...Robert
  • Tom Lane at Jul 23, 2009 at 4:08 pm

    Robert Haas writes:
    Ugh. I took a look at this and it turns out that there are some
    tentacles. It doesn't seem very sane to actually do anything with a
    list of DefElem nodes, so we really need to parse that list and
    convert it to a more sensible format right away (this also seems
    important for proper error checking).
    Yeah, the standard approach is to convert it into a group of values
    at the start of execution of the utility command.
    The obvious solution to that is to create the ExplainState sooner,
    back up at the ExplainQuery level. If we do that, though, then
    ExplainState will need to become a public API, because
    contrib/auto_explain calls ExplainPrintPlan().
    Well, if we add any more options to EXPLAIN then auto_explain may well
    be interested in them, so I'm not sure this is bad. The alternative
    is to keep adding retail parameters to the public functions.
    And if we do that,
    then probably we should declare it in include/nodes/execnodes.h and
    make it a node type...
    No, just a struct declared in commands/explain.h. There's no reason
    for it to be part of the Node system.

    regards, tom lane
  • Robert Haas at Jul 23, 2009 at 6:23 pm

    On Thu, Jul 23, 2009 at 12:08 PM, Tom Lanewrote:
    Robert Haas <robertmhaas@gmail.com> writes:
    Ugh.  I took a look at this and it turns out that there are some
    tentacles.  It doesn't seem very sane to actually do anything with a
    list of DefElem nodes, so we really need to parse that list and
    convert it to a more sensible format right away (this also seems
    important for proper error checking).
    Yeah, the standard approach is to convert it into a group of values
    at the start of execution of the utility command.
    The obvious solution to that is to create the ExplainState sooner,
    back up at the ExplainQuery level.  If we do that, though, then
    ExplainState will need to become a public API, because
    contrib/auto_explain calls ExplainPrintPlan().
    Well, if we add any more options to EXPLAIN then auto_explain may well
    be interested in them, so I'm not sure this is bad.  The alternative
    is to keep adding retail parameters to the public functions.
    And if we do that,
    then probably we should declare it in include/nodes/execnodes.h and
    make it a node type...
    No, just a struct declared in commands/explain.h.  There's no reason
    for it to be part of the Node system.
    Oh, OK. That will work. Thanks.

    ...Robert
  • Robert Haas at Jul 25, 2009 at 2:47 am

    On Thu, Jul 23, 2009 at 2:23 PM, Robert Haaswrote:
    On Thu, Jul 23, 2009 at 12:08 PM, Tom Lanewrote:
    Robert Haas <robertmhaas@gmail.com> writes:
    Ugh.  I took a look at this and it turns out that there are some
    tentacles.  It doesn't seem very sane to actually do anything with a
    list of DefElem nodes, so we really need to parse that list and
    convert it to a more sensible format right away (this also seems
    important for proper error checking).
    Yeah, the standard approach is to convert it into a group of values
    at the start of execution of the utility command.
    The obvious solution to that is to create the ExplainState sooner,
    back up at the ExplainQuery level.  If we do that, though, then
    ExplainState will need to become a public API, because
    contrib/auto_explain calls ExplainPrintPlan().
    Well, if we add any more options to EXPLAIN then auto_explain may well
    be interested in them, so I'm not sure this is bad.  The alternative
    is to keep adding retail parameters to the public functions.
    And if we do that,
    then probably we should declare it in include/nodes/execnodes.h and
    make it a node type...
    No, just a struct declared in commands/explain.h.  There's no reason
    for it to be part of the Node system.
    Oh, OK.  That will work.  Thanks.
    Here's the update. There are a few things that I'm not entirely happy
    with here, but not quite sure what to do about either.

    - ExplainPrintPlan() is now almost trivial. It seems like there
    should be some way to get rid of this altogether, but I'm not quite
    sure how. I thought about ripping pstmt and rtable out of
    ExplainState and just storying queryDesc there. But that involves
    changing a lot of code, and while it makes some things simpler, it
    makes other parts more complex. I'm not sure whether it's a win or
    not; I'm also not sure how much brainpower it's worth spending on
    this.

    - It's becoming increasingly evident to me that the explain stuff in
    prepare.c has no business being there and should be moved to
    explain.c. I haven't done that here, but it's worth thinking about.
    We could turn several functions that are currently public into statics
    if we did that.

    - The hack needed in ExplainLogLevel is just that.

    Help!

    ...Robert
  • Tom Lane at Jul 26, 2009 at 11:40 pm

    Robert Haas writes:
    Here's the update. There are a few things that I'm not entirely happy
    with here, but not quite sure what to do about either.
    Committed with a few editorializations.
    - ExplainPrintPlan() is now almost trivial. It seems like there
    should be some way to get rid of this altogether, but I'm not quite
    sure how. I thought about ripping pstmt and rtable out of
    ExplainState and just storying queryDesc there. But that involves
    changing a lot of code, and while it makes some things simpler, it
    makes other parts more complex. I'm not sure whether it's a win or
    not; I'm also not sure how much brainpower it's worth spending on
    this.
    I think the problem here is that you chose to treat ExplainState.pstmt
    as a parameter, when it's better considered as an internal field.
    I changed it to the latter approach.
    - It's becoming increasingly evident to me that the explain stuff in
    prepare.c has no business being there and should be moved to
    explain.c. I haven't done that here, but it's worth thinking about.
    I'm unconvinced. The reason that code is that way is that the
    alternative would require explain.c to know quite a lot about prepared
    plans, which does not seem like an improvement.
    - The hack needed in ExplainLogLevel is just that.
    Yeah, I thought that was okay. We could alternatively refactor the
    code so that the parameter analysis code is a separate function that
    utility.c could call, but it's unclear that it's worth the trouble.

    regards, tom lane
  • Robert Haas at Jul 27, 2009 at 2:52 am

    On Sun, Jul 26, 2009 at 7:40 PM, Tom Lanewrote:
    Robert Haas <robertmhaas@gmail.com> writes:
    Here's the update.  There are a few things that I'm not entirely happy
    with here, but not quite sure what to do about either.
    Committed with a few editorializations.
    Thanks.
    - ExplainPrintPlan() is now almost trivial.  It seems like there
    should be some way to get rid of this altogether, but I'm not quite
    sure how.  I thought about ripping pstmt and rtable out of
    ExplainState and just storying queryDesc there.  But that involves
    changing a lot of code, and while it makes some things simpler, it
    makes other parts more complex.  I'm not sure whether it's a win or
    not; I'm also not sure how much brainpower it's worth spending on
    this.
    I think the problem here is that you chose to treat ExplainState.pstmt
    as a parameter, when it's better considered as an internal field.
    I changed it to the latter approach.
    Sounds fine.
    - It's becoming increasingly evident to me that the explain stuff in
    prepare.c has no business being there and should be moved to
    explain.c.  I haven't done that here, but it's worth thinking about.
    I'm unconvinced.  The reason that code is that way is that the
    alternative would require explain.c to know quite a lot about prepared
    plans, which does not seem like an improvement.
    I didn't consider that. As it is, prepare.c has to know quite a lot
    about explaining, so it may be six of one, half a dozen of the other.
    - The hack needed in ExplainLogLevel is just that.
    Yeah, I thought that was okay.  We could alternatively refactor the
    code so that the parameter analysis code is a separate function that
    utility.c could call, but it's unclear that it's worth the trouble.
    OK.

    It seems I have quite a bit of work in front of me unbreaking the
    machine-readable explain patch. I started grinding through it, but
    it's not pretty. I'll post an updated version when I have it.

    ...Robert

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedJun 18, '09 at 1:18a
activeJul 27, '09 at 2:52a
posts15
users4
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2021 Grokbase