ALTER TABLE ONLY .. DROP CONSTRAINT on partitioned tables
Hello
While studying a review note from Jian He on not-null constraints, I
came across some behavior introduced by commit 9139aa19423b[1]Discussion: /messages/by-id/7682253a-6f79-6a92-00aa-267c4c412870@lab.ntt.co.jp that I
think is mistaken. Consider the following example:
CREATE TABLE parted (a int CONSTRAINT the_check CHECK (a > 0)) PARTITION BY LIST (a);
CREATE TABLE parted_1 PARTITION OF parted FOR VALUES IN (1);
ALTER TABLE ONLY parted DROP CONSTRAINT the_check;
The ALTER TABLE fails with the following message:
ERROR: cannot remove constraint from only the partitioned table when partitions exist
HINT: Do not specify the ONLY keyword.
and the relevant code in ATExecDropConstraint is:
/*
* For a partitioned table, if partitions exist and we are told not to
* recurse, it's a user error. It doesn't make sense to have a constraint
* be defined only on the parent, especially if it's a partitioned table.
*/
if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
children != NIL && !recurse)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
errmsg("cannot remove constraint from only the partitioned table when partitions exist"),
errhint("Do not specify the ONLY keyword.")));
Note that the comment here is confused: it talks about a constraint that
would "be defined only on the parent", but that's bogus: the end result
would be that the constraint no longer exist on the parent but would
continue to exist on the children. Indeed it's not entirely
unimaginable that you start with a partitioned table with a bunch of
constraints which are enforced on all partitions, then you later decide
that you want this constraint to apply only to some of the partitions,
not the whole partitioned table. To implement that, you would drop the
constraint on the parent using ONLY, then drop it on a few of the
partitions, but still keep it on the other partitions. This would work
just fine if not for this ereport(ERROR).
Also, you can achieve the same end result by creating the constraint on
only some of the partitions and not on the partitioned table to start
with.
This also applies to ALTER TABLE ONLY ... DROP NOT NULL.
Of course, *adding* a constraint in this fashion is also forbidden, but
that makes perfect sense. Both restrictions were added as part of the
same commit, so I suppose we thought they were symmetrical behaviors and
failed to notice they weren't.
The DROP of such constraints can already be done on a table with legacy
inheritance children; it's just partitioned tables that have this
weirdness.
It doesn't surprise me that nobody has reported this inconsistency,
because it seems an unusual enough situation. For the same reason, I
wouldn't propose to backpatch this change. But I put forward the
attached patch, which removes the ereport(ERROR)s.
[1]: Discussion: /messages/by-id/7682253a-6f79-6a92-00aa-267c4c412870@lab.ntt.co.jp
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Here's a general engineering tip: if the non-fun part is too complex for you
to figure out, that might indicate the fun part is too ambitious." (John Naylor)
/messages/by-id/CAFBsxsG4OWHBbSDM=sSeXrQGOtkPiOEOuME4yD7Ce41NtaAD9g@mail.gmail.com
Attachments:
0001-Don-t-disallow-DROP-of-constraints-ONLY-on-partition.patchtext/x-diff; charset=utf-8Download
From 885383c384bf6c58bb7a65e9744b3cbe078c76d8 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Thu, 26 Sep 2024 19:37:50 +0200
Subject: [PATCH] Don't disallow DROP of constraints ONLY on partitioned tables
This restriction seems to have come about due to some fuzzy thinking.
---
src/backend/commands/tablecmds.c | 34 -----------------------
src/test/regress/expected/alter_table.out | 9 ++----
src/test/regress/sql/alter_table.sql | 5 ++--
3 files changed, 4 insertions(+), 44 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 87232e0137..a45f456af5 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -447,7 +447,6 @@ static bool check_for_column_name_collision(Relation rel, const char *colname,
bool if_not_exists);
static void add_column_datatype_dependency(Oid relid, int32 attnum, Oid typid);
static void add_column_collation_dependency(Oid relid, int32 attnum, Oid collid);
-static void ATPrepDropNotNull(Relation rel, bool recurse, bool recursing);
static ObjectAddress ATExecDropNotNull(Relation rel, const char *colName, LOCKMODE lockmode);
static void ATPrepSetNotNull(List **wqueue, Relation rel,
AlterTableCmd *cmd, bool recurse, bool recursing,
@@ -4858,7 +4857,6 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
case AT_DropNotNull: /* ALTER COLUMN DROP NOT NULL */
ATSimplePermissions(cmd->subtype, rel,
ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
- ATPrepDropNotNull(rel, recurse, recursing);
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
pass = AT_PASS_DROP;
break;
@@ -7500,26 +7498,6 @@ add_column_collation_dependency(Oid relid, int32 attnum, Oid collid)
* ALTER TABLE ALTER COLUMN DROP NOT NULL
*/
-static void
-ATPrepDropNotNull(Relation rel, bool recurse, bool recursing)
-{
- /*
- * If the parent is a partitioned table, like check constraints, we do not
- * support removing the NOT NULL while partitions exist.
- */
- if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
- {
- PartitionDesc partdesc = RelationGetPartitionDesc(rel, true);
-
- Assert(partdesc != NULL);
- if (partdesc->nparts > 0 && !recurse && !recursing)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
- errmsg("cannot remove constraint from only the partitioned table when partitions exist"),
- errhint("Do not specify the ONLY keyword.")));
- }
-}
-
/*
* Return the address of the modified column. If the column was already
* nullable, InvalidObjectAddress is returned.
@@ -12713,18 +12691,6 @@ ATExecDropConstraint(Relation rel, const char *constrName,
else
children = NIL;
- /*
- * For a partitioned table, if partitions exist and we are told not to
- * recurse, it's a user error. It doesn't make sense to have a constraint
- * be defined only on the parent, especially if it's a partitioned table.
- */
- if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
- children != NIL && !recurse)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_TABLE_DEFINITION),
- errmsg("cannot remove constraint from only the partitioned table when partitions exist"),
- errhint("Do not specify the ONLY keyword.")));
-
foreach_oid(childrelid, children)
{
Relation childrel;
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 3b3b0738d7..445d68d160 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -4401,7 +4401,7 @@ ALTER TABLE part_2 RENAME COLUMN b to c;
ERROR: cannot rename inherited column "b"
ALTER TABLE part_2 ALTER COLUMN b TYPE text;
ERROR: cannot alter inherited column "b"
--- cannot add/drop NOT NULL or check constraints to *only* the parent, when
+-- cannot add NOT NULL or check constraints to *only* the parent, when
-- partitions exist
ALTER TABLE ONLY list_parted2 ALTER b SET NOT NULL;
ERROR: constraint must be added to child tables too
@@ -4409,20 +4409,15 @@ DETAIL: Column "b" of relation "part_2" is not already NOT NULL.
HINT: Do not specify the ONLY keyword.
ALTER TABLE ONLY list_parted2 ADD CONSTRAINT check_b CHECK (b <> 'zz');
ERROR: constraint must be added to child tables too
+-- dropping them is ok though
ALTER TABLE list_parted2 ALTER b SET NOT NULL;
ALTER TABLE ONLY list_parted2 ALTER b DROP NOT NULL;
-ERROR: cannot remove constraint from only the partitioned table when partitions exist
-HINT: Do not specify the ONLY keyword.
ALTER TABLE list_parted2 ADD CONSTRAINT check_b CHECK (b <> 'zz');
ALTER TABLE ONLY list_parted2 DROP CONSTRAINT check_b;
-ERROR: cannot remove constraint from only the partitioned table when partitions exist
-HINT: Do not specify the ONLY keyword.
-- It's alright though, if no partitions are yet created
CREATE TABLE parted_no_parts (a int) PARTITION BY LIST (a);
ALTER TABLE ONLY parted_no_parts ALTER a SET NOT NULL;
ALTER TABLE ONLY parted_no_parts ADD CONSTRAINT check_a CHECK (a > 0);
-ALTER TABLE ONLY parted_no_parts ALTER a DROP NOT NULL;
-ALTER TABLE ONLY parted_no_parts DROP CONSTRAINT check_a;
DROP TABLE parted_no_parts;
-- cannot drop inherited NOT NULL or check constraints from partition
ALTER TABLE list_parted2 ALTER b SET NOT NULL, ADD CONSTRAINT check_a2 CHECK (a > 0);
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 453799abed..0eac8ed2e9 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2815,11 +2815,12 @@ ALTER TABLE part_2 DROP COLUMN b;
ALTER TABLE part_2 RENAME COLUMN b to c;
ALTER TABLE part_2 ALTER COLUMN b TYPE text;
--- cannot add/drop NOT NULL or check constraints to *only* the parent, when
+-- cannot add NOT NULL or check constraints to *only* the parent, when
-- partitions exist
ALTER TABLE ONLY list_parted2 ALTER b SET NOT NULL;
ALTER TABLE ONLY list_parted2 ADD CONSTRAINT check_b CHECK (b <> 'zz');
+-- dropping them is ok though
ALTER TABLE list_parted2 ALTER b SET NOT NULL;
ALTER TABLE ONLY list_parted2 ALTER b DROP NOT NULL;
ALTER TABLE list_parted2 ADD CONSTRAINT check_b CHECK (b <> 'zz');
@@ -2829,8 +2830,6 @@ ALTER TABLE ONLY list_parted2 DROP CONSTRAINT check_b;
CREATE TABLE parted_no_parts (a int) PARTITION BY LIST (a);
ALTER TABLE ONLY parted_no_parts ALTER a SET NOT NULL;
ALTER TABLE ONLY parted_no_parts ADD CONSTRAINT check_a CHECK (a > 0);
-ALTER TABLE ONLY parted_no_parts ALTER a DROP NOT NULL;
-ALTER TABLE ONLY parted_no_parts DROP CONSTRAINT check_a;
DROP TABLE parted_no_parts;
-- cannot drop inherited NOT NULL or check constraints from partition
--
2.39.2
Hi Alvaro,
On Fri, Sep 27, 2024 at 2:52 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
While studying a review note from Jian He on not-null constraints, I
came across some behavior introduced by commit 9139aa19423b[1] that I
think is mistaken. Consider the following example:CREATE TABLE parted (a int CONSTRAINT the_check CHECK (a > 0)) PARTITION BY LIST (a);
CREATE TABLE parted_1 PARTITION OF parted FOR VALUES IN (1);
ALTER TABLE ONLY parted DROP CONSTRAINT the_check;The ALTER TABLE fails with the following message:
ERROR: cannot remove constraint from only the partitioned table when partitions exist
HINT: Do not specify the ONLY keyword.and the relevant code in ATExecDropConstraint is:
/*
* For a partitioned table, if partitions exist and we are told not to
* recurse, it's a user error. It doesn't make sense to have a constraint
* be defined only on the parent, especially if it's a partitioned table.
*/
if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE &&
children != NIL && !recurse)
ereport(ERROR,
(errcode(ERRCODE_INVALID_TABLE_DEFINITION),
errmsg("cannot remove constraint from only the partitioned table when partitions exist"),
errhint("Do not specify the ONLY keyword.")));Note that the comment here is confused: it talks about a constraint that
would "be defined only on the parent", but that's bogus: the end result
would be that the constraint no longer exist on the parent but would
continue to exist on the children. Indeed it's not entirely
unimaginable that you start with a partitioned table with a bunch of
constraints which are enforced on all partitions, then you later decide
that you want this constraint to apply only to some of the partitions,
not the whole partitioned table. To implement that, you would drop the
constraint on the parent using ONLY, then drop it on a few of the
partitions, but still keep it on the other partitions. This would work
just fine if not for this ereport(ERROR).Also, you can achieve the same end result by creating the constraint on
only some of the partitions and not on the partitioned table to start
with.This also applies to ALTER TABLE ONLY ... DROP NOT NULL.
Of course, *adding* a constraint in this fashion is also forbidden, but
that makes perfect sense. Both restrictions were added as part of the
same commit, so I suppose we thought they were symmetrical behaviors and
failed to notice they weren't.The DROP of such constraints can already be done on a table with legacy
inheritance children; it's just partitioned tables that have this
weirdness.
Yeah, I don’t quite recall why I thought the behavior for both ADD and
DROP had to be the same. I went back and reviewed the thread, trying
to understand why DROP was included in the decision, but couldn’t find
anything that explained it. It also doesn’t seem to be related to the
pg_dump issue that was being discussed at the time.
So, I think you might be right that the restriction on DROP is
overkill, and we should consider removing it, at least in the master
branch.
It doesn't surprise me that nobody has reported this inconsistency,
because it seems an unusual enough situation. For the same reason, I
wouldn't propose to backpatch this change. But I put forward the
attached patch, which removes the ereport(ERROR)s.
The patch looks good to me.
--
Thanks, Amit Langote
Hello,
On 2024-Sep-27, Amit Langote wrote:
On Fri, Sep 27, 2024 at 2:52 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
While studying a review note from Jian He on not-null constraints, I
came across some behavior introduced by commit 9139aa19423b[1] that I
think is mistaken.
Yeah, I don’t quite recall why I thought the behavior for both ADD and
DROP had to be the same. I went back and reviewed the thread, trying
to understand why DROP was included in the decision, but couldn’t find
anything that explained it. It also doesn’t seem to be related to the
pg_dump issue that was being discussed at the time.
Right.
So, I think you might be right that the restriction on DROP is
overkill, and we should consider removing it, at least in the master
branch.
Thanks for looking! I have pushed the removal now.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
<inflex> really, I see PHP as like a strange amalgamation of C, Perl, Shell
<crab> inflex: you know that "amalgam" means "mixture with mercury",
more or less, right?
<crab> i.e., "deadly poison"
On Mon, Sep 30, 2024 at 6:01 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
Hello,
On 2024-Sep-27, Amit Langote wrote:
On Fri, Sep 27, 2024 at 2:52 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
While studying a review note from Jian He on not-null constraints, I
came across some behavior introduced by commit 9139aa19423b[1] that I
think is mistaken.Yeah, I don’t quite recall why I thought the behavior for both ADD and
DROP had to be the same. I went back and reviewed the thread, trying
to understand why DROP was included in the decision, but couldn’t find
anything that explained it. It also doesn’t seem to be related to the
pg_dump issue that was being discussed at the time.Right.
So, I think you might be right that the restriction on DROP is
overkill, and we should consider removing it, at least in the master
branch.Thanks for looking! I have pushed the removal now.
works fine with check constraint, drop not null.
but seems not with primary key/unique constraint.
drop table if exists idxpart, idxpart0 cascade;
create table idxpart (a int not null primary key, constraint nc check
(a > 1)) partition by range (a);
create table idxpart0 (a int not null primary key, constraint nc check (a > 1));
alter table idxpart attach partition idxpart0 for values from (0) to (1000);
alter table only idxpart drop constraint nc;
alter table only idxpart drop constraint idxpart_pkey;
in the above case:
alter table only idxpart drop constraint idxpart_pkey;
is logical equivalent to
alter table idxpart drop constraint idxpart_pkey;
not sure this is desired behavior.