PlaceHolderVars in pushed down child-join cause error

2018-02-22 Thread Ashutosh Bapat
Hi,
postgres_fdw isn't expected to push down joins with placeholder vars.
But the check for that in foreign_join_ok() only considers
joinrel->relids. For a child-join relids contains the child relids but
PlaceHolderInfo refers to the top parent's relids. Hence postgres_fdw
tries to push down a child-join with PlaceHolderVars in it and fails
with error "unsupported expression type for deparse: 198". 198 being
T_PlaceHolderVar.

The fix is to use joinrel->top_parent_relids for a child-join.
Attached patch for the same.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 262c635..b0636c9 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -7800,4 +7800,44 @@ SELECT t1.a,t1.b FROM fprt1 t1, LATERAL (SELECT t2.a, t2.b FROM fprt2 t2 WHERE t
  400 | 400
 (4 rows)
 
+-- with PHVs, partition-wise join selected but no join pushdown
+EXPLAIN (COSTS OFF)
+SELECT t1.a, t1.phv, t2.b, t2.phv FROM (SELECT 't1_phv' phv, * FROM fprt1 WHERE a % 25 = 0) t1 FULL JOIN (SELECT 't2_phv' phv, * FROM fprt2 WHERE b % 25 = 0) t2 ON (t1.a = t2.b) ORDER BY t1.a, t2.b;
+ QUERY PLAN 
+
+ Sort
+   Sort Key: ftprt1_p1.a, ftprt2_p1.b
+   ->  Result
+ ->  Append
+   ->  Hash Full Join
+ Hash Cond: (ftprt1_p1.a = ftprt2_p1.b)
+ ->  Foreign Scan on ftprt1_p1
+ ->  Hash
+   ->  Foreign Scan on ftprt2_p1
+   ->  Hash Full Join
+ Hash Cond: (ftprt1_p2.a = ftprt2_p2.b)
+ ->  Foreign Scan on ftprt1_p2
+ ->  Hash
+   ->  Foreign Scan on ftprt2_p2
+(14 rows)
+
+SELECT t1.a, t1.phv, t2.b, t2.phv FROM (SELECT 't1_phv' phv, * FROM fprt1 WHERE a % 25 = 0) t1 FULL JOIN (SELECT 't2_phv' phv, * FROM fprt2 WHERE b % 25 = 0) t2 ON (t1.a = t2.b) ORDER BY t1.a, t2.b;
+  a  |  phv   |  b  |  phv   
+-++-+
+   0 | t1_phv |   0 | t2_phv
+  50 | t1_phv | | 
+ 100 | t1_phv | | 
+ 150 | t1_phv | 150 | t2_phv
+ 200 | t1_phv | | 
+ 250 | t1_phv | 250 | t2_phv
+ 300 | t1_phv | | 
+ 350 | t1_phv | | 
+ 400 | t1_phv | 400 | t2_phv
+ 450 | t1_phv | | 
+ ||  75 | t2_phv
+ || 225 | t2_phv
+ || 325 | t2_phv
+ || 475 | t2_phv
+(14 rows)
+
 RESET enable_partitionwise_join;
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index d37180a..5c07f0a 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -4565,7 +4565,9 @@ foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel, JoinType jointype,
 	foreach(lc, root->placeholder_list)
 	{
 		PlaceHolderInfo *phinfo = lfirst(lc);
-		Relids		relids = joinrel->relids;
+		/* PlaceHolderInfo refers to parent relids and not those of a child. */
+		Relids		relids = IS_OTHER_REL(joinrel) ?
+joinrel->top_parent_relids : joinrel->relids;
 
 		if (bms_is_subset(phinfo->ph_eval_at, relids) &&
 			bms_nonempty_difference(relids, phinfo->ph_eval_at))
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 2863549..9e47729 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -1913,4 +1913,9 @@ EXPLAIN (COSTS OFF)
 SELECT t1.a,t1.b FROM fprt1 t1, LATERAL (SELECT t2.a, t2.b FROM fprt2 t2 WHERE t1.a = t2.b AND t1.b = t2.a) q WHERE t1.a%25 = 0 ORDER BY 1,2;
 SELECT t1.a,t1.b FROM fprt1 t1, LATERAL (SELECT t2.a, t2.b FROM fprt2 t2 WHERE t1.a = t2.b AND t1.b = t2.a) q WHERE t1.a%25 = 0 ORDER BY 1,2;
 
+-- with PHVs, partition-wise join selected but no join pushdown
+EXPLAIN (COSTS OFF)
+SELECT t1.a, t1.phv, t2.b, t2.phv FROM (SELECT 't1_phv' phv, * FROM fprt1 WHERE a % 25 = 0) t1 FULL JOIN (SELECT 't2_phv' phv, * FROM fprt2 WHERE b % 25 = 0) t2 ON (t1.a = t2.b) ORDER BY t1.a, t2.b;
+SELECT t1.a, t1.phv, t2.b, t2.phv FROM (SELECT 't1_phv' phv, * FROM fprt1 WHERE a % 25 = 0) t1 FULL JOIN (SELECT 't2_phv' phv, * FROM fprt2 WHERE b % 25 = 0) t2 ON (t1.a = t2.b) ORDER BY t1.a, t2.b;
+
 RESET enable_partitionwise_join;


PlaceHolderVars in pushed down child-join cause error

2018-02-22 Thread Ashutosh Bapat
Hi,
postgres_fdw isn't expected to push down joins with placeholder vars.
But the check for that in foreign_join_ok() only considers
joinrel->relids. For a child-join relids contains the child relids but
PlaceHolderInfo refers to the top parent's relids. Hence postgres_fdw
tries to push down a child-join with PlaceHolderVars in it and fails
with error "unsupported expression type for deparse: 198". 198 being
T_PlaceHolderVar.

The fix is to use joinrel->top_parent_relids for a child-join.
Attached patch for the same.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 262c635..b0636c9 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -7800,4 +7800,44 @@ SELECT t1.a,t1.b FROM fprt1 t1, LATERAL (SELECT t2.a, t2.b FROM fprt2 t2 WHERE t
  400 | 400
 (4 rows)
 
+-- with PHVs, partition-wise join selected but no join pushdown
+EXPLAIN (COSTS OFF)
+SELECT t1.a, t1.phv, t2.b, t2.phv FROM (SELECT 't1_phv' phv, * FROM fprt1 WHERE a % 25 = 0) t1 FULL JOIN (SELECT 't2_phv' phv, * FROM fprt2 WHERE b % 25 = 0) t2 ON (t1.a = t2.b) ORDER BY t1.a, t2.b;
+ QUERY PLAN 
+
+ Sort
+   Sort Key: ftprt1_p1.a, ftprt2_p1.b
+   ->  Result
+ ->  Append
+   ->  Hash Full Join
+ Hash Cond: (ftprt1_p1.a = ftprt2_p1.b)
+ ->  Foreign Scan on ftprt1_p1
+ ->  Hash
+   ->  Foreign Scan on ftprt2_p1
+   ->  Hash Full Join
+ Hash Cond: (ftprt1_p2.a = ftprt2_p2.b)
+ ->  Foreign Scan on ftprt1_p2
+ ->  Hash
+   ->  Foreign Scan on ftprt2_p2
+(14 rows)
+
+SELECT t1.a, t1.phv, t2.b, t2.phv FROM (SELECT 't1_phv' phv, * FROM fprt1 WHERE a % 25 = 0) t1 FULL JOIN (SELECT 't2_phv' phv, * FROM fprt2 WHERE b % 25 = 0) t2 ON (t1.a = t2.b) ORDER BY t1.a, t2.b;
+  a  |  phv   |  b  |  phv   
+-++-+
+   0 | t1_phv |   0 | t2_phv
+  50 | t1_phv | | 
+ 100 | t1_phv | | 
+ 150 | t1_phv | 150 | t2_phv
+ 200 | t1_phv | | 
+ 250 | t1_phv | 250 | t2_phv
+ 300 | t1_phv | | 
+ 350 | t1_phv | | 
+ 400 | t1_phv | 400 | t2_phv
+ 450 | t1_phv | | 
+ ||  75 | t2_phv
+ || 225 | t2_phv
+ || 325 | t2_phv
+ || 475 | t2_phv
+(14 rows)
+
 RESET enable_partitionwise_join;
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index d37180a..5c07f0a 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -4565,7 +4565,9 @@ foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel, JoinType jointype,
 	foreach(lc, root->placeholder_list)
 	{
 		PlaceHolderInfo *phinfo = lfirst(lc);
-		Relids		relids = joinrel->relids;
+		/* PlaceHolderInfo refers to parent relids and not those of a child. */
+		Relids		relids = IS_OTHER_REL(joinrel) ?
+joinrel->top_parent_relids : joinrel->relids;
 
 		if (bms_is_subset(phinfo->ph_eval_at, relids) &&
 			bms_nonempty_difference(relids, phinfo->ph_eval_at))
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 2863549..9e47729 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -1913,4 +1913,9 @@ EXPLAIN (COSTS OFF)
 SELECT t1.a,t1.b FROM fprt1 t1, LATERAL (SELECT t2.a, t2.b FROM fprt2 t2 WHERE t1.a = t2.b AND t1.b = t2.a) q WHERE t1.a%25 = 0 ORDER BY 1,2;
 SELECT t1.a,t1.b FROM fprt1 t1, LATERAL (SELECT t2.a, t2.b FROM fprt2 t2 WHERE t1.a = t2.b AND t1.b = t2.a) q WHERE t1.a%25 = 0 ORDER BY 1,2;
 
+-- with PHVs, partition-wise join selected but no join pushdown
+EXPLAIN (COSTS OFF)
+SELECT t1.a, t1.phv, t2.b, t2.phv FROM (SELECT 't1_phv' phv, * FROM fprt1 WHERE a % 25 = 0) t1 FULL JOIN (SELECT 't2_phv' phv, * FROM fprt2 WHERE b % 25 = 0) t2 ON (t1.a = t2.b) ORDER BY t1.a, t2.b;
+SELECT t1.a, t1.phv, t2.b, t2.phv FROM (SELECT 't1_phv' phv, * FROM fprt1 WHERE a % 25 = 0) t1 FULL JOIN (SELECT 't2_phv' phv, * FROM fprt2 WHERE b % 25 = 0) t2 ON (t1.a = t2.b) ORDER BY t1.a, t2.b;
+
 RESET enable_partitionwise_join;


Re: PlaceHolderVars in pushed down child-join cause error

2018-02-22 Thread Robert Haas
On Thu, Feb 22, 2018 at 7:41 AM, Ashutosh Bapat
 wrote:
> postgres_fdw isn't expected to push down joins with placeholder vars.
> But the check for that in foreign_join_ok() only considers
> joinrel->relids. For a child-join relids contains the child relids but
> PlaceHolderInfo refers to the top parent's relids. Hence postgres_fdw
> tries to push down a child-join with PlaceHolderVars in it and fails
> with error "unsupported expression type for deparse: 198". 198 being
> T_PlaceHolderVar.
>
> The fix is to use joinrel->top_parent_relids for a child-join.
> Attached patch for the same.

Committed, but I changed the formatting, because as you had it,
pgindent would have mangled it.  While I was at it, I tweaked the
wording of the comment a bit.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company



Re: PlaceHolderVars in pushed down child-join cause error

2018-02-25 Thread Ashutosh Bapat
On Thu, Feb 22, 2018 at 8:36 PM, Robert Haas  wrote:
> On Thu, Feb 22, 2018 at 7:41 AM, Ashutosh Bapat
>  wrote:
>> postgres_fdw isn't expected to push down joins with placeholder vars.
>> But the check for that in foreign_join_ok() only considers
>> joinrel->relids. For a child-join relids contains the child relids but
>> PlaceHolderInfo refers to the top parent's relids. Hence postgres_fdw
>> tries to push down a child-join with PlaceHolderVars in it and fails
>> with error "unsupported expression type for deparse: 198". 198 being
>> T_PlaceHolderVar.
>>
>> The fix is to use joinrel->top_parent_relids for a child-join.
>> Attached patch for the same.
>
> Committed, but I changed the formatting, because as you had it,
> pgindent would have mangled it.  While I was at it, I tweaked the
> wording of the comment a bit.

Thanks.


-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company



Re: PlaceHolderVars in pushed down child-join cause error

2018-02-22 Thread Robert Haas
On Thu, Feb 22, 2018 at 7:41 AM, Ashutosh Bapat
 wrote:
> postgres_fdw isn't expected to push down joins with placeholder vars.
> But the check for that in foreign_join_ok() only considers
> joinrel->relids. For a child-join relids contains the child relids but
> PlaceHolderInfo refers to the top parent's relids. Hence postgres_fdw
> tries to push down a child-join with PlaceHolderVars in it and fails
> with error "unsupported expression type for deparse: 198". 198 being
> T_PlaceHolderVar.
>
> The fix is to use joinrel->top_parent_relids for a child-join.
> Attached patch for the same.

Committed, but I changed the formatting, because as you had it,
pgindent would have mangled it.  While I was at it, I tweaked the
wording of the comment a bit.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company



Re: PlaceHolderVars in pushed down child-join cause error

2018-02-25 Thread Ashutosh Bapat
On Thu, Feb 22, 2018 at 8:36 PM, Robert Haas  wrote:
> On Thu, Feb 22, 2018 at 7:41 AM, Ashutosh Bapat
>  wrote:
>> postgres_fdw isn't expected to push down joins with placeholder vars.
>> But the check for that in foreign_join_ok() only considers
>> joinrel->relids. For a child-join relids contains the child relids but
>> PlaceHolderInfo refers to the top parent's relids. Hence postgres_fdw
>> tries to push down a child-join with PlaceHolderVars in it and fails
>> with error "unsupported expression type for deparse: 198". 198 being
>> T_PlaceHolderVar.
>>
>> The fix is to use joinrel->top_parent_relids for a child-join.
>> Attached patch for the same.
>
> Committed, but I changed the formatting, because as you had it,
> pgindent would have mangled it.  While I was at it, I tweaked the
> wording of the comment a bit.

Thanks.


-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company