Re: auto_explain contrib moudle

On Thu, Oct 9, 2008 at 03:06, ITAGAKI Takahiro
<itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> wrote:
> Thanks for your reviewing, Alex.
> I applied your comments to my patch.

Sorry for the late reply! Somehow I missed this, saw it on the commit
fest wiki :)

>> *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.

Ok that seems better...

> 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.

Right, thats why I was asking :)

>> *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.

Yeah, well if we are not to worried about it getting out of sync when
people add new node/scan types what you had before was probably ok. I
was just trying to stimulate my own and maybe others brains who are on
the list that might have better ideas. But at least now the commiter
has 2 options here :)

>> 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.

Ok thanks that makes sense.