Thanks Jeevan for reviewing.
On 2018/04/02 13:10, Jeevan Ladhe wrote:
> Hi,
>
> 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.
>>
>
> 1. A minor typo:
>
> +-- check that violating rows are correctly reported when attching as the
> s/attching/attaching
Oops, fixed.
> 2. I think following part of the test is already covered:
>
> +-- 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;
>
> IIUC, the test in create_table covers the same scenario as of above:
>
> -- check default partition overlap
> INSERT INTO list_parted2 VALUES('X');
> CREATE TABLE fail_part PARTITION OF list_parted2 FOR VALUES IN ('W', 'X',
> 'Y');
> ERROR: updated partition constraint for default partition
> "list_parted2_def" would be violated by some row
Sorry, didn't realize that it was already covered in create_tabel.sql.
Removed this one.
Attached updated patch. Adding this to the v11 open items list.
Thanks,
Amit
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index c8da82217d..9d474ad5e2 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..4712fab540 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -3891,3 +3891,19 @@ 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 attaching 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
+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..c557b050af 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2565,3 +2565,21 @@ ANALYZE attmp;
DROP TABLE attmp;
DROP USER regress_alter_table_user1;
+
+-- check that violating rows are correctly reported when attaching 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;
+
+drop table defpart_attach_test;
