Re: Comment about set_join_pathlist_hook()

Hi,

On Thu, Sep 21, 2023 at 11:49 AM Lepikhov Andrei
<a(dot)lepikhov(at)postgrespro(dot)ru> wrote:
> On Wed, Sep 20, 2023, at 5:05 PM, Etsuro Fujita wrote:
> > What I am concerned about from the report [1] is that this comment is
> > a bit too terse; it might cause a misunderstanding that extensions can
> > do different things than we intend to allow:
> >
> > /*
> > * 6. Finally, give extensions a chance to manipulate the path list.
> > */
> > if (set_join_pathlist_hook)
> > set_join_pathlist_hook(root, joinrel, outerrel, innerrel,
> > jointype, &extra);
> >
> > So I would like to propose to extend the comment to explain what they
> > can do, as in the comment about set_rel_pathlist_hook() in allpaths.c.
> > Attached is a patch for that.
>
> It makes sense. But why do you restrict addition to pathlist by only the add_path() routine? It can fail to add a path to the pathlist. We need to find out the result of the add_path operation and need to check the pathlist each time. So, sometimes, it can be better to add a path manually.

I do not agree with you on this point; I think you can do so at your
own responsibility, but I think it is better for extensions to use
add_path(), because that makes them stable. (Assuming that add_path()
has a bug and we change the logic of it to fix the bug, extensions
that do not follow the standard procedure might not work anymore.)

> One more slip-up could be prevented by the comment: removing a path from the pathlist we should remember about the cheapest_* pointers.

Do we really need to do this? I mean we do set_cheapest() afterward.
See standard_join_search().

> Also, it may be good to remind a user, that jointype and extra->sjinfo->jointype aren't the same all the time.

That might be an improvement, but IMO that is not the point here,
because the purpose to expand the comment is to avoid extensions doing
different things than we intend to allow.

Thanks for looking!

Best regards,
Etsuro Fujita