Don't add a redundant constraint when detaching a partition

On ALTER TABLE .. DETACH CONCURRENTLY, we add a new table constraint
that duplicates the partition constraint.  But if the partition already
has another constraint that implies that one, then that's unnecessary.
We were already avoiding the addition of a duplicate constraint if there
was an exact 'equal' match -- this just improves the quality of the check.

Author: Justin Pryzby <pryzby@telsasoft.com>
Reviewed-by: Álvaro Herrera <alvherre@alvh.no-ip.org>
Discussion: https://postgr.es/m/20210410184226.GY6592@telsasoft.com

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 096a6f289155feb8f8061ffac35b97a7c657b06f..97cc9fd6eca390c9b57a7694227303ee100dd3bf 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -17915,47 +17915,42 @@ ATExecDetachPartitionFinalize(Relation rel, RangeVar *name)
  * DetachAddConstraintIfNeeded
  *             Subroutine for ATExecDetachPartition.  Create a constraint that
  *             takes the place of the partition constraint, but avoid creating
- *             a dupe if an equivalent constraint already exists.
+ *             a dupe if an constraint already exists which implies the needed
+ *             constraint.
  */
 static void
 DetachAddConstraintIfNeeded(List **wqueue, Relation partRel)
 {
-       AlteredTableInfo *tab;
-       Expr       *constraintExpr;
-       TupleDesc       td = RelationGetDescr(partRel);
-       Constraint *n;
+       List       *constraintExpr;
 
-       constraintExpr = make_ands_explicit(RelationGetPartitionQual(partRel));
+       constraintExpr = RelationGetPartitionQual(partRel);
+       constraintExpr = (List *) eval_const_expressions(NULL, (Node *) constraintExpr);
 
-       /* If an identical constraint exists, we don't need to create one */
-       if (td->constr && td->constr->num_check > 0)
+       /*
+        * Avoid adding a new constraint if the needed constraint is implied by an
+        * existing constraint
+        */
+       if (!PartConstraintImpliedByRelConstraint(partRel, constraintExpr))
        {
-               for (int i = 0; i < td->constr->num_check; i++)
-               {
-                       Node    *thisconstr;
-
-                       thisconstr = stringToNode(td->constr->check[i].ccbin);
-
-                       if (equal(constraintExpr, thisconstr))
-                               return;
-               }
+               AlteredTableInfo *tab;
+               Constraint *n;
+
+               tab = ATGetQueueEntry(wqueue, partRel);
+
+               /* Add constraint on partition, equivalent to the partition constraint */
+               n = makeNode(Constraint);
+               n->contype = CONSTR_CHECK;
+               n->conname = NULL;
+               n->location = -1;
+               n->is_no_inherit = false;
+               n->raw_expr = NULL;
+               n->cooked_expr = nodeToString(make_ands_explicit(constraintExpr));
+               n->initially_valid = true;
+               n->skip_validation = true;
+               /* It's a re-add, since it nominally already exists */
+               ATAddCheckConstraint(wqueue, tab, partRel, n,
+                                                        true, false, true, ShareUpdateExclusiveLock);
        }
-
-       tab = ATGetQueueEntry(wqueue, partRel);
-
-       /* Add constraint on partition, equivalent to the partition constraint */
-       n = makeNode(Constraint);
-       n->contype = CONSTR_CHECK;
-       n->conname = NULL;
-       n->location = -1;
-       n->is_no_inherit = false;
-       n->raw_expr = NULL;
-       n->cooked_expr = nodeToString(constraintExpr);
-       n->initially_valid = true;
-       n->skip_validation = true;
-       /* It's a re-add, since it nominally already exists */
-       ATAddCheckConstraint(wqueue, tab, partRel, n,
-                                                true, false, true, ShareUpdateExclusiveLock);
 }
 
 /*

diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index ec14b060a637c65c6006c11836e9c677888ecf08..f81bdf513b6bf1e134ec77dec6a79220d99ff01d 100644 (file)
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -4191,6 +4191,26 @@ ALTER TABLE range_parted2 DETACH PARTITION part_rp CONCURRENTLY;
 Partition key: RANGE (a)
 Number of partitions: 0
 
+-- constraint should be created
+\d part_rp
+              Table "public.part_rp"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+Check constraints:
+    "part_rp_a_check" CHECK (a IS NOT NULL AND a >= 0 AND a < 100)
+
+CREATE TABLE part_rp100 PARTITION OF range_parted2 (CHECK (a>=123 AND a<133 AND a IS NOT NULL)) FOR VALUES FROM (100) to (200);
+ALTER TABLE range_parted2 DETACH PARTITION part_rp100 CONCURRENTLY;
+-- redundant constraint should not be created
+\d part_rp100
+             Table "public.part_rp100"
+ Column |  Type   | Collation | Nullable | Default 
+--------+---------+-----------+----------+---------
+ a      | integer |           |          | 
+Check constraints:
+    "part_rp100_a_check" CHECK (a >= 123 AND a < 133 AND a IS NOT NULL)
+
 DROP TABLE range_parted2;
 -- Check ALTER TABLE commands for partitioned tables and partitions
 -- cannot add/drop column to/from *only* the parent

diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 7a9c9252dc0adb2a1a0312dc2f32d55ed589cd65..dc0200adcb81fe94cd7940e662247edb0f8315fd 100644 (file)
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2696,6 +2696,12 @@ DROP TABLE part_rpd;
 -- works fine
 ALTER TABLE range_parted2 DETACH PARTITION part_rp CONCURRENTLY;
 \d+ range_parted2
+-- constraint should be created
+\d part_rp
+CREATE TABLE part_rp100 PARTITION OF range_parted2 (CHECK (a>=123 AND a<133 AND a IS NOT NULL)) FOR VALUES FROM (100) to (200);
+ALTER TABLE range_parted2 DETACH PARTITION part_rp100 CONCURRENTLY;
+-- redundant constraint should not be created
+\d part_rp100
 DROP TABLE range_parted2;
 
 -- Check ALTER TABLE commands for partitioned tables and partitions