Hello
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.
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:
https://postgr.es/m/7682253a-6f79-6a92-00aa-267c4c412...@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)
https://postgr.es/m/CAFBsxsG4OWHBbSDM%3DsSeXrQGOtkPiOEOuME4yD7Ce41NtaAD9g%40mail.gmail.com
>From 885383c384bf6c58bb7a65e9744b3cbe078c76d8 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@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
