Re: Custom/Foreign-Join-APIs (Re: [v9.5] Custom Plan API) - Mailing list pgsql-hackers

2015-05-09 8:32 GMT+09:00 Kohei KaiGai <kaigai@kaigai.gr.jp>:
> 2015-05-09 3:51 GMT+09:00 Tom Lane <tgl@sss.pgh.pa.us>:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> On Fri, May 8, 2015 at 1:46 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> That's nice, but 9.5 feature freeze is only a week away.  I don't have a
>>>> lot of confidence that this stuff is actually in a state where we won't
>>>> regret shipping it in 9.5.
>>
>>> Yeah.  The POC you were asking for upthread certainly exists and has
>>> for a while, or I would not have committed this.  But I do not think
>>> it likely that the  postgres_fdw support will be ready for 9.5.
>>
>> Well, we have two alternatives.  I can keep hacking on this and get it
>> to a state where it seems credible to me, but we won't have any proof
>> that it actually works (though perhaps we could treat any problems
>> as bugs that should hopefully get found before 9.5 ships, if a
>> postgres_fdw patch shows up in the next few months).  Or we could
>> revert the whole thing and bounce it to the 9.6 cycle.  I don't really
>> like doing the latter, but I'm pretty uncomfortable with committing to
>> published FDW APIs that are (a) as messy as this and (b) practically
>> untested.  The odds that something slipped through the cracks are high.
>>
>> Aside from the other gripes I raised, I'm exceedingly unhappy with the
>> ad-hoc APIs proposed for GetForeignJoinPaths and set_join_pathlist_hook.
>> It's okay for internal calls in joinpath.c to look like that, but
>> exporting that set of parameters seems like pure folly.  We've changed
>> those parameter lists repeatedly (for instance in 9.2 and again in 9.3);
>> the odds that they'll need to change again in future approach 100%.
>>
>> One way we could reduce the risk of code breakage there is to stuff all
>> or most of those parameters into a struct.  This might result in a small
>> slowdown for the internal calls, or then again maybe not --- there
>> probably aren't many architectures that can pass 10 parameters in
>> registers anyway.
>>
> Is it like a following structure definition?
>
>   typedef struct
>   {
>     PlannerInfo *root;
>     RelOptInfo *joinrel;
>     RelOptInfo *outerrel;
>     RelOptInfo *innerrel;
>     List *restrictlist;
>     JoinType jointype;
>     SpecialJoinInfo *sjinfo;
>     SemiAntiJoinFactors *semifactors;
>     Relids param_source_rels;
>     Relids extra_lateral_rels;
>   } SetJoinPathListArgs;
>
> I agree the idea. It also helps CSP driver implementation where it calls
> next driver that was already chained on its installation.
>
>   if (set_join_pathlist_next)
>       set_join_pathlist_next(args);
>
> is more stable manner than
>
>   if (set_join_pathlist_next)
>       set_join_pathlist_next(root,
>                                        joinrel,
>                                        outerrel,
>                                        innerrel,
>                                        restrictlist,
>                                        jointype,
>                                        sjinfo,
>                                        semifactors,
>                                        param_source_rels,
>                                        extra_lateral_rels);
>
The attached patch newly defines ExtraJoinPathArgs struct to pack
all the necessary information to be delivered on GetForeignJoinPaths
and set_join_pathlist_hook.

Its definition is below:
  typedef struct
  {
     PlannerInfo    *root;
     RelOptInfo     *joinrel;
     RelOptInfo     *outerrel;
     RelOptInfo     *innerrel;
     List           *restrictlist;
     JoinType        jointype;
     SpecialJoinInfo *sjinfo;
     SemiAntiJoinFactors *semifactors;
     Relids          param_source_rels;
     Relids          extra_lateral_rels;
  } ExtraJoinPathArgs;

then, hook invocation gets much simplified, like:

        /*
         * 6. Finally, give extensions a chance to manipulate the path list.
         */
        if (set_join_pathlist_hook)
            set_join_pathlist_hook(&jargs);

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>