Re: Adding support for Default partition in partitioning

On Tue, May 30, 2017 at 1:08 PM, Jeevan Ladhe
<jeevan(dot)ladhe(at)enterprisedb(dot)com> wrote:
> Hi,
>
> I have rebased the patch on the latest commit.
> PFA.
>

Thanks for rebasing the patch. Here are some review comments.
+ /*
+ * In case of default partition, just note the index, we do not
+ * add this to non_null_values list.
+ */
We may want to rephrase it like
"Note the index of the partition bound spec for the default partition. There's
no datum to add to the list of non-null datums for this partition."

/* Assign mapping index for default partition. */
"mapping index" should be "mapped index". May be we want to use "the" before
default partition everywhere, there's only one specific default partition.

Assert(default_index >= 0 &&
mapping[default_index] == -1);
Needs some explanation for asserting mapping[default_index] == -1. Since
default partition accepts any non-specified value, it should not get a mapped
index while assigning those for non-null datums.

+ * Currently range partition do not have default partition
May be rephrased as "As of now, we do not support default range partition."

+ * ArrayExpr, which would return an negated expression for default
a negated instead of an negated.

+ cur_index = -1;
/*
- * A null partition key is only acceptable if null-accepting list
- * partition exists.
+ * A null partition key is acceptable if null-accepting list partition
+ * or a default partition exists. Check if there exists a null
+ * accepting partition, else this will be handled later by default
+ * partition if it exists.
*/
- cur_index = -1;
Why do we need to move assignment to cur_index before the comment.
The comment should probably change to "Handle NULL partition key here
if there's a
null-accepting list partition. Else it will routed to a default partition if
one exists."

+-- attaching default partition overlaps if a default partition already exists
+ERROR: partition "part_def2" would overlap partition "part_def1"
Saying a default partition overlaps is misleading here. A default partition is
not exepected to overlap with anything. It's expected to "adjust" with the rest
of the partitions. It can "conflict" with another default partition. So the
right error message here is "a default partition "part_def1" already exists."

+CREATE TABLE part_def1 PARTITION OF list_parted DEFAULT;
+CREATE TABLE part_def2 (LIKE part_1 INCLUDING CONSTRAINTS);
+ALTER TABLE list_parted ATTACH PARTITION part_def2 DEFAULT;
May be you want to name part_def1 as def_part and part_def2 as fail_def_part to
be consistent with other names in the file. May be you want to test to
consecutive CREATE TABLE ... DEFAULT.

+ALTER TABLE list_parted2 ATTACH PARTITION part_3 FOR VALUES IN (11);
+ERROR: new default partition constraint is violated by some row
+DETAIL: Violating row contains (11, z).
The error message seems to be misleading. The default partition is not new. May
be we should say, "default partition contains rows that conflict with the
partition bounds of "part_3"". I think we should use a better word instead of
"conflict", but I am not able to find one right now.

+-- check that leaf partitons of default partition are scanned when
s/partitons/partitions/

-ALTER TABLE part_5 ADD CONSTRAINT check_a CHECK (a IN (5)), ALTER a
SET NOT NULL;
-ALTER TABLE list_parted2 ATTACH PARTITION part_5 FOR VALUES IN (5);
+ALTER TABLE part_5 ADD CONSTRAINT check_a CHECK (a IN (5, 55)), ALTER
a SET NOT NULL;
+ALTER TABLE list_parted2 ATTACH PARTITION part_5 FOR VALUES IN (5, 55);
Why do we want to change partition bounds of this one? The test is for children
of part_5 right?

+drop table part_default;
I think this is premature drop. Down the file there's a SELECT from
list_parted, which won't list the rows inserted to the default partition and we
will miss to check whether the tuples were routed to the right partition or
not.

+update list_part1 set a = 'c' where a = 'a';
+ERROR: new row for relation "list_part1" violates partition constraint
+DETAIL: Failing row contains (c, 1).
Why do we need this test here? It's not dealing with the default partition and
partition row movement is not in there. So the updated row may not move to the
default partition, even if it's there.

This isn't a complete review. I will continue to review this patch further.

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