On 28 Jan. 2017 02:08, "Tom Lane" <t...@sss.pgh.pa.us> wrote:
Kyotaro HORIGUCHI <horiguchi.kyot...@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: * 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. 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.
