From: Justin Pryzby <pryzby@telsasoft.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Amit Langote <amitlangote09@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: ALTER TABLE .. DETACH PARTITION CONCURRENTLY
Date: Sat, 10 Apr 2021 13:42:26 -0500
Message-ID: <20210410184226.GY6592@telsasoft.com> (raw)
In-Reply-To: <20210321180115.GA2101@alvherre.pgsql>
References: <20210321175453.GB11765@telsasoft.com>
<20210321180115.GA2101@alvherre.pgsql>
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
Attachments:
[text/x-diff] 0001-DETACH-CONCURRENTLY-avoid-creation-of-redundant-cons.patch (4.2K, ../20210410184226.GY6592@telsasoft.com/2-0001-DETACH-CONCURRENTLY-avoid-creation-of-redundant-cons.patch)
download | inline diff:
From 1ecc8cb17192d13f7432fe582a43ab8597b15c00 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@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.cindex 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.outindex 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.sqlindex 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: pryzby@telsasoft.com, alvherre@alvh.no-ip.org, amitlangote09@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: ALTER TABLE .. DETACH PARTITION CONCURRENTLY
In-Reply-To: <20210410184226.GY6592@telsasoft.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox