On 2018/03/30 17:31, Kyotaro HORIGUCHI wrote:
> At Thu, 29 Mar 2018 13:04:06 -0300, Alvaro Herrera wrote:
>> Rushabh Lathia wrote:
>>> On Thu, Mar 29, 2018 at 7:46 PM, Alvaro Herrera <alvhe...@alvh.no-ip.org>
>>> wrote:
>>
>>>> Hmm, offhand I don't quite see why this error fails to be thrown.
>>>
>>> ATTACH PARTITION should throw an error, because partition table "foo"
>>> already have two partition with key values (1, 2,3 4). And table "foo_d"
>>> which we are attaching here has :
>>
>> Oh, I understand how it's supposed to work. I was just saying I don't
>> understand how this bug occurs. Is it because we fail to determine the
>> correct partition constraint for the default partition in time for its
>> verification scan?
>
> The reason is that CommandCounterIncrement added in
> StorePartitionBound reveals the just added default partition to
> get_default_oid_from_partdesc too early. The revealed partition
> has immature constraint and it overrites the right constraint
> generated just above.
>
> ATExecAttachPartition checks for default partition oid twice but
> the second is just needless before the commit and harms after it.
Yes. What happens as of the commit mentioned in $subject is that the
partition constraint that's set as tab->partition_constraint during the
first call to ValidatePartitionConstraints (which is the correct one) is
overwritten by a wrong one during the 2nd call, which wouldn't happen
before the commit. In the wrongly occurring 2nd call, we'd end up setting
tab->partition_constraint to the negation of the clause expression that
would've been set by the first call (in this case). Thus
tab->partition_constraint ends up returning true for all the values it
contains.
I noticed that there were no tests covering this case causing 4dba331cb3
to not notice this failure in the first place. I updated your patch to
add a few tests. Also, I revised the comment changed by your patch a bit.
Thanks,
Amit
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 83a881eff3..8bba9a2e20 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -14114,11 +14114,9 @@ ATExecAttachPartition(List **wqueue, Relation rel,
PartitionCmd *cmd)
}
/*
- * Check whether default partition has a row that would fit the
partition
- * being attached.
+ * Check if the default partition contains a row that would belong in
the
+ * partition being attached.
*/
- defaultPartOid =
- get_default_oid_from_partdesc(RelationGetPartitionDesc(rel));
if (OidIsValid(defaultPartOid))
{
Relation defaultrel;
diff --git a/src/test/regress/expected/alter_table.out
b/src/test/regress/expected/alter_table.out
index a80d16a394..82b195e034 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3891,3 +3891,24 @@ ALTER TABLE attmp ALTER COLUMN i RESET
(n_distinct_inherited);
ANALYZE attmp;
DROP TABLE attmp;
DROP USER regress_alter_table_user1;
+-- check that violating rows are correctly reported when attching as the
+-- default partition
+create table defpart_attach_test (a int) partition by list (a);
+create table defpart_attach_test1 partition of defpart_attach_test for values
in (1);
+create table defpart_attach_test_d (like defpart_attach_test);
+insert into defpart_attach_test_d values (1), (2);
+-- error because its constraint as the default partition would be violated
+-- by the row containing 1
+alter table defpart_attach_test attach partition defpart_attach_test_d default;
+ERROR: partition constraint is violated by some row
+delete from defpart_attach_test_d where a = 1;
+alter table defpart_attach_test_d add check (a > 1);
+-- should be attached successfully and without needing to be scanned
+alter table defpart_attach_test attach partition defpart_attach_test_d default;
+INFO: partition constraint for table "defpart_attach_test_d" is implied by
existing constraints
+-- trying to add a partition for 2 should fail because the default
+-- partition contains a row that would violate its new constraint which
+-- prevents rows containing 2
+create table defpart_attach_test2 partition of defpart_attach_test for values
in (2);
+ERROR: updated partition constraint for default partition
"defpart_attach_test_d" would be violated by some row
+drop table defpart_attach_test;
diff --git a/src/test/regress/sql/alter_table.sql
b/src/test/regress/sql/alter_table.sql
index 8198d1e930..e0f4009b19 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2565,3 +2565,26 @@ ANALYZE attmp;
DROP TABLE attmp;
DROP USER regress_alter_table_user1;
+
+-- check that violating rows are correctly reported when attching as the
+-- default partition
+create table defpart_attach_test (a int) partition by list (a);
+create table defpart_attach_test1 partition of defpart_attach_test for values
in (1);
+create table defpart_attach_test_d (like defpart_attach_test);
+insert into defpart_attach_test_d values (1), (2);
+
+-- error because its constraint as the default partition would be violated
+-- by the row containing 1
+alter table defpart_attach_test attach partition defpart_attach_test_d default;
+delete from defpart_attach_test_d where a = 1;
+alter table defpart_attach_test_d add check (a > 1);
+
+-- should be attached successfully and without needing to be scanned
+alter table defpart_attach_test attach partition defpart_attach_test_d default;
+
+-- trying to add a partition for 2 should fail because the default
+-- partition contains a row that would violate its new constraint which
+-- prevents rows containing 2
+create table defpart_attach_test2 partition of defpart_attach_test for values
in (2);
+
+drop table defpart_attach_test;
