Re: postgres_fdw: support parameterized foreign joins

On 23.03.2017 15:45, Etsuro Fujita wrote:
>
> Done.  Also, I added regression tests and revised code and comments a
> bit.  (As for create_foreignscan_path(), I haven't done anything about
> that yet.)  Please find attached a new version created on top of [1].

Thank you!

I didn't notice that it is necessary to apply the patch 
"epqpath-for-foreignjoin".

I have a few comments.

>    * 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"

>
> !             Assert(foreignrel->reloptkind == RELOPT_JOINREL);
> !

Here the new macro IS_JOIN_REL() can be used.

> !             /* 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?

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.

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company