Re: [HACKERS] postgres_fdw: support parameterized foreign joins

[Daniel Gustafsson]

> On 11 Apr 2017, at 10:55, Etsuro Fujita  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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] postgres_fdw: support parameterized foreign joins

[Etsuro Fujita]

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




--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] postgres_fdw: support parameterized foreign joins

[David Steele]

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
da...@pgmasters.net


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] postgres_fdw: support parameterized foreign joins

[Arthur Zakirov]

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] postgres_fdw: support parameterized foreign joins

[Etsuro Fujita]

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
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
***
*** 865,871  deparse_type_name(Oid type_oid, int32 typemod)
   * foreign server.
   */
  List *
! build_tlist_to_deparse(RelOptInfo *foreignrel)
  {
  	List	   *tlist = NIL;
  	PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) foreignrel->fdw_private;
--- 865,871 
   * foreign server.
   */
  List *
! build_tlist_to_deparse(RelOptInfo *foreignrel, List *local_param_conds)
  {
  	List	   *tlist = NIL;
  	PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) foreignrel->fdw_private;
***
*** 887,892  build_tlist_to_deparse(RelOptInfo *foreignrel)
--- 887,896 
  	tlist = add_to_flat_tlist(tlist,
  			  pull_var_clause((Node *) fpinfo->local_conds,
  			  PVC_RECURSE_PLACEHOLDERS));
+ 	if (local_param_conds)
+ 		tlist = add_to_flat_tlist(tlist,
+   pull_var_clause((Node *) local_param_conds,
+   PVC_RECURSE_PLACEHOLDERS));
  
  	return tlist;
  }
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 2283,2288  SELECT ft5, ft5.c1, ft5.c2, ft5.c3, ft4.c1, ft4.c2 FROM ft5 left join ft4 on ft5
--- 2283,2343 
   (30,31,AAA030) | 30 | 31 | AAA030 | 30 | 31
  (4 rows)
  
+ -- parameterized remote path for foreign join
+ EXPLAIN (VERBOSE, COSTS OFF)
+   SELECT * FROM "S 1"."T 1" a LEFT JOIN (ft1 b INNER JOIN ft2 c ON (b.c1 = c.c1)) ON (c.c1 = a.c2) WHERE a."C 1" = 47;
+   QUERY PLAN   
+ ---
+  Nested Loop Left Join
+Output: a."C 1", a.c2, a.c3, a.c4, a.c5, a.c6, a.c7, a.c8, b.c1, b.c2, b.c3, b.c4, b.c5, b.c6, b.c7, b.c8, c.c1, c.c2, c.c3, c.c4, c.c5, c.c6, c.c7, c.c8
+->  Index Scan using t1_pkey on "S 1"."T 1" a
+  Output: a."C 1", a.c2, a.c3, a.c4, a.c5, a.c6, a.c7, a.c8
+  Index Cond: 

Re: [HACKERS] postgres_fdw: support parameterized foreign joins

[Arthur Zakirov]

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] postgres_fdw: support parameterized foreign joins

[Arthur Zakirov]

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] postgres_fdw: support parameterized foreign joins

[David Steele]

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
da...@pgmasters.net


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] postgres_fdw: support parameterized foreign joins

[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


postgres-fdw-param-foreign-joins-2.patch
Description: binary/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] postgres_fdw: support parameterized foreign joins

[Etsuro Fujita]

On 2017/03/16 22:23, Arthur Zakirov wrote:

2017-02-27 12:40 GMT+03:00 Etsuro Fujita :

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




--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] postgres_fdw: support parameterized foreign joins

[Arthur Zakirov]

Hello,

2017-02-27 12:40 GMT+03:00 Etsuro Fujita :
> 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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers