Обсуждение: Postgres_fdw join pushdown - getting server crash in left outer join of three table

On Fri, Mar 18, 2016 at 7:22 PM, Rajkumar Raghuwanshi
<rajkumar.raghuwanshi@enterprisedb.com> 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

On Mon, Mar 21, 2016 at 11:32 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> 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

On Wed, Mar 23, 2016 at 5:24 AM, Rajkumar Raghuwanshi
<rajkumar.raghuwanshi@enterprisedb.com> 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

On Thu, Mar 24, 2016 at 1:53 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Mar 23, 2016 at 5:24 AM, Rajkumar Raghuwanshi
> <rajkumar.raghuwanshi@enterprisedb.com> 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

On Wed, Mar 23, 2016 at 12:53 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Mar 23, 2016 at 5:24 AM, Rajkumar Raghuwanshi
> <rajkumar.raghuwanshi@enterprisedb.com> 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

On Fri, May 13, 2016 at 11:14 PM, Robert Haas <robertmhaas@gmail.com> 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

On Fri, May 13, 2016 at 6:40 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, May 13, 2016 at 11:14 PM, Robert Haas <robertmhaas@gmail.com> 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