Horton

Thread

Commits

  1. Fix possible crash in partition-wise join.

  1. Crash in partition-wise join involving dummy partitioned relation

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2018-02-05T09:46:57Z

    Hi,
I noticed a crash in partition-wise involving dummy partitioned
tables. Here's simple testcase

CREATE TABLE prt1 (a int, b int, c varchar) PARTITION BY RANGE(a);
CREATE TABLE prt1_p1 PARTITION OF prt1 FOR VALUES FROM (0) TO (250);
CREATE TABLE prt1_p3 PARTITION OF prt1 FOR VALUES FROM (500) TO (600);
CREATE TABLE prt1_p2 PARTITION OF prt1 FOR VALUES FROM (250) TO (500);
INSERT INTO prt1 SELECT i, i % 25, to_char(i, 'FM0000') FROM
generate_series(0, 599) i WHERE i % 2 = 0;
ANALYZE prt1;

CREATE TABLE prt2 (a int, b int, c varchar) PARTITION BY RANGE(b);
CREATE TABLE prt2_p1 PARTITION OF prt2 FOR VALUES FROM (0) TO (250);
CREATE TABLE prt2_p2 PARTITION OF prt2 FOR VALUES FROM (250) TO (500);
CREATE TABLE prt2_p3 PARTITION OF prt2 FOR VALUES FROM (500) TO (600);
INSERT INTO prt2 SELECT i % 25, i, to_char(i, 'FM0000') FROM
generate_series(0, 599) i WHERE i % 3 = 0;
ANALYZE prt2;

SET enable_partition_wise_join TO true;

EXPLAIN (COSTS OFF)
SELECT t1.a, t1.c, t2.b, t2.c FROM (SELECT * FROM prt1 WHERE a = 1 AND
a = 2) t1 RIGHT JOIN prt2 t2 ON t1.a = t2.b, prt1 t3 WHERE t2.b =
t3.a;

t1 is an empty partitioned relation, with partition scheme matching
that of t2. Thus build_joinrel_partition_info() deems t1 RIGHT JOIN t2
as partitioned and sets part_scheme, nparts and other partition
properties except part_rels. Later in try_partition_wise_join(), the
function bails out since t1 is dummy because of following code

    /*
     * set_rel_pathlist() may not create paths in children of an empty
     * partitioned table and so we can not add paths to child-joins. So, deem
     * such a join as unpartitioned. When a partitioned relation is deemed
     * empty because all its children are empty, dummy path will be set in
     * each of the children.  In such a case we could still consider the join
     * as partitioned, but it might not help much.
     */
    if (IS_DUMMY_REL(rel1) || IS_DUMMY_REL(rel2))
        return;

So, part_rels is never set for relation t1 LEFT JOIN t2. When
build_joinrel_partition_info() processes (t1 LEFT JOIN t2, t3), it
expects part_rels to be set for (t1 LEFT JOIN t2) since it's deemed to
be partitioned and following assertion fails

    Assert(REL_HAS_ALL_PART_PROPS(outer_rel) &&
           REL_HAS_ALL_PART_PROPS(inner_rel));

When I wrote this code, I thought that some join order of an N-way
join involving a dummy relation would have both the joining relations
partitioned with part_rels set i.e. child-join created. But that was a
wrong assumption. Any two-way join involving a dummy relation can not
have child-joins and hence can not be deemed as partitioned. For a 3
way join involving dummy relation, every two-way join involving that
dummy relation won't have child-joins and hence the 3 way join can not
have child-join. Similarly we can use induction to prove that any
N-way join involving a dummy relation will not have child-joins and
hence won't be partitioned. We can detect this case during
build_joinrel_partition_info(). One of the joining relations presented
to that function will involve the dummy relation and would have been
deemed as unpartitioned when it was processed. We don't need any dummy
relation handling in try_partition_wise_join().

Here's patch taking that approach.

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

  2. Re: Crash in partition-wise join involving dummy partitioned relation

    Robert Haas <robertmhaas@gmail.com> — 2018-02-05T22:34:12Z

    On Mon, Feb 5, 2018 at 4:46 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> Here's patch taking that approach.

I rewrote the comment in relation.h like this, which I think is more clear:

 /*
  * Is given relation partitioned?
  *
- * A join between two partitioned relations with same partitioning scheme
- * without any matching partitions will not have any partition in it but will
- * have partition scheme set. So a relation is deemed to be partitioned if it
- * has a partitioning scheme, bounds and positive number of partitions.
+ * It's not enough to test whether rel->part_scheme is set, because it might
+ * be that the basic partitioning properties of the input relations matched
+ * but the partition bounds did not.
+ *
+ * We treat dummy relations as unpartitioned.  We could alternatively
+ * treat them as partitioned, but it's not clear whether that's a useful thing
+ * to do.
  */

With that change, committed.

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

  3. Re: Crash in partition-wise join involving dummy partitioned relation

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2018-02-06T04:02:10Z

    On Tue, Feb 6, 2018 at 4:04 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Feb 5, 2018 at 4:46 AM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>> Here's patch taking that approach.
>
> I rewrote the comment in relation.h like this, which I think is more clear:
>
>  /*
>   * Is given relation partitioned?
>   *
> - * A join between two partitioned relations with same partitioning scheme
> - * without any matching partitions will not have any partition in it but will
> - * have partition scheme set. So a relation is deemed to be partitioned if it
> - * has a partitioning scheme, bounds and positive number of partitions.
> + * It's not enough to test whether rel->part_scheme is set, because it might
> + * be that the basic partitioning properties of the input relations matched
> + * but the partition bounds did not.
> + *
> + * We treat dummy relations as unpartitioned.  We could alternatively
> + * treat them as partitioned, but it's not clear whether that's a useful thing
> + * to do.
>   */

The comment says why it checks both bounds and part_scheme, but it
doesn't explain why we check nparts, part_rels etc. My patch had that
explanation. Or may be with these changes those checks are not needed.
Should we remove those?

Thanks for the commit.

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

  4. Re: Crash in partition-wise join involving dummy partitioned relation

    Robert Haas <robertmhaas@gmail.com> — 2018-02-06T15:42:00Z

    On Mon, Feb 5, 2018 at 11:02 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> The comment says why it checks both bounds and part_scheme, but it
> doesn't explain why we check nparts, part_rels etc. My patch had that
> explanation.

Hmm, well, I couldn't understand it from your comment.  I'm fine with
adding more explanation, but it needs to be brief yet clear.

> Or may be with these changes those checks are not needed.
> Should we remove those?

I think you had them there originally so that you could Assert() on
them, but I do suspect that they're not all needed at runtime.

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