[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