Re: [HACKERS] Auto-explain patch

Thanks for the feedback, and sorry for my delay in replying (I was on
holiday).

> Tom Lane wrote:
>
> Comments:
>
> I do not think that you should invent a new elog level for this, and
> especially not one that is designed to send unexpected messages to the
> client. Having to kluge tab completion like that is just a signal that
> you're going to break a lot of other clients too. It seems to me that
> the right behavior for auto-explain messages is to go only to the log by
> default, which means that LOG is already a perfectly good elog level for
> auto-explain messages.

The more I thought about this, the more I thought that it was OTT to
add a new elog level just for this, so I agree it should probably just
go to the LOG elog level.

I don't agree with your reasoning on tab-completion though. I actually
think that this is a signal of a broken client. If the user sets
client_min_messages to LOG or lower, and has any of the other logging
or debugging parameters enabled, the output tramples all over the
suggested completions. I don't think the average user is interested in
log/debug output from the queries psql does internally during
tab-completion. So IMHO the tab-completion 'kludge', is still
worthwhile, regardless of the rest of the patch.

> Drop the query_string addition to PlannedStmt --- there are other ways
> you can get that string in CVS HEAD.

OK. What is the best way to get this string now? Are you referring to
debug_query_string, or is there another way?

> I don't think that planner_time
> belongs there either. It would be impossible to define a principled way
> to compare two PlannedStmts for equality with that in there. Nor do I
> see the point of doing it the way you're doing it. Why don't you just
> log the slow planning cycle immediately upon detecting it in planner()?
> I don't see that a slow planning cycle has anything necessarily to do
> with a slow execution cycle, so IMHO they ought to just get logged
> independently.

Makes sense.

> Please do not export ExplainState --- that's an internal matter for
> explain.c. Export some wrapper function with a cleaner API than
> explain_outNode, instead.
>
> regards, tom lane

OK, that's much neater.

I'll try to rework this ASAP but I understand if it's too late for
this commitfest.

Cheers, Dean.

_________________________________________________________________
Win a voice over part with Kung Fu Panda & Live Search   and   100's of Kung Fu Panda prizes to win with Live Search
http://clk.atdmt.com/UKM/go/107571439/direct/01/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers