Re: generic explain options v3

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

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

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(at)svana(dot)org> http://svana.org/kleptog/
> Please line up in a tree and maintain the heap invariant while
> boarding. Thank you for flying nlogn airlines.

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

On Sun, Jul 19, 2009 at 9:47 AM, Andres Freund<andres(at)anarazel(dot)de> wrote:
> 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

Robert Haas <robertmhaas(at)gmail(dot)com> 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

On Tue, Jul 21, 2009 at 7:47 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Tue, Jul 21, 2009 at 7:47 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> 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

On Tue, Jul 21, 2009 at 10:05 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Tue, Jul 21, 2009 at 7:47 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> 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

On Tue, Jul 21, 2009 at 10:29 PM, Robert Haas<robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Jul 21, 2009 at 10:05 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>>> On Tue, Jul 21, 2009 at 7:47 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>>> 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

Robert Haas <robertmhaas(at)gmail(dot)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.

regards, tom lane

On Thu, Jul 23, 2009 at 12:08 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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

On Thu, Jul 23, 2009 at 2:23 PM, Robert Haas<robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Jul 23, 2009 at 12:08 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)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

Robert Haas <robertmhaas(at)gmail(dot)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.

> - 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

On Sun, Jul 26, 2009 at 7:40 PM, Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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