Re: Comment about set_join_pathlist_hook()

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 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.)

Ok, I got it.This question related to the add_path() interface itself, not to the comment. It is awkward to check every time in 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 at the current level I need to arrange paths for the inner and outer too.

Thanks for the explanation!

-- 
Regards,
Andrei Lepikhov