Re: BUG: pg_stat_statements query normalization issues with combined queries

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

In response to

Browse pgsql-hackers by date

  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