Re: Auto-explain patch

From: Dean Rasheed <dean_rasheed(at)hotmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Simon Riggs <simon(at)2ndquadrant(dot)com>, <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Auto-explain patch
Date: 2008-08-03 09:05:06
Message-ID: BAY102-W24C752A45DE8143779CE96F2790@phx.gbl
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers


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/

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Gregory Stark 2008-08-03 10:49:13 Re: Parsing of pg_hba.conf and authentication inconsistencies
Previous Message Magnus Hagander 2008-08-03 08:36:56 Re: Parsing of pg_hba.conf and authentication inconsistencies