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

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]/messages/by-id/CAFjFpRdHgeNOhM0AB6Gxz1eVx_yOqkYwuKddZeB5vPzfBaeCnQ@mail.gmail.com 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]: /messages/by-id/CAFjFpRdHgeNOhM0AB6Gxz1eVx_yOqkYwuKddZeB5vPzfBaeCnQ@mail.gmail.com
/messages/by-id/CAFjFpRdHgeNOhM0AB6Gxz1eVx_yOqkYwuKddZeB5vPzfBaeCnQ@mail.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

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

On Tue, Jun 7, 2016 at 11:36 AM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp

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

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

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

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

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 4:07 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
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

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]/messages/by-id/20160527025039.GA447393@tornado.leadboat.com 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]: /messages/by-id/20160527025039.GA447393@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

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 <amitlangote09@gmail.com>
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

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

On Wed, Jun 8, 2016 at 12:25 PM, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp

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

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

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

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

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

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

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

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

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

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

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.

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

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 Tue, Jun 14, 2016 at 4:10 AM, Robert Haas <robertmhaas@gmail.com> wrote:

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.

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

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]/messages/by-id/CAFjFpRfQRKNCvfPj8p=D9+DVZeuTfSN3hnGowKho=rKCSeD9dw@mail.gmail.com 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]: /messages/by-id/CAFjFpRfQRKNCvfPj8p=D9+DVZeuTfSN3hnGowKho=rKCSeD9dw@mail.gmail.com
/messages/by-id/CAFjFpRfQRKNCvfPj8p=D9+DVZeuTfSN3hnGowKho=rKCSeD9dw@mail.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

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

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

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

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

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

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

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

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

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

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