diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index c8da82217d..357b1078f8 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -483,10 +483,9 @@ static void RemoveInheritance(Relation child_rel, Relation parent_rel); static ObjectAddress ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd); static void AttachPartitionEnsureIndexes(Relation rel, Relation attachrel); -static void ValidatePartitionConstraints(List **wqueue, Relation scanrel, - List *scanrel_children, - List *partConstraint, - bool validate_default); +static void QueuePartitionConstraintValidation(List **wqueue, Relation scanrel, + List *partConstraint, + bool validate_default); static void CloneRowTriggersToPartition(Relation parent, Relation partition); static ObjectAddress ATExecDetachPartition(Relation rel, RangeVar *name); static ObjectAddress ATExecAttachPartitionIdx(List **wqueue, Relation rel, @@ -13765,29 +13764,23 @@ PartConstraintImpliedByRelConstraint(Relation scanrel, } /* - * ValidatePartitionConstraints + * QueuePartitionConstraintValidation * - * Check whether all rows in the given table obey the given partition - * constraint; if so, it can be attached as a partition.  We do this by - * scanning the table (or all of its leaf partitions) row by row, except when - * the existing constraints are sufficient to prove that the new partitioning - * constraint must already hold. + * Add an entry to wqueue to have the given partition constraint validated by + * Phase 3, for the given relation, and all its children. + * + * We first verify whether the given constraint is implied by pre-existing + * relation constraints; if it is, there's no need to scan the table to + * validate, so don't queue in that case. */ static void -ValidatePartitionConstraints(List **wqueue, Relation scanrel, - List *scanrel_children, - List *partConstraint, - bool validate_default) +QueuePartitionConstraintValidation(List **wqueue, Relation scanrel, + List *partConstraint, + bool validate_default) { - bool found_whole_row; - ListCell *lc; - - if (partConstraint == NIL) - return; - /* - * Based on the table's existing constraints, determine if we can skip - * scanning the table to validate the partition constraint. + * Based on the table's existing constraints, determine whether or not we + * may skip scanning the table. */ if (PartConstraintImpliedByRelConstraint(scanrel, partConstraint)) { @@ -13802,68 +13795,54 @@ ValidatePartitionConstraints(List **wqueue, Relation scanrel, return; } - /* Constraints proved insufficient, so we need to scan the table. */ - foreach(lc, scanrel_children) + /* + * Constraints proved insufficient. For plain relations, queue a validation + * item now; for partitioned tables, recurse to process each partition. + */ + if (scanrel->rd_rel->relkind == RELKIND_RELATION || + scanrel->rd_rel->relkind == RELKIND_FOREIGN_TABLE) { AlteredTableInfo *tab; - Oid part_relid = lfirst_oid(lc); - Relation part_rel; - List *my_partconstr = partConstraint; - /* Lock already taken */ - if (part_relid != RelationGetRelid(scanrel)) - part_rel = heap_open(part_relid, NoLock); - else - part_rel = scanrel; + /* Grab a work queue entry. */ + tab = ATGetQueueEntry(wqueue, scanrel); + Assert(tab->partition_constraint == NULL); + tab->partition_constraint = (Expr *) linitial(partConstraint); + tab->validate_default = validate_default; + } + else + { + PartitionDesc partdesc = RelationGetPartitionDesc(scanrel); + int i; - /* - * Skip if the partition is itself a partitioned table. We can only - * ever scan RELKIND_RELATION relations. - */ - if (part_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE) + for (i = 0; i < partdesc->nparts; i++) { - if (part_rel != scanrel) - heap_close(part_rel, NoLock); - continue; - } + Relation part_rel; + bool found_whole_row; + List *thisPartConstraint; + + /* + * This is the minimum lock we need to prevent concurrent data + * additions. + */ + part_rel = heap_open(partdesc->oids[i], ShareLock); - if (part_rel != scanrel) - { /* * Adjust the constraint for scanrel so that it matches this * partition's attribute numbers. */ - my_partconstr = map_partition_varattnos(my_partconstr, 1, - part_rel, scanrel, - &found_whole_row); + thisPartConstraint = + map_partition_varattnos(partConstraint, 1, + part_rel, scanrel, &found_whole_row); /* There can never be a whole-row reference here */ if (found_whole_row) - elog(ERROR, "unexpected whole-row reference found in partition key"); + elog(ERROR, "unexpected whole-row reference found in partition constraint"); - /* Can we skip scanning this part_rel? */ - if (PartConstraintImpliedByRelConstraint(part_rel, my_partconstr)) - { - if (!validate_default) - ereport(INFO, - (errmsg("partition constraint for table \"%s\" is implied by existing constraints", - RelationGetRelationName(part_rel)))); - else - ereport(INFO, - (errmsg("updated partition constraint for default partition \"%s\" is implied by existing constraints", - RelationGetRelationName(part_rel)))); - heap_close(part_rel, NoLock); - continue; - } + QueuePartitionConstraintValidation(wqueue, part_rel, + thisPartConstraint, + validate_default); + heap_close(part_rel, NoLock); /* keep lock till commit */ } - - /* Grab a work queue entry. */ - tab = ATGetQueueEntry(wqueue, part_rel); - tab->partition_constraint = (Expr *) linitial(my_partconstr); - tab->validate_default = validate_default; - - /* keep our lock until commit */ - if (part_rel != scanrel) - heap_close(part_rel, NoLock); } } @@ -13891,8 +13870,8 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) List *partBoundConstraint; /* - * We must lock the default partition, because attaching a new partition - * will change its partition constraint. + * We must lock the default partition if one exists, because attaching a + * new partition will change its partition constraint. */ defaultPartOid = get_default_oid_from_partdesc(RelationGetPartitionDesc(rel)); @@ -13957,17 +13936,17 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) * * We do that by checking if rel is a member of the list of attachrel's * partitions provided the latter is partitioned at all. We want to avoid - * having to construct this list again, so we request the strongest lock - * on all partitions. We need the strongest lock, because we may decide - * to scan them if we find out that the table being attached (or its leaf - * partitions) may contain rows that violate the partition constraint. If - * the table has a constraint that would prevent such rows, which by - * definition is present in all the partitions, we need not scan the - * table, nor its partitions. But we cannot risk a deadlock by taking a - * weaker lock now and the stronger one only when needed. + * having to construct this list again, so we request a lock on all + * partitions. We need ShareLock, preventing data changes, because we + * may decide to scan them if we find out that the table being attached (or + * its leaf partitions) may contain rows that violate the partition + * constraint. If the table has a constraint that would prevent such rows, + * which by definition is present in all the partitions, we need not scan + * the table, nor its partitions. But we cannot risk a deadlock by taking + * a weaker lock now and the stronger one only when needed. */ attachrel_children = find_all_inheritors(RelationGetRelid(attachrel), - AccessExclusiveLock, NULL); + ShareLock, NULL); if (list_member_oid(attachrel_children, RelationGetRelid(rel))) ereport(ERROR, (errcode(ERRCODE_DUPLICATE_TABLE), @@ -14086,9 +14065,7 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) /* * Run the partition quals through const-simplification similar to * check constraints. We skip canonicalize_qual, though, because - * partition quals should be in canonical form already; also, since - * the qual is in implicit-AND format, we'd have to explicitly convert - * it to explicit-AND format and back again. + * partition quals should be in canonical form already. */ partConstraint = (List *) eval_const_expressions(NULL, @@ -14109,32 +14086,28 @@ ATExecAttachPartition(List **wqueue, Relation rel, PartitionCmd *cmd) "unexpected whole-row reference found in partition key"); /* Validate partition constraints against the table being attached. */ - ValidatePartitionConstraints(wqueue, attachrel, attachrel_children, - partConstraint, false); + QueuePartitionConstraintValidation(wqueue, attachrel, partConstraint, + false); } /* - * Check whether default partition has a row that would fit the partition - * being attached. + * If we're attaching a partition other than the default partition and a + * default one exists, then that partition's partition constraint changes, + * so add an entry to the work queue to validate it, too. (We must not + * do this when the partition being attached is the default one; we + * already did it above!) */ - defaultPartOid = - get_default_oid_from_partdesc(RelationGetPartitionDesc(rel)); - if (OidIsValid(defaultPartOid)) + if (!cmd->bound->is_default && OidIsValid(defaultPartOid)) { Relation defaultrel; - List *defaultrel_children; List *defPartConstraint; - /* We already have taken a lock on default partition. */ + /* we already hold a lock on the default partition */ defaultrel = heap_open(defaultPartOid, NoLock); defPartConstraint = get_proposed_default_constraint(partBoundConstraint); - defaultrel_children = - find_all_inheritors(defaultPartOid, - AccessExclusiveLock, NULL); - ValidatePartitionConstraints(wqueue, defaultrel, - defaultrel_children, - defPartConstraint, true); + QueuePartitionConstraintValidation(wqueue, defaultrel, + defPartConstraint, true); /* keep our lock until commit. */ heap_close(defaultrel, NoLock); 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;