Re: postgres_fdw: support parameterized foreign joins

From: Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>
To: Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>
Cc: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: postgres_fdw: support parameterized foreign joins
Date: 2017-04-05 09:20:42
Message-ID: debdb1dd-4298-1e81-20a9-c57818fd4ac4@lab.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
postgres-fdw-param-foreign-joins-3.patch text/x-patch 27.2 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Khandekar 2017-04-05 09:22:38 Re: Parallel Append implementation
Previous Message Craig Ringer 2017-04-05 09:18:24 Re: Logical decoding on standby