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

From: Ashutosh Bapat <ashutosh(dot)bapat(at)enterprisedb(dot)com>
To: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
Cc: Rajkumar Raghuwanshi <rajkumar(dot)raghuwanshi(at)enterprisedb(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Postgres_fdw join pushdown - getting server crash in left outer join of three table
Date: 2016-03-21 14:32:00
Message-ID: CAFjFpRdno6CM1MMX0T+_3L_3Tn291pYZG1bT+Fs=kJ8Bgh+sow@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
pg_ec_assert.patch text/x-diff 9.5 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message David Fetter 2016-03-21 14:35:21 Re: [COMMITTERS] pgsql: Support parallel aggregation.
Previous Message Aleksander Alekseev 2016-03-21 14:21:15 Re: Re: PROPOSAL: make PostgreSQL sanitizers-friendly (and prevent information disclosure)