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
