On Sun, Mar 21, 2021 at 03:01:15PM -0300, Alvaro Herrera wrote:
> > But note that it doesn't check if an existing constraint "implies" the new
> > constraint - maybe it should.
>
> Hm, I'm not sure I want to do that, because that means that if I later
> have to attach the partition again with the same partition bounds, then
> I might have to incur a scan to recheck the constraint. I think we want
> to make the new constraint be as tight as possible ...
If it *implies* the partition constraint, then it's at least as tight (and
maybe tighter), yes ?
I think you're concerned with the case that someone has a partition with
"tight" bounds like (a>=200 and a<300) and a check constraint that's "less
tight" like (a>=100 AND a<400). In that case, the loose check constraint
doesn't imply the tighter partition constraint, so your patch would add a
non-redundant constraint.
I'm interested in the case that someone has a check constraint that almost but
not exactly matches the partition constraint, like (a<300 AND a>=200). In that
case, your patch adds a redundant constraint.
I wrote a patch which seems to effect my preferred behavior - please check.
--
Justin
>From 1ecc8cb17192d13f7432fe582a43ab8597b15c00 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Thu, 25 Mar 2021 21:24:10 -0500
Subject: [PATCH] DETACH CONCURRENTLY: avoid creation of redundant constraint
---
src/backend/commands/tablecmds.c | 29 ++++++++++-------------
src/test/regress/expected/alter_table.out | 20 ++++++++++++++++
src/test/regress/sql/alter_table.sql | 6 +++++
3 files changed, 39 insertions(+), 16 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 7d0bdc9ac4..62398b6882 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -17873,31 +17873,28 @@ 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);
+ List *constraintExpr;
Constraint *n;
- constraintExpr = make_ands_explicit(RelationGetPartitionQual(partRel));
+ constraintExpr = (List *) eval_const_expressions(NULL,
+ (Node *) RelationGetPartitionQual(partRel));
- /* If an identical constraint exists, we don't need to create one */
- if (td->constr && td->constr->num_check > 0)
- {
- for (int i = 0; i < td->constr->num_check; i++)
- {
- Node *thisconstr;
+ constraintExpr = (List *) make_ands_explicit(constraintExpr);
- thisconstr = stringToNode(td->constr->check[i].ccbin);
-
- if (equal(constraintExpr, thisconstr))
- return;
- }
- }
+ /*
+ * Avoid adding a new constraint if the needed constraint is implied by an
+ * existing constraint
+ */
+ if (PartConstraintImpliedByRelConstraint(partRel,
+ list_make1(constraintExpr)))
+ return;
tab = ATGetQueueEntry(wqueue, partRel);
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index ec14b060a6..f81bdf513b 100644
--- 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 7a9c9252dc..dc0200adcb 100644
--- 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
--
2.17.0
