auto_explain contrib moudle

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

In response to

Responses

Browse pgsql-hackers by date

  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.