From: | ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> |
---|---|
To: | "Alex Hunsaker" <badalex(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org |
Subject: | auto_explain contrib moudle |
Date: | 2008-10-09 10:06:18 |
Message-ID: | 20081009165157.9BE4.52131E4D@oss.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Thanks for your reviewing, Alex.
I applied your comments to my patch.
- auto_explain.patch : patch against HEAD
- auto_explain.tgz : contrib module
- autoexplain.sgml : documentation
"Alex Hunsaker" <badalex(at)gmail(dot)com> wrote:
> *custom_guc_flags-0828.patch
> My only other concern is the changes to DefineCustom*() to tag the new
> flags param. Now I think anyone who uses Custom gucs will want/should
> be able to set that. I did not see any people in contrib using it but
> did not look on PGfoundry. Do we need to document the change
> somewhere for people who might be using it???
Now it is done with DefineCustomVariable(type, variable) and keep
existing functions as-is for backward compatibility.
Some people will be happy if the functions are documented,
but we need to define 'stable-internal-functions' between
SPI (stable expoted functions) and unstable internal functions.
> *psql_ignore_notices-0828.patch:
> I think this should get dropped.
Hmm, I agree that hiding all messages is not good. I removed it.
If some people need it, we can reconsider it in a separated discussion.
> *auto_explalin.c:
> init_instrument()
> The only "cleaner" way I can
> see is to add a hook for CreateQueryDesc so we can overload
> doInstrument and ExecInitNode will InstrAlloc them all for us.
I wanted to avoid modifying core codes as far as possible,
but I see it was ugly. Now I added 'force_instrument' global
variable as a hook for CreateQueryDesc.
> the only other comment I have is suset_assign() do we really need to
> be a superuser if its all going to LOG ? There was some concern about
> explaining security definer functions right? but surely a regular
> explain on those shows the same thing as this explain? Or what am I
> missing?
Almost logging options in postgres are only for superusers. So I think
auto_explain options should not be modified by non-superusers, too.
If you want to permit usage for users, you can create a security definer
wrapper function to allow it, no?
CREATE FUNCTION set_explain_log_min_duration(text) RETURNS text AS
$$ SELECT set_config('explain.log_min_duration', $1, false); $$
LANGUAGE sql SECURITY DEFINER;
Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center
Attachment | Content-Type | Size |
---|---|---|
auto_explain.patch | application/octet-stream | 13.9 KB |
auto_explain.tgz | application/octet-stream | 1.8 KB |
autoexplain.sgml | application/octet-stream | 4.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Matthew Wakeling | 2008-10-09 10:10:08 | Re: CREATE DATABASE vs delayed table unlink |
Previous Message | Zdenek Kotala | 2008-10-09 07:38:22 | Re: [WIP] Reduce alignment requirements on 64-bit systems. |