Why do we need AccessExclusiveLock on all children of a relation that we
want to scan to search for rows not satisfying the constraint? I think
it should be enough to get ShareLock, which prevents INSERT, no? I have
a feeling I'm missing something here, but I don't know what, and all
tests pass with that change.
Also: the change proposed to remove the get_default_oid_from_partdesc()
call actually fixes the bug, but to me it was not at all obvious why.
To figure out why, I first had to realize that
ValidatePartitionConstraints was lying to me, both in name and in
comments: it doesn't actually validate anything, it merely queues
entries so that alter table's phase 3 would do the validation. I found
this extremely confusing, so I fixed the comments to match reality, and
later decided to rename the function also.
At that point I was able to understand what the problem was: when
attaching the default partition, we were setting the constraint to be
validated for that partition to the correctly computed partition
constraint; and later in the same function we would set the constraint
to "the immature constraint" because we were now seeing that the default
partition OID was not invalid. So it was rather a bug in
ValidatePartitionConstraints, in that it was accepting to set the
expression to validate when another expression had already been set! I
added an assert to protect against this. And then changed the decision
of whether or not to run this block based on the attached partition
being the default one or not; because as the "if" test was, it was just
a recipe for confusion. (Now, if you look carefully you realize that
the new test for bound->is_default I added is kinda redundant: it can
only be set if there was a default partition OID at start of the
function, and therefore we know we're not adding a default partition.
And for the case where we *are* adding the default partition, it is
still Invalid because we don't re-read its own OID. But relying on that
seems brittle because it breaks if we happen to update the default
partition OID in the middle of that function, which is what we were
doing. Hence the new is_default test.)
I looked at the implementation of ValidatePartitionConstraints and
didn't like it, so I rewrote it also.
This comment is mistaken, too:
- /*
- * Skip if the partition is itself a partitioned table. We can only
- * ever scan RELKIND_RELATION relations.
- */
... because it ignores the possibility of a partition being a foreign
table. The code seems to work, but I think there is no test to cover
the case that a foreign table containing data that doesn't satisfy the
constraint is attached, because when I set that case to return doing
nothing (ie. ATGetQueueEntry is not called for a foreign partition), no
test failed.
Generally speaking, I didn't like ATExecAttachPartition; it's doing too
much that should have been done in ATPrepAttachPartition. The only
reason that's not broken is because we don't allow ATTACH PARTITION to
appear together with other commands in alter table, which seems
disappointing ... for example, when attaching multiple tables and a
default partition exist, you have to scan the default one multiple
times, which is lame.
(Speaking of lame: I noticed that RelationGetPartitionQual obtains lock
on the parent relation ... I wonder if this can be used to cause a
deadlock during InitResultRelationInfo.)
BTW, I think this is already broken for the case where the default
partition is partitioned and you attach a partition colliding with a row
that's being concurrently inserted in a partition of the default
partition, though I didn't bother to write a test for that.
Anyway, I'm just an onlooker fixing a CommandCounterIncrement change.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
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;
