Mmm..
At Sat, 28 Jan 2017 11:52:20 +0800, Craig Ringer <craig.ringer@2ndquadrant.com> wrote in
<CAMsr+YGX0uU5Rw0fOKFwxtg_5WxWOBQ=KiTHWiKrb65H67inwg@mail.gmail.com>
> On 28 Jan. 2017 02:08, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
>
> Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.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