From: | Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | craig(dot)ringer(at)2ndquadrant(dot)com |
Cc: | tgl(at)sss(dot)pgh(dot)pa(dot)us, pgsql-hackers(at)postgresql(dot)org, coelho(at)cri(dot)ensmp(dot)fr, robertmhaas(at)gmail(dot)com |
Subject: | Re: BUG: pg_stat_statements query normalization issues with combined queries |
Date: | 2017-01-30 07:01:03 |
Message-ID: | 20170130.160103.100755182.horiguchi.kyotaro@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Mmm..
At Sat, 28 Jan 2017 11:52:20 +0800, Craig Ringer <craig(dot)ringer(at)2ndquadrant(dot)com> wrote in <CAMsr+YGX0uU5Rw0fOKFwxtg_5WxWOBQ=KiTHWiKrb65H67inwg(at)mail(dot)gmail(dot)com>
> On 28 Jan. 2017 02:08, "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> writes:
> > By the way the existing comment for the hook,
>
> >> *
> >> * We provide a function hook variable that lets loadable plugins get
> >> * control when ProcessUtility is called. Such a plugin would normally
> >> * call standard_ProcessUtility().
> >> */
>
> > This is quite a matter of course for developers. planner_hook and
> > other ones don't have such a comment or have a one-liner at
> > most. Since this hook gets a good amount of commnet, it seems
> > better to just remove the existing one..
>
> Hm? I see pretty much the same wording for planner_hook:
Sorry, my eyes were very short-ranged. Surely most of them have
commnents with very similar phrase. ExplainOneQuery seems to be
one exception but I'll call ExplainOneQuery in the hook function,
though:p
Anyway I don't want to remove the comment in ProcessUtility since
now I know that is rather the common case.
> * To support loadable plugins that monitor or modify planner behavior,
> * we provide a hook variable that lets a plugin get control before and
> * after the standard planning process. The plugin would normally call
> * standard_planner().
>
> I think other places have copied-and-pasted this as well.
>
> The real problem with this is it isn't a correct specification of the
> contract: in most cases, the right thing to do is more like "call the
> previous occupant of the ProcessUtility_hook, or standard_ProcessUtility
> if there was none."
>
> Not sure if it's worth trying to be more precise in these comments,
> but if we do something about it, we need to hit all the places with
> this wording.
That seems bad. I'll prefer rather leaving them alone. Modifying
them wouldn't be so much gain if many of them already have such
comment.
> I'd rather leave it for the hooks documentation work that's being done
> elsewhere and have a README.hooks or something that discusses patterns
> common across hooks. Then we can just reference it.
>
> Otherwise we'll have a lot of repetitive comments. Especially once we
> mention that you can also ERROR out or (potentially) take no action at all.
Sorry for the noise..
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
From | Date | Subject | |
---|---|---|---|
Next Message | Andrew Borodin | 2017-01-30 07:28:11 | Re: Review: GIN non-intrusive vacuum of posting tree |
Previous Message | Ashutosh Bapat | 2017-01-30 06:42:27 | Re: Create a separate test file for exercising system views |