Thread: [PATCH] Improve ability to display optimizer analysis using OPTIMIZER_DEBUG

Hi all,

This is patch for TODO item: Improve ability to display optimizer analysis using OPTIMIZER_DEBUG

As per as suggestion in the mailing list which is to replace current mechanism of getting optimizer log via OPTIMIZER_DEBUG macro

to something more configurable (which doesn't require rebuilding postgres from source code). This patch replaces OPTIMIZER_DEBUG

by introducing a GUC show_optimizer_log which can be configured on and off to display (or hide) previously generated log from stdout to postmaster's log.

Please check attached patch, any feedback is appreciated.

-- 
Regards,
Ankit Kumar Pandey

Ankit Kumar Pandey <itsankitkp@gmail.com> writes:
> As per as suggestion in the mailing list which is to replace current 
> mechanism of getting optimizer log via OPTIMIZER_DEBUG macro
> to something more configurable (which doesn't require rebuilding 
> postgres from source code). This patch replaces /OPTIMIZER_DEBUG
> by introducing a//GUC /show_optimizer_log /which can be configured on 
> and off to//display (or hide)//previously generated log from stdout to 
> postmaster's log.

The problem with OPTIMIZER_DEBUG is that it's useless.  I've
been hacking on the PG planner for well more than twenty years,
and I do not think I've ever made any use of that code ---
certainly not since the turn of the century or so.
Making it GUC-accessible isn't going to make it more useful.

There certainly could be value in having some better trace
of what the planner is doing, but I honestly don't have much
of an idea of what that would look like.  debug_print_rel
isn't that, however, because it won't show you anything about
paths that were considered and immediately rejected by
add_path (or never got to add_path at all, because of the
approximate-initial-cost filters in joinpath.c).  We've
sometimes speculated about logging every path submitted to
add_path, but that would likely be too verbose to be very
helpful.

Also, because OPTIMIZER_DEBUG is such a backwater, it's
never gotten much polishing for usability.  There are large
gaps in what it can deal with (print_expr is pretty much
a joke for example), yet also lots of redundancy in the
output ... and dumping stuff to stdout isn't terribly helpful
to the average user in the first place.  These days people
would probably also wish that the output could be machine-
readable in some way (JSON-formatted, perhaps).

So I think this whole area would need some pretty serious
rethinking and attention to detail before we should consider
claiming that it's a useful feature.

            regards, tom lane

On 25/12/22 23:54, Tom Lane wrote:
>
> These days people
> would probably also wish that the output could be machine-
> readable in some way (JSON-formatted, perhaps).

Perhaps switch to enable logs could be on (standard logging style), 
json, xml etc and print the output

as required?


>   however, because it won't show you anything about
> paths that were considered and immediately rejected by
> add_path (or never got to add_path at all, because of the
> approximate-initial-cost filters in joinpath.c).  We've
> sometimes speculated about logging every path submitted to
> add_path, but that would likely be too verbose to be very
> helpful.
>
Maybe we could add verbose option too and this could be one of the 
output. Are there any more relevant information

that you could think of which can be included here?


Also, inputs from other hackers are welcomed here.


-- 
Regards,
Ankit Kumar Pandey

On Mon, 26 Dec 2022 at 08:05, Ankit Kumar Pandey <itsankitkp@gmail.com> wrote:
> Also, inputs from other hackers are welcomed here.

I'm with Tom on this.  I've never once used this feature to try to
figure out why a certain plan was chosen or not chosen.

Do you actually have a need for this or are you just trying to tick
off some TODO items?

I'd really rather not see us compiling all that debug code in by
default unless it's actually going to be useful to a meaningful number
of people.

David

On 03/01/23 08:38, David Rowley wrote:
> I'm with Tom on this.  I've never once used this feature to try to
> figure out why a certain plan was chosen or not chosen.
>
> I'd really rather not see us compiling all that debug code in by
> default unless it's actually going to be useful to a meaningful number
> of people.
>
Okay this makes sense.


>
> Do you actually have a need for this or are you just trying to tick
> off some TODO items?
>
I would say Iatter but reason I picked it up was more on side of 
learning optimizer better. Currently, I am left with

explain analyze which does its job but for understanding internal 
working of optimizer, there are not much alternatives.

Again, if I know where to put breakpoint, I could see required 
path/states but point of this todo item is

ability to do this without need of developer tools.

Also from the thread,

https://www.postgresql.org/message-id/20120821.121611.501104647612634419.t-ishii@sraoss.co.jp

> +1. It would also be popular with our academic users.
>
There could be potential for this as well.


-- 
Regards,
Ankit Kumar Pandey

On Tue, 3 Jan 2023 at 19:59, Ankit Kumar Pandey <itsankitkp@gmail.com> wrote:
>
>
> On 03/01/23 08:38, David Rowley wrote:
> > Do you actually have a need for this or are you just trying to tick
> > off some TODO items?
> >
> I would say Iatter but reason I picked it up was more on side of
> learning optimizer better.

I think it's better you leave this then. I think if someone comes
along and demonstrates the feature's usefulness and can sell us having
it so we can easily enable it by GUC then maybe that's the time to
consider it. I don't think ticking off a TODO item is reason enough.

> Also from the thread,
>
> https://www.postgresql.org/message-id/20120821.121611.501104647612634419.t-ishii@sraoss.co.jp
>
> > +1. It would also be popular with our academic users.
> >
> There could be potential for this as well.

I think the argument is best coming from someone who'll actually use it.

David

On Wed, 4 Jan 2023 at 16:15, Vladimir Churyukin <vladimir@churyukin.com> wrote:
> As an end user that spends a lot of time optimizing pretty complicated queries, I'd say that something like this
couldbe useful.
 

I think we really need to at least see that it *is* useful, not that
it *could be* useful.  For example, as an end user, you might not find
it great that the output is sent to stdout rather than to the window
that you execute the query in.

From what I can see here, the motivation to make this a useful feature
is backwards from what is normal. I think if you're keen to see a
feature that allows you better visibility into rejected paths then you
need to prove this is it rather than speculating that it might be
useful.

There was a bit of work being done in [1] with the end goal of having
the ability for add_path to call a hook function before it outright
rejects a path.  Maybe that would be a better place to put this and
then write some contrib module that provides some extended output in
EXPLAIN. That might require some additional fields so that we could
carry forward additional information that we'd like to show in
EXPLAIN. I imagine it's not ok just to start writing result lines in
the planner. The EXPLAIN format must be considered too and explain.c
seems like the place that should be done. add_path might need to
become a bit more verbose about the reason it rejected a certain path
for this to be useful.

David

[1] https://commitfest.postgresql.org/39/3599/

On Wed, 4 Jan 2023 at 17:39, Vladimir Churyukin <vladimir@churyukin.com> wrote:
>
> On Tue, Jan 3, 2023 at 7:41 PM David Rowley <dgrowleyml@gmail.com> wrote:
>> From what I can see here, the motivation to make this a useful feature
>> is backwards from what is normal. I think if you're keen to see a
>> feature that allows you better visibility into rejected paths then you
>> need to prove this is it rather than speculating that it might be
>> useful.
>>
>
> You can't see people using the feature unless you make it useful. If it's not useful right now (because it's
implementedas a compile-time flag with stdout prints for example),
 
> it doesn't mean it's not useful when it becomes more convenient. Probably the best way to find out is to create a
*convenient*extension and see if people start using it.
 

I don't think anyone is against making it useful. It's just not
seemingly useful enough for either Tom or I to make use of it. Nobody
else seems to have come along to tell us that it's useful to them.
Some people only speculated that it might be useful.

As I said before, if someone wants to make this work, then I think the
problem needs to be approached from the opposite direction.  i.e they
have a plan that they're not happy with and they need to come up with
something that usefully shows the reason why the plan that they expect
to be better is not chosen. That's not what's happened here.  add_path
does not just reject paths based on cost, so ISTM, for this to be
meaningful, add_path would need to do something to specify the reason
that the path was rejected, i.e a similarly costed path has pathkeys,
this one does not.

> I agree, extended EXPLAIN output would be a much better solution than writing into stdout. Can be implemented as an
extraEXPLAIN flag, something like EXPLAIN (TRACE).
 
> One of the issues here is the result will rather be pretty long (and may consist of multiple parts, so something like
returningmultiple refcursors might be necessary, so a client can fetch multiple result sets.
 
> Otherwise it won't be human-readable. Although it's not necessary the purpose, if the purpose is to make it
machine-readableand create tools to interpret the results, json format and a single resultset would be ok.
 
> The result can be represented as a list of trace events that shows profiler logic (the traces can be generated by the
hookyou mentioned and or by some other additional hooks).
 
> Is that what you were talking about?

The thing I had in mind was some mode that would record additional
details during planning that could be tagged onto the final plan in
createplan.c so that EXPLAIN could display them. I just think that
EXPLAIN is the place where people go to learn about _what_ the plan is
and it might also be the place where they might expect to go to find
out more details about _why_ that plan was chosen. I by no means have
a fully bakes idea on what that would look like, but I just think that
dumping a bunch of lines to stdout is not going to be useful to many
people and we need to think of something better in order to properly
make this useful.

> Another thing, since people react to this TODO item on https://wiki.postgresql.org/wiki/Todo, maybe it's better to
modifyor remove it, so they don't spend time working on something that is pretty much a dead end currently?
 

I've just adjusted it based on the discussion that's going on on this thread.

David

David Rowley <dgrowleyml@gmail.com> writes:
> The thing I had in mind was some mode that would record additional
> details during planning that could be tagged onto the final plan in
> createplan.c so that EXPLAIN could display them. I just think that
> EXPLAIN is the place where people go to learn about _what_ the plan is
> and it might also be the place where they might expect to go to find
> out more details about _why_ that plan was chosen. I by no means have
> a fully bakes idea on what that would look like, but I just think that
> dumping a bunch of lines to stdout is not going to be useful to many
> people and we need to think of something better in order to properly
> make this useful.

There's a number of problems in this area, but I think the really
fundamental issue is that for speed reasons the planner wants to
reject losing plan alternatives as quickly as possible.  So we simply
don't pursue those alternatives far enough to produce anything that
could serve as input for EXPLAIN (in its current form, anyway).
What that means is that a trace of add_path decisions just can't be
very useful to an end user: there isn't enough data to present the
decisions in a recognizable form, besides which there is too much
noise because most of the rejected options are in fact silly.
So indeed we find that even hard-core developers aren't interested
in consuming the data in that form.

Another issue is that frequently the problem is that we never
considered the desired plan at all, so that even if you had a
perfectly readable add_path trace it wouldn't show you what you want
to see.  This might happen because the planner is simply incapable of
producing that plan shape from the given query, but often it happens
for reasons like "function F() used in the query is marked volatile,
so we didn't flatten a subquery or consider an indexscan or whatever".
I'm not sure how we could produce output that would help people
discover that kind of problem ... but I am sure that an add_path
trace won't do it.

So, not only am I pretty down on exposing OPTIMIZER_DEBUG in
its current form, but I don't really believe that adding hooks
to add_path would allow an extension to produce anything of value.
I'd for sure want to see a convincing demonstration to the contrary
before we slow down that hot code path by adding hook calls.

            regards, tom lane

On 04/01/23 06:27, David Rowley wrote:
> I think it's better you leave this then. I think if someone comes
> along and demonstrates the feature's usefulness and can sell us having
> it so we can easily enable it by GUC then maybe that's the time to
> consider it. I don't think ticking off a TODO item is reason enough.
>
Make sense, not going further with this.

-- 
Regards,
Ankit Kumar Pandey

John Naylor <john.naylor@enterprisedb.com> writes:
> Note that the TODO list has accumulated some cruft over the years. Some
> time ago I started an effort to remove outdated/undesirable entries, and I
> should get back to that, but for the present, please take the warning at
> the top to heart:

> "WARNING for Developers: Unfortunately this list does not contain all the
> information necessary for someone to start coding a feature. Some of these
> items might have become unnecessary since they were added --- others might
> be desirable but the implementation might be unclear. When selecting items
> listed below, be prepared to first discuss the value of the feature. Do not
> assume that you can select one, code it and then expect it to be committed.
> "

I think we could make that even stronger: there's basically nothing on
the TODO list that isn't problematic in some way.  Otherwise it would
have been done already.  The entries involve large amounts of work,
or things that are subtler than they might appear, or cases where the
desirable semantics aren't clear, or tradeoffs that there's not
consensus about, or combinations of those.

IME it's typically a lot more productive to approach things via
"scratch your own itch".  If a problem is biting you directly, then
at least you have some clear idea of what it is that needs to be fixed.
You might have to work up to an understanding of how to fix it, but
you have a clear goal.

            regards, tom lane

> On 11/01/23 09:57, Tom Lane wrote:
> IME it's typically a lot more productive to approach things via
> "scratch your own itch".  If a problem is biting you directly, then
> at least you have some clear idea of what it is that needs to be fixed.
> You might have to work up to an understanding of how to fix it, but
> you have a clear goal.


Question is, how newcomers should start contribution if they are not 
coming with a problem in their hand?

Todo list is possibly first thing anyone, who is willing to contribute 
is going to read and for a new

contributor, it is not easy to judge situation (if todo item is easy for 
newcomers or bit involved).

One way to exacerbate this issue is to mention mailing thread with 
discussions under todo items.

It is done for most of todo items but sometime pressing issues are left out.


That being said, I think this is part of learning process and okay to 
come up with ideas and fail.
Pghackers can possibly bring up issues in their approach (if discussion 
for the issue is not mentioned under
todo item).

-- 
Regards,
Ankit Kumar Pandey