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;