Re: postgres_fdw: support parameterized foreign joins

Hi,

I'd like to propose to support parameterized foreign joins. Attached is
a patch for that, which has been created on top of [1].

In [2], I said that postgres_fdw could create parameterized paths from
joinable combinations of the cheapest-parameterized paths for the
inner/outer relations, but for identifying the joinable combinations,
postgres_fdw would need to do the same work as the core, which wouldn't
be good. Also, I thought that the parameterized paths could be created
by using the required_outer relations in ParamPathInfos stored in the
join relation's ppilist, which I thought would have already built
ParamPathInfos for parameterized local-join paths, but I noticed it
isn't guaranteed that such local-join paths are always created and their
ParamPathInfos are always stored in the pplilist. Instead, I'd propose
to collect the required_outer outer relations that the core tried to
create parameterized local-join paths for during add_paths_to_joinrel(),
and build parameterized foreign-join paths for those outer relations
during postgresGetForeignJoinPaths().

Here is an example:

postgres=# create extension postgres_fdw;
CREATE EXTENSION
postgres=# create server loopback foreign data wrapper postgres_fdw
options (dbname 'postgres');
CREATE SERVER
postgres=# create user mapping for public server loopback;
CREATE USER MAPPING
postgres=# create table t1 (a int , b int, CONSTRAINT t1_pkey PRIMARY
KEY (a));
CREATE TABLE
postgres=# create table t2 (a int , b int, CONSTRAINT t2_pkey PRIMARY
KEY (a));
CREATE TABLE
postgres=# create foreign table ft1 (a int, b int) server loopback
options (table_name 't1');
CREATE FOREIGN TABLE
postgres=# create foreign table ft2 (a int, b int) server loopback
options (table_name 't2');
CREATE FOREIGN TABLE
postgres=# insert into t1 select id, id % 10 from generate_series(1,
10000) id;
INSERT 0 10000
postgres=# insert into t2 select id, id % 10 from generate_series(1,
10000) id;
INSERT 0 10000
postgres=# alter foreign table ft1 options (use_remote_estimate 'true');
ALTER FOREIGN TABLE
postgres=# alter foreign table ft2 options (use_remote_estimate 'true');
ALTER FOREIGN TABLE
postgres=# create table test (a int, b int);
CREATE TABLE
postgres=# insert into test values (1, 1);
INSERT 0 1
postgres=# analyze test;
ANALYZE
postgres=# explain verbose select * from test r1 left join (ft1 r2 inner
join ft2 r3 on (r2.a = r3.a)) on (r3.a = r1.a) limit 1;

QUERY PLAN
-------------------------------------------------------------------------------------------------------------------------------------------------------
Limit (cost=100.58..100.92 rows=1 width=24)
Output: r1.a, r1.b, r2.a, r2.b, r3.a, r3.b
-> Nested Loop Left Join (cost=100.58..117.67 rows=50 width=24)
Output: r1.a, r1.b, r2.a, r2.b, r3.a, r3.b
-> Seq Scan on public.test r1 (cost=0.00..1.01 rows=1 width=8)
Output: r1.a, r1.b
-> Foreign Scan (cost=100.58..116.65 rows=1 width=16)
Output: r2.a, r2.b, r3.a, r3.b
Relations: (public.ft1 r2) INNER JOIN (public.ft2 r3)
Remote SQL: SELECT r2.a, r2.b, r3.a, r3.b FROM
(public.t1 r2 INNER JOIN public.t2 r3 ON (((r2.a = r3.a)))) WHERE ((r3.a
= $1::integer))
(10 rows)

Notes:
* Since add_paths_to_joinrel() for join {B, A} might provide different
parameterizations of result local-join paths from that for join {A, B},
so the patch allows postgresGetForeignJoinPaths() to build paths after
that function has pushdown_safe=true.
* create_foreignscan_path() only calls get_baserel_parampathinfo() to
set the param_info member. We would need to do something about that so
it can handle the parameterized-foreign-join-path case properly. Though
I left that function as-is because get_baserel_parampathinfo() can
return the ParamPathInfo created in postgresGetForeignJoinPaths() for an
input parameterized foreign-join path, by accident.

I'll add this to the upcoming commitfest.

Best regards,
Etsuro Fujita

[1]
https://www.postgresql.org/message-id/0700eb97-d9db-33da-4ba2-e28d2a1631d9%40lab.ntt.co.jp
[2]
https://www.postgresql.org/message-id/e18b9bf5-1557-cb9c-001e-0861a1d7dfce%40lab.ntt.co.jp

Hello,

2017-02-27 12:40 GMT+03:00 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
> Hi,
>
> I'd like to propose to support parameterized foreign joins. Attached is a
> patch for that, which has been created on top of [1].
>

Can you rebase the patch? It is not applied now.

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

On 2017/03/16 22:23, Arthur Zakirov wrote:
> 2017-02-27 12:40 GMT+03:00 Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp>:
>> I'd like to propose to support parameterized foreign joins. Attached is a
>> patch for that, which has been created on top of [1].

> Can you rebase the patch? It is not applied now.

Ok, will do. Thanks for the report!

Best regards,
Etsuro Fujita

On 2017/03/21 18:38, Etsuro Fujita wrote:
> On 2017/03/16 22:23, Arthur Zakirov wrote:
>> Can you rebase the patch? It is not applied now.

> Ok, will do. Thanks for the report!

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].

Best regards,
Etsuro Fujita

[1]
https://www.postgresql.org/message-id/79b98e30-4d38-7deb-f1fb-bc0bc589a958%40lab.ntt.co.jp

Hi Arthur.

On 3/23/17 8:45 AM, Etsuro Fujita wrote:
> On 2017/03/21 18:38, Etsuro Fujita wrote:
>> On 2017/03/16 22:23, Arthur Zakirov wrote:
>>> Can you rebase the patch? It is not applied now.
>
>> Ok, will do. Thanks for the report!
>
> 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].

Are you planning to review this patch?

Thanks,
--
-David
david(at)pgmasters(dot)net

Hello,

On 31.03.2017 18:47, David Steele wrote:
> Hi Arthur.
>
> On 3/23/17 8:45 AM, 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].
>
> Are you planning to review this patch?
>
> Thanks,

Yes, I wanted to review the patch. But there was a lack of time this
week. I marked myself as reviewer in the commitfest app.

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

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

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

On 05.04.2017 12:20, Etsuro Fujita wrote:
>
> 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.
>

Thank you!

> 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.)

Oh, I see. I've missed that condition.

Marked the patch as "Ready for Commiter". But the patch should be
commited only after the patch [1].

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

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

On 4/7/17 9:54 AM, Arthur Zakirov wrote:
>
> Marked the patch as "Ready for Commiter". But the patch should be
> commited only after the patch [1].

This submission has been moved to CF 2017-07.

--
-David
david(at)pgmasters(dot)net

On 2017/04/07 22:54, Arthur Zakirov wrote:
> Marked the patch as "Ready for Commiter". But the patch should be
> commited only after the patch [1].

Thanks for reviewing! I'll continue to work on this for PG11.

Best regards,
Etsuro Fujita

> On 11 Apr 2017, at 10:55, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>
> On 2017/04/07 22:54, Arthur Zakirov wrote:
>> Marked the patch as "Ready for Commiter". But the patch should be
>> commited only after the patch [1].
>
> Thanks for reviewing! I'll continue to work on this for PG11.

This patch has been marked Ready for Committer in the current commitfest
without being committed or rejected. Moving to the next commitfest, but since
it has bitrotted I’m moving it to Waiting for author.

cheers ./daniel

On Mon, Oct 2, 2017 at 9:06 AM, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> This patch has been marked Ready for Committer in the current commitfest
> without being committed or rejected. Moving to the next commitfest, but since
> it has bitrotted I’m moving it to Waiting for author.

One month and a half after, the already-rotten patch has not been
updated, so I am marking it as returned with feedback. Of course feel
free to come back to it.
--
Michael