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