Re: multi-column range partition constraint

On 2017/05/03 6:30, Robert Haas wrote:
> On Tue, May 2, 2017 at 2:51 AM, Amit Langote
> <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>> Per an off-list report from Olaf Gawenda (thanks Olaf), it seems that the
>> range partition's constraint is sometimes incorrect, at least in the case
>> of multi-column range partitioning. See below:
>>
>> create table p (a int, b int) partition by range (a, b);
>> create table p1 partition of p for values from (1, 1) to (10 ,10);
>> create table p2 partition of p for values from (11, 1) to (20, 10);
>>
>> Perhaps unusual, but it's still a valid definition. Tuple-routing puts
>> rows where they belong correctly.
>>
>> -- ok
>> insert into p values (10, 9);
>> select tableoid::regclass, * from p;
>> tableoid | a | b
>> ----------+----+---
>> p1 | 10 | 9
>> (1 row)
>>
>> -- but see this
>> select tableoid::regclass, * from p where a = 10;
>> tableoid | a | b
>> ----------+---+---
>> (0 rows)
>>
>> explain select tableoid::regclass, * from p where a = 10;
>> QUERY PLAN
>> -------------------------------------------
>> Result (cost=0.00..0.00 rows=0 width=12)
>> One-Time Filter: false
>> (2 rows)
>>
>> -- or this
>> insert into p1 values (10, 9);
>> ERROR: new row for relation "p1" violates partition constraint
>> DETAIL: Failing row contains (10, 9).
>>
>> This is because of the constraint being generated is not correct in this
>> case. p1's constraint is currently:
>>
>> a >= 1 and a < 10
>>
>> where it should really be the following:
>>
>> (a > 1 OR (a = 1 AND b >= 1))
>> AND
>> (a < 10 OR (a = 10 AND b < 10))
>>
>> Attached patch rewrites get_qual_for_range() for the same, along with some
>> code rearrangement for reuse. I also added some new tests to insert.sql
>> and inherit.sql, but wondered (maybe, too late now) whether there should
>> really be a declarative_partition.sql for these, moving in some of the old
>> tests too.
>>
>> Adding to the open items list.
>
> I think there are more problems here. With the patch:
>
> rhaas=# create table p (a int, b int) partition by range (a, b);
> CREATE TABLE
> rhaas=# create table p1 partition of p for values from (unbounded,0)
> to (unbounded,1);
> CREATE TABLE
> rhaas=# insert into p1 values (-2,-2);
> ERROR: new row for relation "p1" violates partition constraint
> DETAIL: Failing row contains (-2, -2).
> rhaas=# insert into p1 values (2,2);
> ERROR: new row for relation "p1" violates partition constraint
> DETAIL: Failing row contains (2, 2).
>
> Really, the whole CREATE TABLE .. PARTITION statement is meaningless
> and should be disallowed, because it's not meaningful to have a
> partition bound specification with a non-unbounded value following an
> unbounded value.

Yes, disallowing this in the first place is the best thing to do.
Attached patch 0001 implements that. With the patch:

create table p1 partition of p for values from (unbounded,0) to (unbounded,1);
ERROR: cannot specify finite value after UNBOUNDED
LINE 1: ...able p1 partition of p for values from (unbounded,0) to (unb...
^
> BTW, I think we should also add a function that prints the partition
> constraint, and have psql display that in the \d+ output, because
> people might need that - e.g. if you want to attach a partition
> without having to validate it, you need to be able to apply an
> appropriate constraint to it in advance, so you'll want to see what
> the existing partition constraints look like.

Agree that that would be helpful, so done in the attached 0003. With the
patch:

create table p (a int, b int) partition by range (a, b);
create table p1 partition of p for values from (1, 1) to (10 ,10);
create table p2 partition of p for values from (11, 1) to (20, 10);
\d p2
Table "public.p2"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
a | integer | | not null |
b | integer | | not null |
Partition of: p FOR VALUES FROM (11, 1) TO (20, 10)
Partition constraint: CHECK ((((a > 11) OR ((a = 11) AND (b >= 1))) AND
((a < 20) OR ((a = 20) AND (b < 10)))))

create table p3 (like p);
alter table p3 add constraint partcheck check ((((a > 21) OR ((a = 21) AND
(b >= 1))) AND ((a < 30) OR ((a = 30) AND (b < 5)))));

alter table p attach partition p3 for values from (21, 1) to (30, 10);
INFO: partition constraint for table "p3" is implied by existing constraints
ALTER TABLE

BTW, is it strange that the newly added pg_get_partition_constraintdef()
requires the relcache entry to be created for the partition and all of its
ancestor relations up to the root (I mean the fact that the relcache entry
needs to be created at all)? I can see only one other function,
set_relation_column_names(), creating the relcache entry at all.

> While I'm glad we have partitioning has a feature, I'm starting to get
> a bit depressed by the number of bugs that are turning up here. This
> was committed in early December, and ideally ought to have been stable
> long before now.

I would think so too. This bug was particularly unsettling for me,
although it is not to say that other bugs that turned up were any less
serious.

> Since Amit is back from vacation May 8th, I'll update no later than May 9th.

Attached updated 0002 and 0001, 0003 as mentioned above. Thanks a lot for
your patience.

Regards,
Amit