Re: [HACKERS] Postgres_fdw join pushdown - getting server crash in left outer join of three table

[Ashutosh Bapat]

On Fri, May 13, 2016 at 10:14 AM, Robert Haas  wrote:

> On Wed, Mar 23, 2016 at 12:53 PM, Robert Haas 
> wrote:
> > On Wed, Mar 23, 2016 at 5:24 AM, Rajkumar Raghuwanshi
> >  wrote:
> >> Thanks Ashutosh for the patch. I have apply and retested it, now not
> getting
> >> server crash.
> >
> > Thanks for the report and the testing.  I have committed the patch.
>
> So it's been pointed out to me off-list that I have committed a
> different patch than the one I intended to commit - that is, the one
> Michael sent, not the one Ashutosh sent.  Oops.
>
> Reverting Michael's patch and applying Ashutosh's doesn't work any
> more due to conflicts in the regression tests.  And in re-rereviewing
> Ashutosh's patch I came to feel like the comments in this area needed
> a lot more work.
>

Thanks for improving comments. Those are much better than mine.

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

Re: [HACKERS] Postgres_fdw join pushdown - getting server crash in left outer join of three table

[Rajkumar Raghuwanshi]

Thanks for the commit. I have tested it again. Not getting server crash now.

Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation

On Mon, May 16, 2016 at 9:38 PM, Robert Haas  wrote:

> On Fri, May 13, 2016 at 6:40 PM, Michael Paquier
>  wrote:
> > On Fri, May 13, 2016 at 11:14 PM, Robert Haas 
> wrote:
> >> So, barring objections, I intend to apply the attached fixup patch,
> >> which replaces Michael's logic with Ashutosh's logic and rewrites the
> >> comments such to be much more explicit.
> >
> > Re-oops. I didn't check what was committed to be honest. And it should
> > not have been my version, definitely.
>
> OK, committed.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Re: [HACKERS] Postgres_fdw join pushdown - getting server crash in left outer join of three table

[Robert Haas]

On Fri, May 13, 2016 at 6:40 PM, Michael Paquier
 wrote:
> On Fri, May 13, 2016 at 11:14 PM, Robert Haas  wrote:
>> So, barring objections, I intend to apply the attached fixup patch,
>> which replaces Michael's logic with Ashutosh's logic and rewrites the
>> comments such to be much more explicit.
>
> Re-oops. I didn't check what was committed to be honest. And it should
> not have been my version, definitely.

OK, committed.

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


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

Re: [HACKERS] Postgres_fdw join pushdown - getting server crash in left outer join of three table

[Michael Paquier]

On Fri, May 13, 2016 at 11:14 PM, Robert Haas  wrote:
> So, barring objections, I intend to apply the attached fixup patch,
> which replaces Michael's logic with Ashutosh's logic and rewrites the
> comments such to be much more explicit.

Re-oops. I didn't check what was committed to be honest. And it should
not have been my version, definitely.
-- 
Michael


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

Re: [HACKERS] Postgres_fdw join pushdown - getting server crash in left outer join of three table

[Robert Haas]

On Wed, Mar 23, 2016 at 12:53 PM, Robert Haas  wrote:
> On Wed, Mar 23, 2016 at 5:24 AM, Rajkumar Raghuwanshi
>  wrote:
>> Thanks Ashutosh for the patch. I have apply and retested it, now not getting
>> server crash.
>
> Thanks for the report and the testing.  I have committed the patch.

So it's been pointed out to me off-list that I have committed a
different patch than the one I intended to commit - that is, the one
Michael sent, not the one Ashutosh sent.  Oops.

Reverting Michael's patch and applying Ashutosh's doesn't work any
more due to conflicts in the regression tests.  And in re-rereviewing
Ashutosh's patch I came to feel like the comments in this area needed
a lot more work.

So, barring objections, I intend to apply the attached fixup patch,
which replaces Michael's logic with Ashutosh's logic and rewrites the
comments such to be much more explicit.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
From 5de87782f3fb5898edbdd870e53f08f5e83ddbe5 Mon Sep 17 00:00:00 2001
From: Robert Haas 
Date: Thu, 12 May 2016 14:46:08 -0400
Subject: [PATCH] postgres_fdw: Fix the fix for crash when pushing down
 multiple joins.

Commit 3151f16e1874db82ed85a005dac15368903ca9fb was intended to be
a commit of a patch from Ashutosh Bapat, but instead I mistakenly
committed an earlier version from Michael Paquier (because both
patches were submitted with the same filename, and I confused them).
Michael's patch fixes the crash but doesn't actually implement the
correct test.

Repair the incorrect logic, and also expand the comments considerably
so that this is all more clear.

Ashutosh Bapat and Robert Haas
---
 contrib/postgres_fdw/expected/postgres_fdw.out | 68 ++
 contrib/postgres_fdw/postgres_fdw.c| 36 --
 contrib/postgres_fdw/sql/postgres_fdw.sql  | 10 
 3 files changed, 110 insertions(+), 4 deletions(-)

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 698aa8f..1de0bc4 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -478,6 +478,74 @@ SELECT t1."C 1" FROM "S 1"."T 1" t1 left join ft1 t2 join ft2 t3 on (t2.c1 = t3.
  110
 (10 rows)
 
+-- Test similar to above, except that the full join prevents any equivalence
+-- classes from being merged. This produces single relation equivalence classes
+-- included in join restrictions.
+EXPLAIN (COSTS false, VERBOSE)
+	SELECT t1."C 1", t2.c1, t3.c1 FROM "S 1"."T 1" t1 left join ft1 t2 full join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10;
+QUERY PLAN
+--
+ Limit
+   Output: t1."C 1", t2.c1, t3.c1
+   ->  Merge Right Join
+ Output: t1."C 1", t2.c1, t3.c1
+ Merge Cond: (t3.c1 = t1."C 1")
+ ->  Foreign Scan
+   Output: t3.c1, t2.c1
+   Relations: (public.ft2 t3) LEFT JOIN (public.ft1 t2)
+   Remote SQL: SELECT r3."C 1", r2."C 1" FROM ("S 1"."T 1" r3 LEFT JOIN "S 1"."T 1" r2 ON (((r2."C 1" = r3."C 1" ORDER BY r3."C 1" ASC NULLS LAST
+ ->  Index Only Scan using t1_pkey on "S 1"."T 1" t1
+   Output: t1."C 1"
+(11 rows)
+
+SELECT t1."C 1", t2.c1, t3.c1 FROM "S 1"."T 1" t1 left join ft1 t2 full join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10;
+ C 1 | c1  | c1  
+-+-+-
+ 101 | 101 | 101
+ 102 | 102 | 102
+ 103 | 103 | 103
+ 104 | 104 | 104
+ 105 | 105 | 105
+ 106 | 106 | 106
+ 107 | 107 | 107
+ 108 | 108 | 108
+ 109 | 109 | 109
+ 110 | 110 | 110
+(10 rows)
+
+-- Test similar to above with all full outer joins
+EXPLAIN (COSTS false, VERBOSE)
+	SELECT t1."C 1", t2.c1, t3.c1 FROM "S 1"."T 1" t1 full join ft1 t2 full join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10;
+QUERY PLAN
+--
+ Limit
+   Output: t1."C 1", t2.c1, t3.c1
+   ->  Merge Full Join
+ Output: t1."C 1", t2.c1, t3.c1
+ Merge Cond: (t3.c1 = t1."C 1")
+ ->  Foreign Scan
+   Output: t2.c1, t3.c1
+   Relations: (public.ft1 t2) FULL JOIN (public.ft2 t3)
+   Remote SQL: SELECT r2."C 1", r3."C 1" FROM ("S 1"."T 1" r2 FULL JOIN "S 1"."T 1" r3 ON (((r2."C 1" = r3."C 1" ORDER BY 

Re: [HACKERS] Postgres_fdw join pushdown - getting server crash in left outer join of three table

[Ashutosh Bapat]

> > Thanks for the report and the testing.  I have committed the patch.
>
>
Thanks.


> Cool, I have refreshed the wiki page for open items accordingly.
>

Thanks.


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

Re: [HACKERS] Postgres_fdw join pushdown - getting server crash in left outer join of three table

[Michael Paquier]

On Thu, Mar 24, 2016 at 1:53 AM, Robert Haas  wrote:
> On Wed, Mar 23, 2016 at 5:24 AM, Rajkumar Raghuwanshi
>  wrote:
>> Thanks Ashutosh for the patch. I have apply and retested it, now not getting
>> server crash.
>
> Thanks for the report and the testing.  I have committed the patch.

Cool, I have refreshed the wiki page for open items accordingly.
-- 
Michael


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

Re: [HACKERS] Postgres_fdw join pushdown - getting server crash in left outer join of three table

[Robert Haas]

On Wed, Mar 23, 2016 at 5:24 AM, Rajkumar Raghuwanshi
 wrote:
> Thanks Ashutosh for the patch. I have apply and retested it, now not getting
> server crash.

Thanks for the report and the testing.  I have committed the patch.

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


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

Re: [HACKERS] Postgres_fdw join pushdown - getting server crash in left outer join of three table

[Michael Paquier]

On Mon, Mar 21, 2016 at 11:32 PM, Ashutosh Bapat
 wrote:
>> In get_useful_ecs_for_relation, it seems to me that this assertion
>> should be removed and replaces by an actual check because even if
>> right_ec and left_ec are initialized, we cannot be sure that ec_relids
>> contains the relations specified:
>> /*
>>  * restrictinfo->mergeopfamilies != NIL is sufficient to guarantee
>>  * that left_ec and right_ec will be initialized, per comments in
>>  * distribute_qual_to_rels, and rel->joininfo should only contain
>> ECs
>>  * where this relation appears on one side or the other.
>>  */
>> if (bms_is_subset(relids, restrictinfo->right_ec->ec_relids))
>> useful_eclass_list =
>> list_append_unique_ptr(useful_eclass_list,
>>
>> restrictinfo->right_ec);
>> else
>> {
>> Assert(bms_is_subset(relids,
>> restrictinfo->left_ec->ec_relids));
>> useful_eclass_list =
>> list_append_unique_ptr(useful_eclass_list,
>>
>> restrictinfo->left_ec);
>> }
>
> An EC covers all the relations covered by all the equivalence members it
> contains. In case of mergejoinable clause for outer join, EC may cover just
> a single relation whose column appears on either side of the clause. In this
> case, bms_is_subset() for a given join relation covering single relation in
> EC will be false. So, we have to use bms_overlap() instead of
> bms_is_subset(). The caller get_useful_pathkeys_for_rel() extracts the
> equivalence member (if any), which is entirely covered by the given
> relation. Otherwise, you are correct that we have to convert the assertion
> into a condition. I have added comments in get_useful_ecs_for_relation()
> explaining, why.

Ah, OK. Thanks for the explanation. This code is fixing the problem
for me as well here.
(note to self: study more the planner code).
-- 
Michael


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

Re: [HACKERS] Postgres_fdw join pushdown - getting server crash in left outer join of three table

[Rajkumar Raghuwanshi]

Thanks Ashutosh for the patch. I have apply and retested it, now not
getting server crash.

Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation

On Mon, Mar 21, 2016 at 8:02 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

> Thanks Michael for looking into this.
>
>
>
>> In get_useful_ecs_for_relation, it seems to me that this assertion
>> should be removed and replaces by an actual check because even if
>> right_ec and left_ec are initialized, we cannot be sure that ec_relids
>> contains the relations specified:
>> /*
>>  * restrictinfo->mergeopfamilies != NIL is sufficient to guarantee
>>  * that left_ec and right_ec will be initialized, per comments in
>>  * distribute_qual_to_rels, and rel->joininfo should only contain
>> ECs
>>  * where this relation appears on one side or the other.
>>  */
>> if (bms_is_subset(relids, restrictinfo->right_ec->ec_relids))
>> useful_eclass_list =
>> list_append_unique_ptr(useful_eclass_list,
>>
>>  restrictinfo->right_ec);
>> else
>> {
>> Assert(bms_is_subset(relids,
>> restrictinfo->left_ec->ec_relids));
>> useful_eclass_list =
>> list_append_unique_ptr(useful_eclass_list,
>>
>> restrictinfo->left_ec);
>> }
>>
>
> An EC covers all the relations covered by all the equivalence members it
> contains. In case of mergejoinable clause for outer join, EC may cover just
> a single relation whose column appears on either side of the clause. In
> this case, bms_is_subset() for a given join relation covering single
> relation in EC will be false. So, we have to use bms_overlap() instead of
> bms_is_subset(). The caller get_useful_pathkeys_for_rel() extracts the
> equivalence member (if any), which is entirely covered by the given
> relation. Otherwise, you are correct that we have to convert the assertion
> into a condition. I have added comments in get_useful_ecs_for_relation()
> explaining, why.
>
> See for example the attached (with more tests including combinations
>> of joins, and three-table joins). I have added an open item for 9.6 on
>> the wiki.
>>
>
> Thanks for those tests. Actually, that code is relevant for joins which
> can not be pushed down to the foreign server. For such joins we try to add
> pathkeys which will help merge joins. I have included the relevant tests
> rewriting them to use local tables, so that the entire join is not pushed
> down to the foreign server.
>
> --
> Best Wishes,
> Ashutosh Bapat
> EnterpriseDB Corporation
> The Postgres Database Company
>

Re: [HACKERS] Postgres_fdw join pushdown - getting server crash in left outer join of three table

[Ashutosh Bapat]

Thanks Michael for looking into this.



> In get_useful_ecs_for_relation, it seems to me that this assertion
> should be removed and replaces by an actual check because even if
> right_ec and left_ec are initialized, we cannot be sure that ec_relids
> contains the relations specified:
> /*
>  * restrictinfo->mergeopfamilies != NIL is sufficient to guarantee
>  * that left_ec and right_ec will be initialized, per comments in
>  * distribute_qual_to_rels, and rel->joininfo should only contain
> ECs
>  * where this relation appears on one side or the other.
>  */
> if (bms_is_subset(relids, restrictinfo->right_ec->ec_relids))
> useful_eclass_list = list_append_unique_ptr(useful_eclass_list,
>
>  restrictinfo->right_ec);
> else
> {
> Assert(bms_is_subset(relids,
> restrictinfo->left_ec->ec_relids));
> useful_eclass_list = list_append_unique_ptr(useful_eclass_list,
>
> restrictinfo->left_ec);
> }
>

An EC covers all the relations covered by all the equivalence members it
contains. In case of mergejoinable clause for outer join, EC may cover just
a single relation whose column appears on either side of the clause. In
this case, bms_is_subset() for a given join relation covering single
relation in EC will be false. So, we have to use bms_overlap() instead of
bms_is_subset(). The caller get_useful_pathkeys_for_rel() extracts the
equivalence member (if any), which is entirely covered by the given
relation. Otherwise, you are correct that we have to convert the assertion
into a condition. I have added comments in get_useful_ecs_for_relation()
explaining, why.

See for example the attached (with more tests including combinations
> of joins, and three-table joins). I have added an open item for 9.6 on
> the wiki.
>

Thanks for those tests. Actually, that code is relevant for joins which can
not be pushed down to the foreign server. For such joins we try to add
pathkeys which will help merge joins. I have included the relevant tests
rewriting them to use local tables, so that the entire join is not pushed
down to the foreign server.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index a7f32f3..d38ff86 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -471,20 +471,88 @@ SELECT t1."C 1" FROM "S 1"."T 1" t1 left join ft1 t2 join ft2 t3 on (t2.c1 = t3.
  103
  104
  105
  106
  107
  108
  109
  110
 (10 rows)
 
+-- Test similar to above, except that the full join prevents any equivalence
+-- classes from being merged. This produces single relation equivalence classes
+-- included in join restrictions.
+EXPLAIN (COSTS false, VERBOSE)
+	SELECT t1."C 1", t2.c1, t3.c1 FROM "S 1"."T 1" t1 left join ft1 t2 full join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10;
+QUERY PLAN
+--
+ Limit
+   Output: t1."C 1", t2.c1, t3.c1
+   ->  Merge Right Join
+ Output: t1."C 1", t2.c1, t3.c1
+ Merge Cond: (t3.c1 = t1."C 1")
+ ->  Foreign Scan
+   Output: t3.c1, t2.c1
+   Relations: (public.ft2 t3) LEFT JOIN (public.ft1 t2)
+   Remote SQL: SELECT r3."C 1", r2."C 1" FROM ("S 1"."T 1" r3 LEFT JOIN "S 1"."T 1" r2 ON (((r2."C 1" = r3."C 1" ORDER BY r3."C 1" ASC NULLS LAST
+ ->  Index Only Scan using t1_pkey on "S 1"."T 1" t1
+   Output: t1."C 1"
+(11 rows)
+
+SELECT t1."C 1", t2.c1, t3.c1 FROM "S 1"."T 1" t1 left join ft1 t2 full join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10;
+ C 1 | c1  | c1  
+-+-+-
+ 101 | 101 | 101
+ 102 | 102 | 102
+ 103 | 103 | 103
+ 104 | 104 | 104
+ 105 | 105 | 105
+ 106 | 106 | 106
+ 107 | 107 | 107
+ 108 | 108 | 108
+ 109 | 109 | 109
+ 110 | 110 | 110
+(10 rows)
+
+-- Test similar to above with all full outer joins
+EXPLAIN (COSTS false, VERBOSE)
+	SELECT t1."C 1", t2.c1, t3.c1 FROM "S 1"."T 1" t1 full join ft1 t2 full join ft2 t3 on (t2.c1 = t3.c1) on (t3.c1 = t1."C 1") OFFSET 100 LIMIT 10;
+QUERY PLAN
+--
+ Limit
+   Output: t1."C 1", t2.c1, t3.c1
+   ->  Merge Full Join
+ Output: t1."C 1", t2.c1, t3.c1
+ Merge Cond: 

Re: [HACKERS] Postgres_fdw join pushdown - getting server crash in left outer join of three table

[Michael Paquier]

On Fri, Mar 18, 2016 at 7:22 PM, Rajkumar Raghuwanshi
 wrote:
> Hi,
>
> I am testing postgres_fdw join pushdown feature for PostgreSQL 9.6 DB, and I
> observed below issue.
>
> Observation: If do a left outer join on foreign tables more than two times.
> It is causing the server crash.
>
> Added below statement in contrib/postgres_fdw/postgres_fdw.sql and ran make
> check, did a server crash
>
> -- left outer join three tables
> EXPLAIN (COSTS false, VERBOSE)
> SELECT t1.c1,t2.c1,t3.c1 FROM ft2 t1 LEFT JOIN ft2 t2 ON (t1.c1 = t2.c1)
> LEFT JOIN ft2 t3 ON (t2.c1 = t3.c1) OFFSET 10 LIMIT 10;
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> connection to server was lost
>
> Facing the same crash while doing left outer join, right outer join or
> combination of left-right outer joins for three tables and one local and two
> foreign tables.

In get_useful_ecs_for_relation, it seems to me that this assertion
should be removed and replaces by an actual check because even if
right_ec and left_ec are initialized, we cannot be sure that ec_relids
contains the relations specified:
/*
 * restrictinfo->mergeopfamilies != NIL is sufficient to guarantee
 * that left_ec and right_ec will be initialized, per comments in
 * distribute_qual_to_rels, and rel->joininfo should only contain ECs
 * where this relation appears on one side or the other.
 */
if (bms_is_subset(relids, restrictinfo->right_ec->ec_relids))
useful_eclass_list = list_append_unique_ptr(useful_eclass_list,
 restrictinfo->right_ec);
else
{
Assert(bms_is_subset(relids, restrictinfo->left_ec->ec_relids));
useful_eclass_list = list_append_unique_ptr(useful_eclass_list,
  restrictinfo->left_ec);
}
See for example the attached (with more tests including combinations
of joins, and three-table joins). I have added an open item for 9.6 on
the wiki.
-- 
Michael


postgres_fdw_join_fix-v1.patch
Description: binary/octet-stream

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