Re: postgres_fdw: support parameterized foreign joins

Hi Arthur,

On 2017/04/05 0:55, Arthur Zakirov wrote:
> On 23.03.2017 15:45, Etsuro Fujita wrote:

> I have a few comments.

Thank you for the review!

>>    * innersortkeys are the sort pathkeys for the inner side of the
>> mergejoin
>> +  * req_outer_list is a list of sensible parameterizations for the
>> join rel
>>    */
>
> I think it would be better if the comment explained what type is stored
> in req_outer_list. So the following comment would be good:
>
> "req_outer_list is a list of Relids of sensible parameterizations for
> the join rel"

Done.

>>
>> !             Assert(foreignrel->reloptkind == RELOPT_JOINREL);
>> !
>
> Here the new macro IS_JOIN_REL() can be used.

Done.

>> !             /* Get the remote and local conditions */
>> !             remote_conds =
>> list_concat(list_copy(remote_param_join_conds),
>> !                                        fpinfo->remote_conds);
>> !             local_param_join_exprs =
>> !                 get_actual_clauses(local_param_join_conds);
>> !             local_exprs =
>> list_concat(list_copy(local_param_join_exprs),
>> !                                       fpinfo->local_conds);
>
> Is this code correct? 'remote_conds' and 'local_exprs' are initialized
> above when 'scan_clauses' is separated. Maybe better to use
> 'remote_conds' and 'local_exprs' instead of 'fpinfo->remote_conds' and
> 'fpinfo->local_conds' respectively?

Let me explain.  As described in the comment in postgresGetForeignPlan:

     if (IS_SIMPLE_REL(foreignrel))
         scan_relid = foreignrel->relid;
     else
     {
         scan_relid = 0;

         /*
          * create_scan_plan() and create_foreignscan_plan() pass
          * rel->baserestrictinfo + parameterization clauses through
          * scan_clauses, but for a join or upper relation, there should 
be no
          * scan_clauses.
          */
         Assert(!scan_clauses);
     }

scan_clauses=NIL for a join relation.  So, for a join relation we use 
fpinfo->remote_conds and fpinfo->local_conds, instead.  (Note that those 
conditions are created at path creation time, ie, 
postgresGetForeignJoinPaths.  See foreign_join_ok.)

> And the last. The patch needs rebasing because new macroses
> IS_JOIN_REL() and IS_UPPER_REL() were added. And the patch is applied
> with errors.

Rebased.

Attached is an updated version created on top of the latest patch 
"epqpath-for-foreignjoin" [1].

Other changes:
* Added a bit more regression tests with FOR UPDATE clause to see if 
CreateLocalJoinPath works well for parameterized foreign join paths.
* Added/revised comments a bit.

Best regards,
Etsuro Fujita

[1] 
https://www.postgresql.org/message-id/424933d7-d6bb-4b8f-4e44-1fea212af083%40lab.ntt.co.jp