Thread: Comment about set_join_pathlist_hook()
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.
Best regards,
Etsuro Fujita
[1] https://www.postgresql.org/message-id/CACawEhV%3D%2BQ0HXrcDergbTR9EkVFukgRPMTZbRFL-YK5CRmvYag%40mail.gmail.com
Attachment
On Wed, 20 Sept 2023 at 22:06, Etsuro Fujita <etsuro.fujita@gmail.com> wrote: > 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. Looks good to me. I see you've copy/edited the comment just above the call to set_rel_pathlist_hook(). That makes sense. David
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 tothe 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
Hi, On Thu, Sep 21, 2023 at 11:49 AM Lepikhov Andrei <a.lepikhov@postgrespro.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 pathto 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
On Thu, Sep 21, 2023, at 12:53 PM, Etsuro Fujita wrote: > Hi, > > On Thu, Sep 21, 2023 at 11:49 AM Lepikhov Andrei > <a.lepikhov@postgrespro.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 pathto 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.) Ok, I got it.This question related to the add_path() interface itself, not to the comment. It is awkward to check every timein the pathlist the result of the add_path. >> 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(). Agree, in the case of current join it doesn't make sense. I stuck in this situation because providing additional path atthe current level I need to arrange paths for the inner and outer too. Thanks for the explanation! -- Regards, Andrei Lepikhov
Hi, On Wed, Sep 20, 2023 at 7:49 PM David Rowley <dgrowleyml@gmail.com> wrote: > On Wed, 20 Sept 2023 at 22:06, Etsuro Fujita <etsuro.fujita@gmail.com> wrote: > > 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. > > Looks good to me. > > I see you've copy/edited the comment just above the call to > set_rel_pathlist_hook(). That makes sense. Cool! Pushed. Thanks for taking a look! Best regards, Etsuro Fujita