Re: Comment about set_join_pathlist_hook()

On Wed, Sep 20, 2023, at 5:05 PM, Etsuro Fujita wrote:
> Hi,
>
> 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.
One more slip-up could be prevented by the comment: removing a path from the pathlist we should remember about the cheapest_* pointers.
Also, it may be good to remind a user, that jointype and extra->sjinfo->jointype aren't the same all the time.

> [1] 
> https://www.postgresql.org/message-id/CACawEhV%3D%2BQ0HXrcDergbTR9EkVFukgRPMTZbRFL-YK5CRmvYag%40mail.gmail.com

-- 
Regards,
Andrei Lepikhov