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

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

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

On Sun, Jun 5, 2016 at 11:01 PM, Andreas Seltenreich <seltenreich@gmx.de> 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

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.12rows=42778 width=8)  Output: 1, b2.a  Relations: (public.fbase2 b2) LEFT JOIN (public.fbase1 b1)
RemoteSQL: 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.88rows=32512 width=8)  Output: (1), b2.a  Join Filter: (1 = b2.a)  ->  Seq Scan on public.base2 b2
(cost=0.00..35.50rows=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.19rows=42778 width=8)  Output: (1), b2.a  Join Filter: (1 = b2.a)  ->  Foreign Scan on
public.fbase2b2  (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=2925width=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)


Am I missing something?

Thanks,
Amit

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.

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

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.GA447393@tornado.leadboat.com

<div dir="ltr">On further thought, I think we need to restrict the join pushdown only for outer joins. Only those joins
canproduce NULL rows. If we go with that change, we will need my changes as well and a testcase with inner join.<br
/></div><divclass="gmail_extra"><br /><div class="gmail_quote">On Tue, Jun 7, 2016 at 6:19 PM, Amit Langote <span
dir="ltr"><<ahref="mailto:amitlangote09@gmail.com" target="_blank">amitlangote09@gmail.com</a>></span> wrote:<br
/><blockquoteclass="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Tue, Jun 7,
2016at 7:47 PM, Ashutosh Bapat wrote:<br /><span class="">> On Tue, Jun 7, 2016 at 4:07 PM, Amit Langote wrote:<br
/>>> On 2016/06/07 19:13, Ashutosh Bapat wrote:<br /></span><span class="">>> > So, your patch looks to
bethe correct approach (even after we support<br /> >> > deparsing subqueries). Can you please include a test
inregression?<br /> >><br /> >> I added a slightly modified version of your test to the originally
posted<br/> >> patch.<br /> >><br /> > Looks good to me. If we add a column from the outer relation, the
"NULL"ness<br/> > of inner column would be more clear. May be we should tweak the query to<br /> > produce few
morerows, some with non-NULL columns from both the relations.<br /> > Also add a note to the comment in the test
mentioningthat such a join won't<br /> > be pushed down for a reader to understand the EXPLAIN output. Also, you<br
/>> might want to move that test, closer to other un-pushability tests.<br /><br /></span>Done in the attached. 
Pleasecheck if my comment explains the reason<br /> of push-down failure correctly.<br /><br /> Thanks,<br /> Amit<br
/></blockquote></div><br/><br clear="all" /><br />-- <br /><div class="gmail_signature"
data-smartmail="gmail_signature"><divdir="ltr">Best Wishes,<br />Ashutosh Bapat<br />EnterpriseDB Corporation<br />The
PostgresDatabase Company<br /></div></div></div> 

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

On Tue, Jun 7, 2016 at 10:41 PM, Noah Misch <noah@leadboat.com> 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

On Wed, Jun 8, 2016 at 9:27 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Jun 7, 2016 at 10:41 PM, Noah Misch <noah@leadboat.com> 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

On 2016/06/08 23:16, Amit Langote wrote:
> On Wed, Jun 8, 2016 at 9:27 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Tue, Jun 7, 2016 at 10:41 PM, Noah Misch <noah@leadboat.com> 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

On Thu, Jun 9, 2016 at 5:50 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2016/06/08 23:16, Amit Langote wrote:
>> On Wed, Jun 8, 2016 at 9:27 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> On Tue, Jun 7, 2016 at 10:41 PM, Noah Misch <noah@leadboat.com> 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

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

On Wed, Jun 8, 2016 at 8:27 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Jun 7, 2016 at 10:41 PM, Noah Misch <noah@leadboat.com> 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

On Fri, Jun 10, 2016 at 4:14 PM, Robert Haas <robertmhaas@gmail.com> 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.77rows=41100 width=19)  Output: ft4.c1, ft4.c2, ft4.c3, (13)  ->  Foreign Scan on public.ft4
(cost=100.00..102.50rows=50 width=15)        Output: ft4.c1, ft4.c2, ft4.c3        Remote SQL: SELECT c1, c2, c3 FROM
"S1"."T 3"  ->  Materialize  (cost=253.50..306.57 rows=822 width=4)        Output: (13)        ->  Hash Left Join
(cost=253.50..302.46rows=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
Scanon 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

On Mon, Jun 13, 2016 at 5:51 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Jun 10, 2016 at 4:14 PM, Robert Haas <robertmhaas@gmail.com> 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

On 2016/06/14 6:51, Robert Haas wrote:
> On Fri, Jun 10, 2016 at 4:14 PM, Robert Haas <robertmhaas@gmail.com> 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

On Tue, Jun 14, 2016 at 4:06 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2016/06/14 6:51, Robert Haas wrote:
>> On Fri, Jun 10, 2016 at 4:14 PM, Robert Haas <robertmhaas@gmail.com> 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

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

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)
INNERJOIN (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)
HashCond: (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
INNERJOIN 
 
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=3width=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

On Thu, Jun 16, 2016 at 7:05 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> 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

On 2016/06/16 22:00, Robert Haas wrote:
> On Thu, Jun 16, 2016 at 7:05 AM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp> 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

On Thu, Jun 16, 2016 at 10:44 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:
> On 2016/06/16 22:00, Robert Haas wrote:
>> On Thu, Jun 16, 2016 at 7:05 AM, Etsuro Fujita
>> <fujita.etsuro@lab.ntt.co.jp> 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

On 2016/06/17 21:45, Robert Haas wrote:
> On Thu, Jun 16, 2016 at 10:44 PM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp> wrote:
>> On 2016/06/16 22:00, Robert Haas wrote:
>>> On Thu, Jun 16, 2016 at 7:05 AM, Etsuro Fujita
>>> <fujita.etsuro@lab.ntt.co.jp> 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