Push down join with PHVs (WAS: Re: [HACKERS] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116)

[Etsuro Fujita]

On 2016/06/17 21:45, Robert Haas wrote:

On Thu, Jun 16, 2016 at 10:44 PM, Etsuro Fujita
 wrote:

On 2016/06/16 22:00, Robert Haas wrote:

On Thu, Jun 16, 2016 at 7:05 AM, Etsuro Fujita
 wrote:


ISTM that a robuster solution to this is to push down the ft1-ft2-ft3
join
with the PHV by extending deparseExplicitTargetList() and/or something
else
so that we can send the remote query like:

select ft1.a, (ft3.a IS NOT NULL) from (ft1 inner join ft2 on ft1.a =
ft2.a)
left join ft3 on ft1.a = ft3.a



I completely agree we should have that, but not for 9.6.



OK, I'll work on that for 10.0.



Great, that will be a nice improvement.


Here is a patch for that.

* Modified deparseExplicitTargetList as well as is_foreign_expr and 
deparseExpr so that a join is pushed down with PHVs if not only the join 
but the PHVs are safe to execute remotely.  The existing deparsing logic 
can't build a remote query that involves subqueries, so the patch 
currently gives up pushing down any upper joins involving a join (or 
foreign table!) with PHVs in the tlist.  But I think the patch would be 
easily extended to support that, once we support deparsing subqueries.


* Besides that, simplified foreign_join_ok a bit, by adding a new flag, 
allow_upper_foreign_join, to PgFdwRelationInfo, replacing the existing 
pushdown_safe flag with it.  See the comments in postgres_fdw.h.


Comments welcome.

Best regards,
Etsuro Fujita


pg-fdw-phv-handling-v1.patch
Description: application/download

-- 
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] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Robert Haas]

On Thu, Jun 16, 2016 at 10:44 PM, Etsuro Fujita
 wrote:
> On 2016/06/16 22:00, Robert Haas wrote:
>> On Thu, Jun 16, 2016 at 7:05 AM, Etsuro Fujita
>>  wrote:
>>>
>>> ISTM that a robuster solution to this is to push down the ft1-ft2-ft3
>>> join
>>> with the PHV by extending deparseExplicitTargetList() and/or something
>>> else
>>> so that we can send the remote query like:
>>>
>>> select ft1.a, (ft3.a IS NOT NULL) from (ft1 inner join ft2 on ft1.a =
>>> ft2.a)
>>> left join ft3 on ft1.a = ft3.a
>
>> I completely agree we should have that, but not for 9.6.
>
> OK, I'll work on that for 10.0.

Great, that will be a nice improvement.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Etsuro Fujita]

On 2016/06/16 22:00, Robert Haas wrote:

On Thu, Jun 16, 2016 at 7:05 AM, Etsuro Fujita
 wrote:

ISTM that a robuster solution to this is to push down the ft1-ft2-ft3 join
with the PHV by extending deparseExplicitTargetList() and/or something else
so that we can send the remote query like:

select ft1.a, (ft3.a IS NOT NULL) from (ft1 inner join ft2 on ft1.a = ft2.a)
left join ft3 on ft1.a = ft3.a



I completely agree we should have that, but not for 9.6.


OK, I'll work on that for 10.0.

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] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Robert Haas]

On Thu, Jun 16, 2016 at 7:05 AM, Etsuro Fujita
 wrote:
> ISTM that a robuster solution to this is to push down the ft1-ft2-ft3 join
> with the PHV by extending deparseExplicitTargetList() and/or something else
> so that we can send the remote query like:
>
> select ft1.a, (ft3.a IS NOT NULL) from (ft1 inner join ft2 on ft1.a = ft2.a)
> left join ft3 on ft1.a = ft3.a

I completely agree we should have that, but not for 9.6.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Etsuro Fujita]

On 2016/06/15 9:13, Amit Langote wrote:

On 2016/06/15 0:50, Robert Haas wrote:

On Tue, Jun 14, 2016 at 4:06 AM, Amit Langote wrote:

Attached new version of the patch with following changes:



OK, I committed this version with some cosmetic changes.


Thank you all for working on this!

While reviewing the patch, I noticed that the patch is still 
restrictive.  Consider:


postgres=# explain verbose select ft1.a, (ft3.a IS NOT NULL) from (ft1 
inner join ft2 on ft1.a = ft2.a) left join ft3 on ft1.a = ft3.a;

 QUERY PLAN

  Foreign Scan  (cost=100.00..103.10 rows=2 width=5)
Output: ft1.a, (ft3.a IS NOT NULL)
Relations: ((public.ft1) INNER JOIN (public.ft2)) LEFT JOIN 
(public.ft3)
Remote SQL: SELECT r1.a, r4.a FROM ((public.t1 r1 INNER JOIN 
public.t2 r2 ON (((r1.a = r2.a LEFT JOIN public.t3 r4 ON (((r1.a = 
r4.a

(4 rows)

That's great, but:

postgres=# explain verbose select * from t1 left join (select ft1.a, 
(ft3.a IS NOT NULL) from (ft1 inner join ft2 on ft1.a = ft2.a) left join 
ft3 on ft1.a = ft3.a) ss (a, b) on t1.a = ss.a;

QUERY PLAN

  Hash Right Join  (cost=202.11..204.25 rows=3 width=13)
Output: t1.a, t1.b, ft1.a, ((ft3.a IS NOT NULL))
Hash Cond: (ft1.a = t1.a)
->  Hash Left Join  (cost=201.04..203.15 rows=2 width=5)
  Output: ft1.a, (ft3.a IS NOT NULL)
  Hash Cond: (ft1.a = ft3.a)
  ->  Foreign Scan  (cost=100.00..102.09 rows=2 width=4)
Output: ft1.a
Relations: (public.ft1) INNER JOIN (public.ft2)
Remote SQL: SELECT r4.a FROM (public.t1 r4 INNER JOIN 
public.t2 r5 ON (((r4.a = r5.a

  ->  Hash  (cost=101.03..101.03 rows=1 width=4)
Output: ft3.a
->  Foreign Scan on public.ft3  (cost=100.00..101.03 
rows=1 width=4)

  Output: ft3.a
  Remote SQL: SELECT a FROM public.t3
->  Hash  (cost=1.03..1.03 rows=3 width=8)
  Output: t1.a, t1.b
  ->  Seq Scan on public.t1  (cost=0.00..1.03 rows=3 width=8)
Output: t1.a, t1.b
(19 rows)

As in the example shown upthread, we could still push down the 
ft1-ft2-ft3 join and then perform the join between the result and t1. 
However, the patch doesn't allow that, because ph_eval_at is (b 4 7) and 
relids for the ft1-ft2-ft3 join is (b 4 5 7), and so the 
bms_nonempty_difference(relids, phinfo->ph_eval_at) test returns true.


ISTM that a robuster solution to this is to push down the ft1-ft2-ft3 
join with the PHV by extending deparseExplicitTargetList() and/or 
something else so that we can send the remote query like:


select ft1.a, (ft3.a IS NOT NULL) from (ft1 inner join ft2 on ft1.a = 
ft2.a) left join ft3 on ft1.a = ft3.a


Right?

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] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Amit Langote]

On 2016/06/15 0:50, Robert Haas wrote:
> On Tue, Jun 14, 2016 at 4:06 AM, Amit Langote wrote:
>> You're right.  It indeed should be possible to push down ft1-ft2 join.
>> However it could not be done without also modifying
>> build_tlist_to_deparse() a little (as Ashutosh proposed [1] to do
>> upthread).  Attached new version of the patch with following changes:
>>
>> 1) Modified the check in foreign_join_ok() so that it is not overly
>> restrictive.  Previously, it used ph_needed as the highest level at which
>> the PHV is needed (by definition) and checked using it whether it is above
>> the join under consideration, which ended up being an overkill.  ISTM, we
>> can just decide from joinrel->relids of the join under consideration
>> whether we are above the lowest level where the PHV could be evaluated
>> (ph_eval_at) and stop if so.  So in the example you provided, it won't now
>> reject {ft1, ft2}, but will {ft4, ft1, ft2}.
>>
>> 2) Modified build_tlist_to_deparse() to get rid of the PlaceHolderVars in
>> the targetlist to deparse by using PVC_RECURSE_PLACEHOLDER flag of
>> pull_var_clause().  That is because we do not yet support anything other
>> than Vars in deparseExplicitTargetList() (+1 to your patch to change the
>> Assert to elog).
> 
> OK, I committed this version with some cosmetic changes.  I ripped out
> the header comment to foreign_join_ok(), which is just a nuisance to
> maintain.  It unnecessarily recapitulates the tests contained within
> the function, requiring us to update the comments in two places
> instead of one every time we make a change for no real gain.  And I
> rewrote the comment you added.

Thanks.

Regards,
Amit





-- 
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] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Robert Haas]

On Tue, Jun 14, 2016 at 4:06 AM, Amit Langote
 wrote:
> On 2016/06/14 6:51, Robert Haas wrote:
>> On Fri, Jun 10, 2016 at 4:14 PM, Robert Haas  wrote:
>>> Although I have done a bit of review of this patch, it needs more
>>> thought than I have so far had time to give it.  I will update again
>>> by Tuesday.
>>
>> I've reviewed this a bit further and have discovered an infelicity.
>> The following is all with the patch applied.
>>
>> By itself, this join can be pushed down:
>>
>> contrib_regression=# EXPLAIN SELECT 13 FROM ft1 RIGHT JOIN ft2 ON
>> ft1.c1 = ft2.c1;
>>   QUERY PLAN
>> --
>>  Foreign Scan  (cost=100.00..137.66 rows=822 width=4)
>>Relations: (public.ft2) LEFT JOIN (public.ft1)
>> (2 rows)
>>
>> That's great.  However, when that query is used as the outer rel of a
>> left join, it can't:
>>
>> contrib_regression=# explain verbose select * from ft4 LEFT JOIN
>> (SELECT 13 FROM ft1 RIGHT JOIN ft2 ON ft1.c1 = ft2.c1) q on true;
>>  QUERY PLAN
>> -
>>  Nested Loop Left Join  (cost=353.50..920.77 rows=41100 width=19)
>>Output: ft4.c1, ft4.c2, ft4.c3, (13)
>>->  Foreign Scan on public.ft4  (cost=100.00..102.50 rows=50 width=15)
>>  Output: ft4.c1, ft4.c2, ft4.c3
>>  Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3"
>>->  Materialize  (cost=253.50..306.57 rows=822 width=4)
>>  Output: (13)
>>  ->  Hash Left Join  (cost=253.50..302.46 rows=822 width=4)
>>Output: 13
>>Hash Cond: (ft2.c1 = ft1.c1)
>>->  Foreign Scan on public.ft2  (cost=100.00..137.66
>> rows=822 width=4)
>>  Output: ft2.c1
>>  Remote SQL: SELECT "C 1" FROM "S 1"."T 1"
>>->  Hash  (cost=141.00..141.00 rows=1000 width=4)
>>  Output: ft1.c1
>>  ->  Foreign Scan on public.ft1
>> (cost=100.00..141.00 rows=1000 width=4)
>>Output: ft1.c1
>>Remote SQL: SELECT "C 1" FROM "S 1"."T 1"
>> (18 rows)
>>
>> Of course, because of the PlaceHolderVar, there's no way to push down
>> the ft1-ft2-ft4 join to the remote side.  But we could still push down
>> the ft1-ft2 join and then locally perform the join between the result
>> and ft4.  However, the proposed fix doesn't allow that, because
>> ph_eval_at is (b 4 5) and relids for the ft1-ft2 join is also (b 4 5),
>> and so the bms_is_subset(phinfo->ph_eval_at, relids) test returns
>> true.
>
> You're right.  It indeed should be possible to push down ft1-ft2 join.
> However it could not be done without also modifying
> build_tlist_to_deparse() a little (as Ashutosh proposed [1] to do
> upthread).  Attached new version of the patch with following changes:
>
> 1) Modified the check in foreign_join_ok() so that it is not overly
> restrictive.  Previously, it used ph_needed as the highest level at which
> the PHV is needed (by definition) and checked using it whether it is above
> the join under consideration, which ended up being an overkill.  ISTM, we
> can just decide from joinrel->relids of the join under consideration
> whether we are above the lowest level where the PHV could be evaluated
> (ph_eval_at) and stop if so.  So in the example you provided, it won't now
> reject {ft1, ft2}, but will {ft4, ft1, ft2}.
>
> 2) Modified build_tlist_to_deparse() to get rid of the PlaceHolderVars in
> the targetlist to deparse by using PVC_RECURSE_PLACEHOLDER flag of
> pull_var_clause().  That is because we do not yet support anything other
> than Vars in deparseExplicitTargetList() (+1 to your patch to change the
> Assert to elog).

OK, I committed this version with some cosmetic changes.  I ripped out
the header comment to foreign_join_ok(), which is just a nuisance to
maintain.  It unnecessarily recapitulates the tests contained within
the function, requiring us to update the comments in two places
instead of one every time we make a change for no real gain.  And I
rewrote the comment you added.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Amit Langote]

On 2016/06/14 6:51, Robert Haas wrote:
> On Fri, Jun 10, 2016 at 4:14 PM, Robert Haas  wrote:
>> Although I have done a bit of review of this patch, it needs more
>> thought than I have so far had time to give it.  I will update again
>> by Tuesday.
> 
> I've reviewed this a bit further and have discovered an infelicity.
> The following is all with the patch applied.
> 
> By itself, this join can be pushed down:
> 
> contrib_regression=# EXPLAIN SELECT 13 FROM ft1 RIGHT JOIN ft2 ON
> ft1.c1 = ft2.c1;
>   QUERY PLAN
> --
>  Foreign Scan  (cost=100.00..137.66 rows=822 width=4)
>Relations: (public.ft2) LEFT JOIN (public.ft1)
> (2 rows)
> 
> That's great.  However, when that query is used as the outer rel of a
> left join, it can't:
> 
> contrib_regression=# explain verbose select * from ft4 LEFT JOIN
> (SELECT 13 FROM ft1 RIGHT JOIN ft2 ON ft1.c1 = ft2.c1) q on true;
>  QUERY PLAN
> -
>  Nested Loop Left Join  (cost=353.50..920.77 rows=41100 width=19)
>Output: ft4.c1, ft4.c2, ft4.c3, (13)
>->  Foreign Scan on public.ft4  (cost=100.00..102.50 rows=50 width=15)
>  Output: ft4.c1, ft4.c2, ft4.c3
>  Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3"
>->  Materialize  (cost=253.50..306.57 rows=822 width=4)
>  Output: (13)
>  ->  Hash Left Join  (cost=253.50..302.46 rows=822 width=4)
>Output: 13
>Hash Cond: (ft2.c1 = ft1.c1)
>->  Foreign Scan on public.ft2  (cost=100.00..137.66
> rows=822 width=4)
>  Output: ft2.c1
>  Remote SQL: SELECT "C 1" FROM "S 1"."T 1"
>->  Hash  (cost=141.00..141.00 rows=1000 width=4)
>  Output: ft1.c1
>  ->  Foreign Scan on public.ft1
> (cost=100.00..141.00 rows=1000 width=4)
>Output: ft1.c1
>Remote SQL: SELECT "C 1" FROM "S 1"."T 1"
> (18 rows)
> 
> Of course, because of the PlaceHolderVar, there's no way to push down
> the ft1-ft2-ft4 join to the remote side.  But we could still push down
> the ft1-ft2 join and then locally perform the join between the result
> and ft4.  However, the proposed fix doesn't allow that, because
> ph_eval_at is (b 4 5) and relids for the ft1-ft2 join is also (b 4 5),
> and so the bms_is_subset(phinfo->ph_eval_at, relids) test returns
> true.

You're right.  It indeed should be possible to push down ft1-ft2 join.
However it could not be done without also modifying
build_tlist_to_deparse() a little (as Ashutosh proposed [1] to do
upthread).  Attached new version of the patch with following changes:

1) Modified the check in foreign_join_ok() so that it is not overly
restrictive.  Previously, it used ph_needed as the highest level at which
the PHV is needed (by definition) and checked using it whether it is above
the join under consideration, which ended up being an overkill.  ISTM, we
can just decide from joinrel->relids of the join under consideration
whether we are above the lowest level where the PHV could be evaluated
(ph_eval_at) and stop if so.  So in the example you provided, it won't now
reject {ft1, ft2}, but will {ft4, ft1, ft2}.

2) Modified build_tlist_to_deparse() to get rid of the PlaceHolderVars in
the targetlist to deparse by using PVC_RECURSE_PLACEHOLDER flag of
pull_var_clause().  That is because we do not yet support anything other
than Vars in deparseExplicitTargetList() (+1 to your patch to change the
Assert to elog).

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CAFjFpRfQRKNCvfPj8p%3DD9%2BDVZeuTfSN3hnGowKho%3DrKCSeD9dw%40mail.gmail.com
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 7d2512c..1abff3f 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -731,7 +731,9 @@ build_tlist_to_deparse(RelOptInfo *foreignrel)
 	 * We require columns specified in foreignrel->reltarget->exprs and those
 	 * required for evaluating the local conditions.
 	 */
-	tlist = add_to_flat_tlist(tlist, foreignrel->reltarget->exprs);
+	tlist = add_to_flat_tlist(tlist,
+		pull_var_clause((Node *) foreignrel->reltarget->exprs,
+		PVC_RECURSE_PLACEHOLDERS));
 	tlist = add_to_flat_tlist(tlist,
 			  pull_var_clause((Node *) fpinfo->local_conds,
 			  PVC_RECURSE_PLACEHOLDERS));
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 1de0bc4..73900d9 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -2202,6 +2202,64 @@ SELECT t1.c1, t2.c1 FROM (ft5 t1 JOIN v_ft5 t2 ON (t1.c1 = t2.c1)) left join (ft
Remote SQL: SELECT c1 FROM "S 1"."T 4"
 (27 rows)
 
+-- non-Var items 

Re: [HACKERS] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Ashutosh Bapat]

On Tue, Jun 14, 2016 at 4:10 AM, Robert Haas  wrote:

> On Mon, Jun 13, 2016 at 5:51 PM, Robert Haas 
> wrote:
> > On Fri, Jun 10, 2016 at 4:14 PM, Robert Haas 
> wrote:
> >> Although I have done a bit of review of this patch, it needs more
> >> thought than I have so far had time to give it.  I will update again
> >> by Tuesday.
> >
> > I've reviewed this a bit further and have discovered an infelicity.
>
> Also, independent of the fix for this particular issue, I think it
> would be smart to apply the attached patch to promote the assertion
> that failed here to an elog().  If we have more bugs of this sort, now
> or in the future, I'd like to catch them even in non-assert-enabled
> builds by getting a sensible error rather than just by failing an
> assertion.  I think it's our general practice to check node types with
> elog() rather than Assert() when the nodes are coming from some
> far-distant part of the code, which is certainly the case here.
>
> I plan to commit this without delay unless there are vigorous,
> well-reasoned objections.
>

Fine with me. Serves the purpose for which I added the Assert, but in a
better manner. May be the error message can read "non-Var
nodes/targets/expressions not expected in target list". I am not sure what
do we call individual (whole) members of target list.

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

Re: [HACKERS] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Robert Haas]

On Mon, Jun 13, 2016 at 5:51 PM, Robert Haas  wrote:
> On Fri, Jun 10, 2016 at 4:14 PM, Robert Haas  wrote:
>> Although I have done a bit of review of this patch, it needs more
>> thought than I have so far had time to give it.  I will update again
>> by Tuesday.
>
> I've reviewed this a bit further and have discovered an infelicity.

Also, independent of the fix for this particular issue, I think it
would be smart to apply the attached patch to promote the assertion
that failed here to an elog().  If we have more bugs of this sort, now
or in the future, I'd like to catch them even in non-assert-enabled
builds by getting a sensible error rather than just by failing an
assertion.  I think it's our general practice to check node types with
elog() rather than Assert() when the nodes are coming from some
far-distant part of the code, which is certainly the case here.

I plan to commit this without delay unless there are vigorous,
well-reasoned objections.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 7d2512c..f38da5d 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -1112,8 +1112,10 @@ deparseExplicitTargetList(List *tlist, List **retrieved_attrs,
 		/* Extract expression if TargetEntry node */
 		Assert(IsA(tle, TargetEntry));
 		var = (Var *) tle->expr;
+
 		/* We expect only Var nodes here */
-		Assert(IsA(var, Var));
+		if (!IsA(var, Var))
+			elog(ERROR, "non-Var not expected in target list");
 
 		if (i > 0)
 			appendStringInfoString(buf, ", ");

-- 
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] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Robert Haas]

On Fri, Jun 10, 2016 at 4:14 PM, Robert Haas  wrote:
> Although I have done a bit of review of this patch, it needs more
> thought than I have so far had time to give it.  I will update again
> by Tuesday.

I've reviewed this a bit further and have discovered an infelicity.
The following is all with the patch applied.

By itself, this join can be pushed down:

contrib_regression=# EXPLAIN SELECT 13 FROM ft1 RIGHT JOIN ft2 ON
ft1.c1 = ft2.c1;
  QUERY PLAN
--
 Foreign Scan  (cost=100.00..137.66 rows=822 width=4)
   Relations: (public.ft2) LEFT JOIN (public.ft1)
(2 rows)

That's great.  However, when that query is used as the outer rel of a
left join, it can't:

contrib_regression=# explain verbose select * from ft4 LEFT JOIN
(SELECT 13 FROM ft1 RIGHT JOIN ft2 ON ft1.c1 = ft2.c1) q on true;
 QUERY PLAN
-
 Nested Loop Left Join  (cost=353.50..920.77 rows=41100 width=19)
   Output: ft4.c1, ft4.c2, ft4.c3, (13)
   ->  Foreign Scan on public.ft4  (cost=100.00..102.50 rows=50 width=15)
 Output: ft4.c1, ft4.c2, ft4.c3
 Remote SQL: SELECT c1, c2, c3 FROM "S 1"."T 3"
   ->  Materialize  (cost=253.50..306.57 rows=822 width=4)
 Output: (13)
 ->  Hash Left Join  (cost=253.50..302.46 rows=822 width=4)
   Output: 13
   Hash Cond: (ft2.c1 = ft1.c1)
   ->  Foreign Scan on public.ft2  (cost=100.00..137.66
rows=822 width=4)
 Output: ft2.c1
 Remote SQL: SELECT "C 1" FROM "S 1"."T 1"
   ->  Hash  (cost=141.00..141.00 rows=1000 width=4)
 Output: ft1.c1
 ->  Foreign Scan on public.ft1
(cost=100.00..141.00 rows=1000 width=4)
   Output: ft1.c1
   Remote SQL: SELECT "C 1" FROM "S 1"."T 1"
(18 rows)

Of course, because of the PlaceHolderVar, there's no way to push down
the ft1-ft2-ft4 join to the remote side.  But we could still push down
the ft1-ft2 join and then locally perform the join between the result
and ft4.  However, the proposed fix doesn't allow that, because
ph_eval_at is (b 4 5) and relids for the ft1-ft2 join is also (b 4 5),
and so the bms_is_subset(phinfo->ph_eval_at, relids) test returns
true.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Robert Haas]

On Wed, Jun 8, 2016 at 8:27 AM, Robert Haas  wrote:
> On Tue, Jun 7, 2016 at 10:41 PM, Noah Misch  wrote:
>> [Action required within 72 hours.  This is a generic notification.]
>>
>> The above-described topic is currently a PostgreSQL 9.6 open item.  Robert,
>> since you committed the patch believed to have created it, you own this open
>> item.  If some other commit is more relevant or if this does not belong as a
>> 9.6 open item, please let us know.  Otherwise, please observe the policy on
>> open item ownership[1] and send a status update within 72 hours of this
>> message.  Include a date for your subsequent status update.  Testers may
>> discover new open items at any time, and I want to plan to get them all fixed
>> well in advance of shipping 9.6rc1.  Consequently, I will appreciate your
>> efforts toward speedy resolution.  Thanks.
>
> Discussion of this issue is still ongoing.  Accordingly, I intend to
> wait until that discussion has concluded before proceeding further.
> I'll check this thread again no later than Friday and send an update
> by then.

Although I have done a bit of review of this patch, it needs more
thought than I have so far had time to give it.  I will update again
by Tuesday.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Amit Langote]

On 2016/06/10 2:07, Robert Haas wrote:
> On Thu, Jun 9, 2016 at 5:50 AM, Amit Langote wrote:
>> I adjusted some comments per off-list suggestion from Ashutosh.  Please
>> find attached the new version.
> 
> Are PlaceHolderVars the only problem we need to worry about here?

It seems so, as far as postgres_fdw join push-down logic is concerned.

> What about other expressions that creep into the target list during
> subquery pull-up which are not safe to push down?  See comments in
> set_append_rel_size(), recent discussion on the thread "Failed
> assertion in parallel worker (ExecInitSubPlan)", and commit
> b12fd41c695b43c76b0a9a4d19ba43b05536440c.

I went through the discussion on that thread.  I see at least some
difference between how those considerations affect parallel-safety and
postgres_fdw join push-down safety.  While parallelism is considered for
append child rels requiring guards discussed on that thread, the same does
not seem to hold for the join push-down case.  The latter would fail the
safety check much earlier on the grounds of one of the component rels not
being pushdown_safe.  That's because the failing component rel would be
append parent rel (not in its role as append child) so would not have any
foreign path to begin with.  Any hazards introduced by subquery pull-up
then become irrelevant.  That's the case when we have an inheritance tree
of foreign tables (headed by a foreign table).  The other case (union all
with subquery pull-up) does not even get that far.

So the only case this fix should account for is where we have a single
foreign table entering a potential foreign join after being pulled up from
a subquery with unsafe PHVs being created that are referenced above the
join.  If a given push-down attempt reaches as far as the check introduced
by the proposed patch, we can be sure that there are no other unsafe
expressions to worry about (accounted for by is_foreign_expr() checks up
to that point).

Am I missing something?

Thanks,
Amit




-- 
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] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Robert Haas]

On Thu, Jun 9, 2016 at 5:50 AM, Amit Langote
 wrote:
> On 2016/06/08 23:16, Amit Langote wrote:
>> On Wed, Jun 8, 2016 at 9:27 PM, Robert Haas  wrote:
>>> On Tue, Jun 7, 2016 at 10:41 PM, Noah Misch  wrote:
 [Action required within 72 hours.  This is a generic notification.]

 The above-described topic is currently a PostgreSQL 9.6 open item.  Robert,
 since you committed the patch believed to have created it, you own this 
 open
 item.  If some other commit is more relevant or if this does not belong as 
 a
 9.6 open item, please let us know.  Otherwise, please observe the policy on
 open item ownership[1] and send a status update within 72 hours of this
 message.  Include a date for your subsequent status update.  Testers may
 discover new open items at any time, and I want to plan to get them all 
 fixed
 well in advance of shipping 9.6rc1.  Consequently, I will appreciate your
 efforts toward speedy resolution.  Thanks.
>>>
>>> Discussion of this issue is still ongoing.  Accordingly, I intend to
>>> wait until that discussion has concluded before proceeding further.
>>> I'll check this thread again no later than Friday and send an update
>>> by then.
>>
>> Ashutosh seemed OK with the latest patch.
>
> I adjusted some comments per off-list suggestion from Ashutosh.  Please
> find attached the new version.

Are PlaceHolderVars the only problem we need to worry about here?
What about other expressions that creep into the target list during
subquery pull-up which are not safe to push down?  See comments in
set_append_rel_size(), recent discussion on the thread "Failed
assertion in parallel worker (ExecInitSubPlan)", and commit
b12fd41c695b43c76b0a9a4d19ba43b05536440c.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Amit Langote]

On 2016/06/08 23:16, Amit Langote wrote:
> On Wed, Jun 8, 2016 at 9:27 PM, Robert Haas  wrote:
>> On Tue, Jun 7, 2016 at 10:41 PM, Noah Misch  wrote:
>>> [Action required within 72 hours.  This is a generic notification.]
>>>
>>> The above-described topic is currently a PostgreSQL 9.6 open item.  Robert,
>>> since you committed the patch believed to have created it, you own this open
>>> item.  If some other commit is more relevant or if this does not belong as a
>>> 9.6 open item, please let us know.  Otherwise, please observe the policy on
>>> open item ownership[1] and send a status update within 72 hours of this
>>> message.  Include a date for your subsequent status update.  Testers may
>>> discover new open items at any time, and I want to plan to get them all 
>>> fixed
>>> well in advance of shipping 9.6rc1.  Consequently, I will appreciate your
>>> efforts toward speedy resolution.  Thanks.
>>
>> Discussion of this issue is still ongoing.  Accordingly, I intend to
>> wait until that discussion has concluded before proceeding further.
>> I'll check this thread again no later than Friday and send an update
>> by then.
> 
> Ashutosh seemed OK with the latest patch.

I adjusted some comments per off-list suggestion from Ashutosh.  Please
find attached the new version.

Thanks,
Amit
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 1de0bc4..0468095 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -2202,6 +2202,36 @@ SELECT t1.c1, t2.c1 FROM (ft5 t1 JOIN v_ft5 t2 ON (t1.c1 = t2.c1)) left join (ft
Remote SQL: SELECT c1 FROM "S 1"."T 4"
 (27 rows)
 
+-- query which introduces placeholders in the targetlist cannot be
+-- pushed down
+EXPLAIN (COSTS false, VERBOSE)
+SELECT q.a, ft2.c1 FROM (SELECT 13 as a FROM ft1 WHERE c1 = 13) q RIGHT JOIN ft2 ON (q.a = ft2.c1) WHERE ft2.c1 BETWEEN 10 AND 15;
+QUERY PLAN 
+---
+ Nested Loop Left Join
+   Output: (13), ft2.c1
+   Join Filter: (13 = ft2.c1)
+   ->  Foreign Scan on public.ft2
+ Output: ft2.c1
+ Remote SQL: SELECT "C 1" FROM "S 1"."T 1" WHERE (("C 1" >= 10)) AND (("C 1" <= 15)) ORDER BY "C 1" ASC NULLS LAST
+   ->  Materialize
+ Output: (13)
+ ->  Foreign Scan on public.ft1
+   Output: 13
+   Remote SQL: SELECT NULL FROM "S 1"."T 1" WHERE (("C 1" = 13))
+(11 rows)
+
+SELECT q.a, ft2.c1 FROM (SELECT 13 as a FROM ft1 WHERE c1 = 13) q RIGHT JOIN ft2 ON (q.a = ft2.c1) WHERE ft2.c1 BETWEEN 10 AND 15;
+ a  | c1 
++
+| 10
+| 11
+| 12
+ 13 | 13
+| 14
+| 15
+(6 rows)
+
 -- recreate the dropped user mapping for further tests
 CREATE USER MAPPING FOR CURRENT_USER SERVER loopback;
 DROP USER MAPPING FOR PUBLIC SERVER loopback;
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 4d17272..f010124 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -3960,6 +3960,8 @@ postgresImportForeignSchema(ImportForeignSchemaStmt *stmt, Oid serverOid)
  * 3) All join conditions are safe to push down
  * 4) No relation has local filter (this can be relaxed for INNER JOIN, if we
  *	  can move unpushable clauses upwards in the join tree).
+ * 5) There is no PlaceHolderVar originating at the joinrel that is being
+ *	  referenced elsewhere.
  */
 static bool
 foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel, JoinType jointype,
@@ -4036,6 +4038,25 @@ foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel, JoinType jointype,
 			return false;
 	}
 
+	/*
+	 * Cannot push down the join if any PlaceHolderVar in its targetlist is
+	 * needed elsewhere.  That is so because the correct result cannot be
+	 * ensured from the foreign join query with deparse code currently being
+	 * unable to generate and push down subqueries.  Remember that such a
+	 * PHV was created when some subquery on nullable side of an outer join,
+	 * with a non-nullable expression in its targetlist (wrapped in the PHV),
+	 * was pulled up during the planner prep phase.
+	 */
+	foreach(lc, root->placeholder_list)
+	{
+		PlaceHolderInfo	   *phinfo = lfirst(lc);
+		Relidsrelids = joinrel->relids;
+
+		if (bms_nonempty_difference(phinfo->ph_needed, relids) &&
+			bms_is_subset(phinfo->ph_eval_at, relids))
+			return false;
+	}
+
 	/* Save the join clauses, for later use. */
 	fpinfo->joinclauses = joinclauses;
 
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 6c2b08c..5ae3d53 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ 

Re: [HACKERS] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Amit Langote]

On Wed, Jun 8, 2016 at 9:27 PM, Robert Haas  wrote:
> On Tue, Jun 7, 2016 at 10:41 PM, Noah Misch  wrote:
>> [Action required within 72 hours.  This is a generic notification.]
>>
>> The above-described topic is currently a PostgreSQL 9.6 open item.  Robert,
>> since you committed the patch believed to have created it, you own this open
>> item.  If some other commit is more relevant or if this does not belong as a
>> 9.6 open item, please let us know.  Otherwise, please observe the policy on
>> open item ownership[1] and send a status update within 72 hours of this
>> message.  Include a date for your subsequent status update.  Testers may
>> discover new open items at any time, and I want to plan to get them all fixed
>> well in advance of shipping 9.6rc1.  Consequently, I will appreciate your
>> efforts toward speedy resolution.  Thanks.
>
> Discussion of this issue is still ongoing.  Accordingly, I intend to
> wait until that discussion has concluded before proceeding further.
> I'll check this thread again no later than Friday and send an update
> by then.

Ashutosh seemed OK with the latest patch.

Thanks,
Amit


-- 
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] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Robert Haas]

On Tue, Jun 7, 2016 at 10:41 PM, Noah Misch  wrote:
> [Action required within 72 hours.  This is a generic notification.]
>
> The above-described topic is currently a PostgreSQL 9.6 open item.  Robert,
> since you committed the patch believed to have created it, you own this open
> item.  If some other commit is more relevant or if this does not belong as a
> 9.6 open item, please let us know.  Otherwise, please observe the policy on
> open item ownership[1] and send a status update within 72 hours of this
> message.  Include a date for your subsequent status update.  Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping 9.6rc1.  Consequently, I will appreciate your
> efforts toward speedy resolution.  Thanks.

Discussion of this issue is still ongoing.  Accordingly, I intend to
wait until that discussion has concluded before proceeding further.
I'll check this thread again no later than Friday and send an update
by then.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL 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] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Ashutosh Bapat]

On Wed, Jun 8, 2016 at 12:25 PM, Amit Langote  wrote:

> On 2016/06/08 14:13, Ashutosh Bapat wrote:
> > On Tue, Jun 7, 2016 at 6:19 PM, Amit Langote wrote:
> >> On Tue, Jun 7, 2016 at 7:47 PM, Ashutosh Bapat wrote:
> >>> Looks good to me. If we add a column from the outer relation, the
> >> "NULL"ness
> >>> of inner column would be more clear. May be we should tweak the query
> to
> >>> produce few more rows, some with non-NULL columns from both the
> >> relations.
> >>> Also add a note to the comment in the test mentioning that such a join
> >> won't
> >>> be pushed down for a reader to understand the EXPLAIN output. Also, you
> >>> might want to move that test, closer to other un-pushability tests.
> >>
> >> Done in the attached.  Please check if my comment explains the reason
> >> of push-down failure correctly.
> >
> > On further thought, I think we need to restrict the join pushdown only
> for
> > outer joins. Only those joins can produce NULL rows. If we go with that
> > change, we will need my changes as well and a testcase with inner join.
>
> I think the added test in foreign_join_ok() would restrict only the outer
> joins from being pushed down (and further, only those with placeholdervars
> in their targetlist being referred to above their level).  Do you have any
> query handy as example where unintended push-down failure occurs?
>
>
Right, PHVs are created in case of outer joins. So, for an inner join the
list will not have anything in there for it.

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

Re: [HACKERS] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Amit Langote]

On 2016/06/08 14:13, Ashutosh Bapat wrote:
> On Tue, Jun 7, 2016 at 6:19 PM, Amit Langote wrote:
>> On Tue, Jun 7, 2016 at 7:47 PM, Ashutosh Bapat wrote:
>>> Looks good to me. If we add a column from the outer relation, the
>> "NULL"ness
>>> of inner column would be more clear. May be we should tweak the query to
>>> produce few more rows, some with non-NULL columns from both the
>> relations.
>>> Also add a note to the comment in the test mentioning that such a join
>> won't
>>> be pushed down for a reader to understand the EXPLAIN output. Also, you
>>> might want to move that test, closer to other un-pushability tests.
>>
>> Done in the attached.  Please check if my comment explains the reason
>> of push-down failure correctly.
>
> On further thought, I think we need to restrict the join pushdown only for
> outer joins. Only those joins can produce NULL rows. If we go with that
> change, we will need my changes as well and a testcase with inner join.

I think the added test in foreign_join_ok() would restrict only the outer
joins from being pushed down (and further, only those with placeholdervars
in their targetlist being referred to above their level).  Do you have any
query handy as example where unintended push-down failure occurs?

I tried the following example where the join {b1, b2} is pushed down
whereas {b1, b2, b3} is not, which seems reasonable because the
placeholdervar corresponding to subq.a referred to in select targetlist is
traceable to b3:

explain verbose
select b1.a, b2.a, subq.a
from fbase1 as b1 left join fbase2 b2 on (b1.a = b2.a) left join (select 1
as a from fbase3 as b3) as subq on (subq.a = b2.a);
 QUERY PLAN

-
 Nested Loop Left Join  (cost=205.69..225.41 rows=10 width=12)
   Output: b1.a, b2.a, (1)
   Join Filter: (1 = b2.a)
   ->  Foreign Scan  (cost=105.69..124.23 rows=10 width=8)
 Output: b1.a, b2.a
 Relations: (public.fbase1 b1) LEFT JOIN (public.fbase2 b2)
 Remote SQL: SELECT r1.a, r2.a FROM (public.base1 r1 LEFT JOIN
public.base2 r2 ON (((r1.a = r2.a
   ->  Materialize  (cost=100.00..101.04 rows=1 width=32)
 Output: (1)
 ->  Foreign Scan on public.fbase3 b3  (cost=100.00..101.03 rows=1
width=32)
   Output: 1
   Remote SQL: SELECT NULL FROM public.base3
(12 rows)

truncate base1;
truncate base2;
truncate base3;
insert into base1 select generate_series (1, 20);
insert into base2 select generate_series (1, 10);
insert into base3 select generate_series (1, 1);

select b1.a, b2.a, subq.a
from base1 as b1 left join base2 b2 on (b1.a = b2.a) left join (select 1
as a from base3 as b3) as subq on (subq.a = b2.a);
 a  | a  | a
++---
  1 |  1 | 1
  2 |  2 |
  3 |  3 |
  4 |  4 |
  5 |  5 |
  6 |  6 |
  7 |  7 |
  8 |  8 |
  9 |  9 |
 10 | 10 |
 10 | 10 |
 11 ||
 12 ||
 13 ||
 14 ||
 15 ||
 16 ||
 17 ||
 18 ||
 19 ||
 20 ||
(21 rows)

Missing something?

Thanks,
Amit




-- 
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] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Ashutosh Bapat]

On further thought, I think we need to restrict the join pushdown only for
outer joins. Only those joins can produce NULL rows. If we go with that
change, we will need my changes as well and a testcase with inner join.

On Tue, Jun 7, 2016 at 6:19 PM, Amit Langote 
wrote:

> On Tue, Jun 7, 2016 at 7:47 PM, Ashutosh Bapat wrote:
> > On Tue, Jun 7, 2016 at 4:07 PM, Amit Langote wrote:
> >> On 2016/06/07 19:13, Ashutosh Bapat wrote:
> >> > So, your patch looks to be the correct approach (even after we support
> >> > deparsing subqueries). Can you please include a test in regression?
> >>
> >> I added a slightly modified version of your test to the originally
> posted
> >> patch.
> >>
> > Looks good to me. If we add a column from the outer relation, the
> "NULL"ness
> > of inner column would be more clear. May be we should tweak the query to
> > produce few more rows, some with non-NULL columns from both the
> relations.
> > Also add a note to the comment in the test mentioning that such a join
> won't
> > be pushed down for a reader to understand the EXPLAIN output. Also, you
> > might want to move that test, closer to other un-pushability tests.
>
> Done in the attached.  Please check if my comment explains the reason
> of push-down failure correctly.
>
> Thanks,
> Amit
>



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

Re: [HACKERS] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Noah Misch]

On Tue, Jun 07, 2016 at 09:49:23PM +0900, Amit Langote wrote:
> On Tue, Jun 7, 2016 at 7:47 PM, Ashutosh Bapat wrote:
> > On Tue, Jun 7, 2016 at 4:07 PM, Amit Langote wrote:
> >> On 2016/06/07 19:13, Ashutosh Bapat wrote:
> >> > So, your patch looks to be the correct approach (even after we support
> >> > deparsing subqueries). Can you please include a test in regression?
> >>
> >> I added a slightly modified version of your test to the originally posted
> >> patch.
> >>
> > Looks good to me. If we add a column from the outer relation, the "NULL"ness
> > of inner column would be more clear. May be we should tweak the query to
> > produce few more rows, some with non-NULL columns from both the relations.
> > Also add a note to the comment in the test mentioning that such a join won't
> > be pushed down for a reader to understand the EXPLAIN output. Also, you
> > might want to move that test, closer to other un-pushability tests.
> 
> Done in the attached.  Please check if my comment explains the reason
> of push-down failure correctly.

[Action required within 72 hours.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 9.6 open item.  Robert,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
9.6 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within 72 hours of this
message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping 9.6rc1.  Consequently, I will appreciate your
efforts toward speedy resolution.  Thanks.

[1] 
http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.com


-- 
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] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Amit Langote]

On Tue, Jun 7, 2016 at 7:47 PM, Ashutosh Bapat wrote:
> On Tue, Jun 7, 2016 at 4:07 PM, Amit Langote wrote:
>> On 2016/06/07 19:13, Ashutosh Bapat wrote:
>> > So, your patch looks to be the correct approach (even after we support
>> > deparsing subqueries). Can you please include a test in regression?
>>
>> I added a slightly modified version of your test to the originally posted
>> patch.
>>
> Looks good to me. If we add a column from the outer relation, the "NULL"ness
> of inner column would be more clear. May be we should tweak the query to
> produce few more rows, some with non-NULL columns from both the relations.
> Also add a note to the comment in the test mentioning that such a join won't
> be pushed down for a reader to understand the EXPLAIN output. Also, you
> might want to move that test, closer to other un-pushability tests.

Done in the attached.  Please check if my comment explains the reason
of push-down failure correctly.

Thanks,
Amit


pgfdw-join-pd-bug-3.patch
Description: Binary data

-- 
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] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Ashutosh Bapat]

On Tue, Jun 7, 2016 at 4:07 PM, Amit Langote 
wrote:

> On 2016/06/07 19:13, Ashutosh Bapat wrote:
> > I thought, columns of inner relation will be set to null during
> projection
> > from ForeignScan for joins. But I was wrong. If we want to push-down
> joins
> > in this case, we have two solutions
> > 1. Build queries with subqueries at the time of deparsing. Thus a base
> > relation or join has to include placeholders while being deparsed as a
> > subquery. This means that the deparser should deparse expression
> > represented by the placeholder. This may not be possible always.
> > 2. At the time of projection from ForeignScan recognize the nullable
> > placeholders and nullify them if the corresponding relation is nullified.
> > That looks like a major surgery.
> > So, your patch looks to be the correct approach (even after we support
> > deparsing subqueries). Can you please include a test in regression?
>
> I added a slightly modified version of your test to the originally posted
> patch.
>
> Looks good to me. If we add a column from the outer relation, the
"NULL"ness of inner column would be more clear. May be we should tweak the
query to produce few more rows, some with non-NULL columns from both the
relations. Also add a note to the comment in the test mentioning that such
a join won't be pushed down for a reader to understand the EXPLAIN output.
Also, you might want to move that test, closer to other un-pushability
tests.


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

Re: [HACKERS] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Amit Langote]

On 2016/06/07 19:13, Ashutosh Bapat wrote:
> I thought, columns of inner relation will be set to null during projection
> from ForeignScan for joins. But I was wrong. If we want to push-down joins
> in this case, we have two solutions
> 1. Build queries with subqueries at the time of deparsing. Thus a base
> relation or join has to include placeholders while being deparsed as a
> subquery. This means that the deparser should deparse expression
> represented by the placeholder. This may not be possible always.
> 2. At the time of projection from ForeignScan recognize the nullable
> placeholders and nullify them if the corresponding relation is nullified.
> That looks like a major surgery.
> So, your patch looks to be the correct approach (even after we support
> deparsing subqueries). Can you please include a test in regression?

I added a slightly modified version of your test to the originally posted
patch.

Thanks,
Amit.
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 1de0bc4..2dcce36 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -2053,6 +2053,28 @@ SELECT t1."C 1" FROM "S 1"."T 1" t1, LATERAL (SELECT DISTINCT t2.c1, t3.c1 FROM
1
 (10 rows)
 
+-- query which introduces placeholders in the targetlist
+EXPLAIN (COSTS false, VERBOSE)
+SELECT q.a FROM (SELECT 3 as a FROM ft1 WHERE 10 = ft1.c1) q RIGHT JOIN ft2 ON (q.a = ft2.c1) WHERE ft2.c1 = 10;
+  QUERY PLAN  
+--
+ Nested Loop Left Join
+   Output: (3)
+   Join Filter: (3 = ft2.c1)
+   ->  Foreign Scan on public.ft2
+ Output: ft2.c1
+ Remote SQL: SELECT "C 1" FROM "S 1"."T 1" WHERE (("C 1" = 10)) ORDER BY "C 1" ASC NULLS LAST
+   ->  Foreign Scan on public.ft1
+ Output: 3
+ Remote SQL: SELECT NULL FROM "S 1"."T 1" WHERE ((10 = "C 1"))
+(9 rows)
+
+SELECT q.a FROM (SELECT 3 as a FROM ft1 WHERE 10 = ft1.c1) q RIGHT JOIN ft2 ON (q.a = ft2.c1) WHERE ft2.c1 = 10;
+ a 
+---
+  
+(1 row)
+
 -- create another user for permission, user mapping, effective user tests
 CREATE USER view_owner;
 -- grant privileges on ft4 and ft5 to view_owner
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 4d17272..ec86b9a 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -4036,6 +4036,20 @@ foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel, JoinType jointype,
 			return false;
 	}
 
+	/*
+	 * Cannot push down if any PlaceHolderVars in its result are needed above
+	 * the join.
+	 */
+	foreach(lc, root->placeholder_list)
+	{
+		PlaceHolderInfo	   *phinfo = lfirst(lc);
+		Relidsrelids = joinrel->relids;
+
+		if (bms_nonempty_difference(phinfo->ph_needed, relids) &&
+			bms_is_subset(phinfo->ph_eval_at, relids))
+			return false;
+	}
+
 	/* Save the join clauses, for later use. */
 	fpinfo->joinclauses = joinclauses;
 
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 6c2b08c..8970fd6 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -484,6 +484,10 @@ SELECT t1c1, avg(t1c1 + t2c1) FROM (SELECT t1.c1, t2.c1 FROM ft1 t1 JOIN ft2 t2
 EXPLAIN (COSTS false, VERBOSE)
 SELECT t1."C 1" FROM "S 1"."T 1" t1, LATERAL (SELECT DISTINCT t2.c1, t3.c1 FROM ft1 t2, ft2 t3 WHERE t2.c1 = t3.c1 AND t2.c2 = t1.c2) q ORDER BY t1."C 1" OFFSET 10 LIMIT 10;
 SELECT t1."C 1" FROM "S 1"."T 1" t1, LATERAL (SELECT DISTINCT t2.c1, t3.c1 FROM ft1 t2, ft2 t3 WHERE t2.c1 = t3.c1 AND t2.c2 = t1.c2) q ORDER BY t1."C 1" OFFSET 10 LIMIT 10;
+-- query which introduces placeholders in the targetlist
+EXPLAIN (COSTS false, VERBOSE)
+SELECT q.a FROM (SELECT 3 as a FROM ft1 WHERE 10 = ft1.c1) q RIGHT JOIN ft2 ON (q.a = ft2.c1) WHERE ft2.c1 = 10;
+SELECT q.a FROM (SELECT 3 as a FROM ft1 WHERE 10 = ft1.c1) q RIGHT JOIN ft2 ON (q.a = ft2.c1) WHERE ft2.c1 = 10;
 
 -- create another user for permission, user mapping, effective user tests
 CREATE USER view_owner;

-- 
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] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Ashutosh Bapat]

> That's the patch I came up with initially but it seemed to me to produce
> the wrong result.  Correct me if that is not so:
>
> CREATE EXTENSION IF NOT EXISTS postgres_fdw WITH SCHEMA public;
>
> CREATE SERVER myserver FOREIGN DATA WRAPPER postgres_fdw OPTIONS (dbname
> 'test');
>
> CREATE USER MAPPING FOR CURRENT_USER SERVER myserver;
>
> CREATE TABLE base1 (a integer);
> CREATE TABLE base2 (a integer);
>
> CREATE FOREIGN TABLE fbase1 (a integer) SERVER myserver OPTIONS
> (table_name 'base1');
>
> INSERT INTO fbase1 VALUES (1);
>
> CREATE FOREIGN TABLE fbase2 (a integer) SERVER myserver OPTIONS
> (table_name 'base2');
>
> INSERT INTO fbase2 VALUES (2);
>
>
> explain verbose select subq.a, b2.a from (select 1 as a from fbase1 as b1)
> as subq right join fbase2 as b2 on (subq.a = b2.a);
>   QUERY PLAN
>
>
> --
>  Foreign Scan  (cost=100.00..22423.12 rows=42778 width=8)
>Output: 1, b2.a
>Relations: (public.fbase2 b2) LEFT JOIN (public.fbase1 b1)
>Remote SQL: SELECT r2.a FROM (public.base2 r2 LEFT JOIN public.base1 r4
> ON (((1 = r2.a
> (4 rows)
>
> select subq.a, b2.a from (select 1 as a from fbase1 as b1) as subq right
> join fbase2 as b2 on (subq.a = b2.a);
>  a | a
> ---+---
>  1 | 2
> (1 row)
>
>
>  to crosscheck - just using the local tables
>
> explain verbose select subq.a, b2.a from (select 1 as a from base1 as b1)
> as subq right join base2 as b2 on (subq.a = b2.a);
>   QUERY PLAN
>
>
> ---
>  Nested Loop Left Join  (cost=0.00..97614.88 rows=32512 width=8)
>Output: (1), b2.a
>Join Filter: (1 = b2.a)
>->  Seq Scan on public.base2 b2  (cost=0.00..35.50 rows=2550 width=4)
>  Output: b2.a
>->  Materialize  (cost=0.00..48.25 rows=2550 width=4)
>  Output: (1)
>  ->  Seq Scan on public.base1 b1  (cost=0.00..35.50 rows=2550
> width=4)
>Output: 1
> (9 rows)
>
> select subq.a, b2.a from (select 1 as a from base1 as b1) as subq right
> join base2 as b2 on (subq.a = b2.a);
>  a | a
> ---+---
>| 2
> (1 row)
>
> I thought both queries should produce the same result (the latter).
>
> Which the non-push-down version does:
>
> explain verbose select subq.a, b2.a from (select 1 as a from fbase1 as b1)
> as subq right join fbase2 as b2 on (subq.a = b2.a);
>   QUERY PLAN
>
>
> ---
>  Nested Loop Left Join  (cost=200.00..128737.19 rows=42778 width=8)
>Output: (1), b2.a
>Join Filter: (1 = b2.a)
>->  Foreign Scan on public.fbase2 b2  (cost=100.00..197.75 rows=2925
> width=4)
>  Output: b2.a
>  Remote SQL: SELECT a FROM public.base2
>->  Materialize  (cost=100.00..212.38 rows=2925 width=4)
>  Output: (1)
>  ->  Foreign Scan on public.fbase1 b1  (cost=100.00..197.75
> rows=2925 width=4)
>Output: 1
>Remote SQL: SELECT NULL FROM public.base1
> (11 rows)
>
> select subq.a, b2.a from (select 1 as a from fbase1 as b1) as subq right
> join fbase2 as b2 on (subq.a = b2.a);
>  a | a
> ---+---
>| 2
> (1 row)
>
>
Thanks for that case.

I thought, columns of inner relation will be set to null during projection
from ForeignScan for joins. But I was wrong. If we want to push-down joins
in this case, we have two solutions
1. Build queries with subqueries at the time of deparsing. Thus a base
relation or join has to include placeholders while being deparsed as a
subquery. This means that the deparser should deparse expression
represented by the placeholder. This may not be possible always.
2. At the time of projection from ForeignScan recognize the nullable
placeholders and nullify them if the corresponding relation is nullified.
That looks like a major surgery.

So, your patch looks to be the correct approach (even after we support
deparsing subqueries). Can you please include a test in regression?

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

Re: [HACKERS] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Amit Langote]

Hi Ashutosh,

On 2016/06/07 17:02, Ashutosh Bapat wrote:
> On Tue, Jun 7, 2016 at 11:36 AM, Amit Langote wrote:
>> On 2016/06/05 23:01, Andreas Seltenreich wrote:

...

>>> --8<---cut here---start->8---
>>> create extension postgres_fdw;
>>> create server myself foreign data wrapper postgres_fdw;
>>> create schema fdw_postgres;
>>> create user mapping for public server myself options (user :'USER');
>>> import foreign schema public from server myself into fdw_postgres;
>>> select subq_0.c0 as c0 from
>>>(select 31 as c0 from fdw_postgres.a as ref_0
>>> where 93 >= ref_0.aa) as subq_0
>>>right join fdw_postgres.rtest_vview5 as ref_1
>>>on (subq_0.c0 = ref_1.a )
>>>where 92 = subq_0.c0;
>>> --8<---cut here---end--->8---
>>
> 
> The repro assumes existence of certain tables/views e.g. rtest_vview5, a in
> public schema. Their definition is not included here. Although I could
> reproduce the issue by adding a similar query in the postgres_fdw
> regression tests (see attached patch).

See below for the query I used (almost same as the regression test you added).

>> Thanks for the example.  It seems that postgres_fdw join-pushdown logic
>> (within foreign_join_ok()?) should reject a join if any PlaceHolderVars in
>> its targetlist are required above it.  Tried to do that with the attached
>> patch which trivially fixes the reported assertion failure.
>>
> 
> Although the patch fixes the issue, it's restrictive. The placeholder Vars
> can be evaluated locally after the required columns are fetched from the
> foreign server. The right fix, therefore, is to build targetlist containing
> only the Vars that belong to the foreign tables, which in this case would
> contain "nothing". Attached patch does this and fixes the issue, while
> pushing down the join. Although, I haven't tried the exact query given in
> the report. Please let me know if the patch fixes issue with that query as
> well.

That's the patch I came up with initially but it seemed to me to produce
the wrong result.  Correct me if that is not so:

CREATE EXTENSION IF NOT EXISTS postgres_fdw WITH SCHEMA public;

CREATE SERVER myserver FOREIGN DATA WRAPPER postgres_fdw OPTIONS (dbname
'test');

CREATE USER MAPPING FOR CURRENT_USER SERVER myserver;

CREATE TABLE base1 (a integer);
CREATE TABLE base2 (a integer);

CREATE FOREIGN TABLE fbase1 (a integer) SERVER myserver OPTIONS
(table_name 'base1');

INSERT INTO fbase1 VALUES (1);

CREATE FOREIGN TABLE fbase2 (a integer) SERVER myserver OPTIONS
(table_name 'base2');

INSERT INTO fbase2 VALUES (2);


explain verbose select subq.a, b2.a from (select 1 as a from fbase1 as b1)
as subq right join fbase2 as b2 on (subq.a = b2.a);
  QUERY PLAN

--
 Foreign Scan  (cost=100.00..22423.12 rows=42778 width=8)
   Output: 1, b2.a
   Relations: (public.fbase2 b2) LEFT JOIN (public.fbase1 b1)
   Remote SQL: SELECT r2.a FROM (public.base2 r2 LEFT JOIN public.base1 r4
ON (((1 = r2.a
(4 rows)

select subq.a, b2.a from (select 1 as a from fbase1 as b1) as subq right
join fbase2 as b2 on (subq.a = b2.a);
 a | a
---+---
 1 | 2
(1 row)


 to crosscheck - just using the local tables

explain verbose select subq.a, b2.a from (select 1 as a from base1 as b1)
as subq right join base2 as b2 on (subq.a = b2.a);
  QUERY PLAN

---
 Nested Loop Left Join  (cost=0.00..97614.88 rows=32512 width=8)
   Output: (1), b2.a
   Join Filter: (1 = b2.a)
   ->  Seq Scan on public.base2 b2  (cost=0.00..35.50 rows=2550 width=4)
 Output: b2.a
   ->  Materialize  (cost=0.00..48.25 rows=2550 width=4)
 Output: (1)
 ->  Seq Scan on public.base1 b1  (cost=0.00..35.50 rows=2550 width=4)
   Output: 1
(9 rows)

select subq.a, b2.a from (select 1 as a from base1 as b1) as subq right
join base2 as b2 on (subq.a = b2.a);
 a | a
---+---
   | 2
(1 row)

I thought both queries should produce the same result (the latter).

Which the non-push-down version does:

explain verbose select subq.a, b2.a from (select 1 as a from fbase1 as b1)
as subq right join fbase2 as b2 on (subq.a = b2.a);
  QUERY PLAN

---
 Nested Loop Left Join  (cost=200.00..128737.19 rows=42778 width=8)
   Output: (1), b2.a
   Join Filter: (1 = b2.a)
   ->  Foreign Scan on public.fbase2 b2  (cost=100.00..197.75 rows=2925
width=4)
 Output: b2.a
 Remote SQL: SELECT a FROM public.base2
   ->  Materialize  (cost=100.00..212.38 rows=2925 width=4)
 Output: (1)
 ->  Foreign Scan on public.fbase1 b1  (cost=100.00..197.75
rows=2925 width=4)
   Output: 1
 

Re: [HACKERS] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Ashutosh Bapat]

On Tue, Jun 7, 2016 at 11:36 AM, Amit Langote  wrote:

> On 2016/06/05 23:01, Andreas Seltenreich wrote:
> > Creating some foreign tables via postgres_fdw in the regression db of
> > master as of de33af8, sqlsmith triggers the following assertion:
> >
> > TRAP: FailedAssertion("!(const Node*)(var))->type) == T_Var))",
> File: "deparse.c", Line: 1116)
> >
> > gdb says var is holding a T_PlaceHolderVar instead.  In a build without
> > assertions, it leads to an error later:
> >
> > ERROR:  cache lookup failed for type 0
> >
> > Recipe:
> >
> > --8<---cut here---start->8---
> > create extension postgres_fdw;
> > create server myself foreign data wrapper postgres_fdw;
> > create schema fdw_postgres;
> > create user mapping for public server myself options (user :'USER');
> > import foreign schema public from server myself into fdw_postgres;
> > select subq_0.c0 as c0 from
> >(select 31 as c0 from fdw_postgres.a as ref_0
> > where 93 >= ref_0.aa) as subq_0
> >right join fdw_postgres.rtest_vview5 as ref_1
> >on (subq_0.c0 = ref_1.a )
> >where 92 = subq_0.c0;
> > --8<---cut here---end--->8---
>

The repro assumes existence of certain tables/views e.g. rtest_vview5, a in
public schema. Their definition is not included here. Although I could
reproduce the issue by adding a similar query in the postgres_fdw
regression tests (see attached patch).


>
> Thanks for the example.  It seems that postgres_fdw join-pushdown logic
> (within foreign_join_ok()?) should reject a join if any PlaceHolderVars in
> its targetlist are required above it.  Tried to do that with the attached
> patch which trivially fixes the reported assertion failure.
>

Although the patch fixes the issue, it's restrictive. The placeholder Vars
can be evaluated locally after the required columns are fetched from the
foreign server. The right fix, therefore, is to build targetlist containing
only the Vars that belong to the foreign tables, which in this case would
contain "nothing". Attached patch does this and fixes the issue, while
pushing down the join. Although, I haven't tried the exact query given in
the report. Please let me know if the patch fixes issue with that query as
well.

The query generated by sqlsmith doesn't seem to return any result since it
assigns 31 to subq_0.c0 and the WHERE clause checks 92 =subq_0.c0. Is that
expected?

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 35c27e7..f72cff6 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -724,21 +724,23 @@ deparse_type_name(Oid type_oid, int32 typemod)
 List *
 build_tlist_to_deparse(RelOptInfo *foreignrel)
 {
 	List	   *tlist = NIL;
 	PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) foreignrel->fdw_private;
 
 	/*
 	 * We require columns specified in foreignrel->reltarget->exprs and those
 	 * required for evaluating the local conditions.
 	 */
-	tlist = add_to_flat_tlist(tlist, foreignrel->reltarget->exprs);
+	tlist = add_to_flat_tlist(tlist,
+		 pull_var_clause((Node *) foreignrel->reltarget->exprs,
+		 PVC_RECURSE_PLACEHOLDERS));
 	tlist = add_to_flat_tlist(tlist,
 			  pull_var_clause((Node *) fpinfo->local_conds,
 			  PVC_RECURSE_PLACEHOLDERS));
 
 	return tlist;
 }
 
 /*
  * Deparse SELECT statement for given relation into buf.
  *
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 1de0bc4..50fb5c3 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -2046,20 +2046,37 @@ SELECT t1."C 1" FROM "S 1"."T 1" t1, LATERAL (SELECT DISTINCT t2.c1, t3.c1 FROM
1
1
1
1
1
1
1
1
 (10 rows)
 
+-- query which introduces placeholders in the targetlist
+EXPLAIN (COSTS false, VERBOSE)
+SELECT q.a FROM (SELECT 3 as a FROM ft1 WHERE 10 >= ft1.c1) q RIGHT JOIN ft2 ON (q.a = ft2.c1) WHERE 9 = q.a;
+  QUERY PLAN   
+---
+ Foreign Scan
+   Output: 3
+   Filter: (9 = 3)
+   Relations: (public.ft2) LEFT JOIN (public.ft1)
+   Remote SQL: SELECT NULL FROM ("S 1"."T 1" r2 LEFT JOIN "S 1"."T 1" r4 ON (((3 = r2."C 1")) AND ((10 >= r4."C 1"
+(5 rows)
+
+SELECT q.a FROM (SELECT 3 as a FROM ft1 WHERE 10 >= ft1.c1) q RIGHT JOIN ft2 ON (q.a = ft2.c1) WHERE 9 = q.a;
+ a 
+---
+(0 rows)
+
 -- create another user for permission, user mapping, effective user tests
 CREATE USER view_owner;
 -- grant privileges on ft4 and ft5 to view_owner
 GRANT ALL ON ft4 TO view_owner;
 GRANT ALL ON ft5 TO view_owner;
 

Re: [HACKERS] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Michael Paquier]

On Sun, Jun 5, 2016 at 11:01 PM, Andreas Seltenreich  wrote:
> Creating some foreign tables via postgres_fdw in the regression db of
> master as of de33af8, sqlsmith triggers the following assertion:
>
> TRAP: FailedAssertion("!(const Node*)(var))->type) == T_Var))", File: 
> "deparse.c", Line: 1116)
>
> gdb says var is holding a T_PlaceHolderVar instead.  In a build without
> assertions, it leads to an error later:
>
> ERROR:  cache lookup failed for type 0
>
> Recipe:
>
> --8<---cut here---start->8---
> create extension postgres_fdw;
> create server myself foreign data wrapper postgres_fdw;
> create schema fdw_postgres;
> create user mapping for public server myself options (user :'USER');
> import foreign schema public from server myself into fdw_postgres;
> select subq_0.c0 as c0 from
>(select 31 as c0 from fdw_postgres.a as ref_0
>   where 93 >= ref_0.aa) as subq_0
>right join fdw_postgres.rtest_vview5 as ref_1
>on (subq_0.c0 = ref_1.a )
>where 92 = subq_0.c0;
> --8<---cut here---end--->8---

Open item for 9.6 added.
-- 
Michael


-- 
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] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Amit Langote]

On 2016/06/05 23:01, Andreas Seltenreich wrote:
> Creating some foreign tables via postgres_fdw in the regression db of
> master as of de33af8, sqlsmith triggers the following assertion:
> 
> TRAP: FailedAssertion("!(const Node*)(var))->type) == T_Var))", File: 
> "deparse.c", Line: 1116)
> 
> gdb says var is holding a T_PlaceHolderVar instead.  In a build without
> assertions, it leads to an error later:
> 
> ERROR:  cache lookup failed for type 0
> 
> Recipe:
> 
> --8<---cut here---start->8---
> create extension postgres_fdw;
> create server myself foreign data wrapper postgres_fdw;
> create schema fdw_postgres;
> create user mapping for public server myself options (user :'USER');
> import foreign schema public from server myself into fdw_postgres;
> select subq_0.c0 as c0 from
>(select 31 as c0 from fdw_postgres.a as ref_0
> where 93 >= ref_0.aa) as subq_0
>right join fdw_postgres.rtest_vview5 as ref_1
>on (subq_0.c0 = ref_1.a )
>where 92 = subq_0.c0;
> --8<---cut here---end--->8---

Thanks for the example.  It seems that postgres_fdw join-pushdown logic
(within foreign_join_ok()?) should reject a join if any PlaceHolderVars in
its targetlist are required above it.  Tried to do that with the attached
patch which trivially fixes the reported assertion failure.

A guess from my reading of the patch's thread [1] is that to support such
a case, join deparse code should be able to construct subqueries which it
currently doesn't support.  I may be missing something though.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CAFjFpRdHgeNOhM0AB6Gxz1eVx_yOqkYwuKddZeB5vPzfBaeCnQ%40mail.gmail.com
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 4d17272..ec86b9a 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -4036,6 +4036,20 @@ foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel, JoinType jointype,
 			return false;
 	}
 
+	/*
+	 * Cannot push down if any PlaceHolderVars in its result are needed above
+	 * the join.
+	 */
+	foreach(lc, root->placeholder_list)
+	{
+		PlaceHolderInfo	   *phinfo = lfirst(lc);
+		Relidsrelids = joinrel->relids;
+
+		if (bms_nonempty_difference(phinfo->ph_needed, relids) &&
+			bms_is_subset(phinfo->ph_eval_at, relids))
+			return false;
+	}
+
 	/* Save the join clauses, for later use. */
 	fpinfo->joinclauses = joinclauses;
 

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

[HACKERS] [sqlsmith] Failed assertion in postgres_fdw/deparse.c:1116

[Andreas Seltenreich]

Creating some foreign tables via postgres_fdw in the regression db of
master as of de33af8, sqlsmith triggers the following assertion:

TRAP: FailedAssertion("!(const Node*)(var))->type) == T_Var))", File: 
"deparse.c", Line: 1116)

gdb says var is holding a T_PlaceHolderVar instead.  In a build without
assertions, it leads to an error later:

ERROR:  cache lookup failed for type 0

Recipe:

--8<---cut here---start->8---
create extension postgres_fdw;
create server myself foreign data wrapper postgres_fdw;
create schema fdw_postgres;
create user mapping for public server myself options (user :'USER');
import foreign schema public from server myself into fdw_postgres;
select subq_0.c0 as c0 from
   (select 31 as c0 from fdw_postgres.a as ref_0
  where 93 >= ref_0.aa) as subq_0
   right join fdw_postgres.rtest_vview5 as ref_1
   on (subq_0.c0 = ref_1.a )
   where 92 = subq_0.c0;
--8<---cut here---end--->8---

regards,
Andreas


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