Fix ALTER TABLE DROP EXPRESSION with inheritance hierarchy
hi.
--this ALTER COLUMN DROP EXPRESSION work as expected
DROP TABLE IF EXISTS parent cascade;
CREATE TABLE parent (a int, d INT GENERATED ALWAYS AS (11) STORED);
CREATE TABLE child () INHERITS (parent);
ALTER TABLE parent ALTER COLUMN d DROP EXPRESSION;
----- the below (ALTER COLUMN DROP EXPRESSION) should also work.
-----
DROP TABLE IF EXISTS parent cascade;
CREATE TABLE parent (a int, d INT GENERATED ALWAYS AS (11) STORED);
CREATE TABLE child () INHERITS (parent);
CREATE TABLE grandchild () INHERITS (child);
ALTER TABLE parent ALTER COLUMN d DROP EXPRESSION;
but currently it will generated error:
ERROR: 0A000: ALTER TABLE / DROP EXPRESSION must be applied to child tables too
LOCATION: ATPrepDropExpression, tablecmds.c:8734
The attached patch fixes this potential issue.
Attachments:
v1-0001-Fix-ALTER-TABLE-DROP-EXPRESSION-with-inheritance-hierarch.patchtext/x-patch; charset=US-ASCII; name=v1-0001-Fix-ALTER-TABLE-DROP-EXPRESSION-with-inheritance-hierarch.patchDownload
From 78f2d0734d492296289671cc0b740329d1f2da30 Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
Date: Sun, 24 Aug 2025 16:57:01 +0800
Subject: [PATCH v1 1/1] Fix ALTER TABLE DROP EXPRESSION with inheritance
hierarchy
--this works.
DROP TABLE IF EXISTS parent cascade;
CREATE TABLE parent (a int, d INT GENERATED ALWAYS AS (11) STORED);
CREATE TABLE child () INHERITS (parent);
ALTER TABLE parent ALTER COLUMN d DROP EXPRESSION;
-- so the below should also works.
DROP TABLE IF EXISTS parent cascade;
CREATE TABLE parent (a int, d INT GENERATED ALWAYS AS (11) STORED);
CREATE TABLE child () INHERITS (parent);
CREATE TABLE grandchild () INHERITS (child);
ALTER TABLE parent ALTER COLUMN d DROP EXPRESSION;
discussion: https://postgr.es/m/
---
src/backend/commands/tablecmds.c | 11 +++++++----
src/test/regress/expected/generated_stored.out | 14 +++++++++++++-
src/test/regress/expected/generated_virtual.out | 14 +++++++++++++-
src/test/regress/sql/generated_stored.sql | 2 ++
src/test/regress/sql/generated_virtual.sql | 2 ++
5 files changed, 37 insertions(+), 6 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 082a3575d62..cbc5ab7d7c0 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -521,7 +521,8 @@ static ObjectAddress ATExecDropIdentity(Relation rel, const char *colName, bool
bool recurse, bool recursing);
static ObjectAddress ATExecSetExpression(AlteredTableInfo *tab, Relation rel, const char *colName,
Node *newExpr, LOCKMODE lockmode);
-static void ATPrepDropExpression(Relation rel, AlterTableCmd *cmd, bool recurse, bool recursing, LOCKMODE lockmode);
+static void ATPrepDropExpression(Relation rel, AlterTableCmd *cmd, bool recurse, bool recursing, LOCKMODE lockmode,
+ AlterTableUtilityContext *context);
static ObjectAddress ATExecDropExpression(Relation rel, const char *colName, bool missing_ok, LOCKMODE lockmode);
static ObjectAddress ATExecSetStatistics(Relation rel, const char *colName, int16 colNum,
Node *newValue, LOCKMODE lockmode);
@@ -5024,7 +5025,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
ATSimplePermissions(cmd->subtype, rel,
ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
- ATPrepDropExpression(rel, cmd, recurse, recursing, lockmode);
+ ATPrepDropExpression(rel, cmd, recurse, recursing, lockmode, context);
pass = AT_PASS_DROP;
break;
case AT_SetStatistics: /* ALTER COLUMN SET STATISTICS */
@@ -8717,7 +8718,8 @@ ATExecSetExpression(AlteredTableInfo *tab, Relation rel, const char *colName,
* ALTER TABLE ALTER COLUMN DROP EXPRESSION
*/
static void
-ATPrepDropExpression(Relation rel, AlterTableCmd *cmd, bool recurse, bool recursing, LOCKMODE lockmode)
+ATPrepDropExpression(Relation rel, AlterTableCmd *cmd, bool recurse, bool recursing, LOCKMODE lockmode,
+ AlterTableUtilityContext *context)
{
/*
* Reject ONLY if there are child tables. We could implement this, but it
@@ -8730,7 +8732,8 @@ ATPrepDropExpression(Relation rel, AlterTableCmd *cmd, bool recurse, bool recurs
* resulting state can be properly dumped and restored.
*/
if (!recurse &&
- find_inheritance_children(RelationGetRelid(rel), lockmode))
+ find_inheritance_children(RelationGetRelid(rel), lockmode) &&
+ RelationGetRelid(rel) == context->relid)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("ALTER TABLE / DROP EXPRESSION must be applied to child tables too")));
diff --git a/src/test/regress/expected/generated_stored.out b/src/test/regress/expected/generated_stored.out
index adac2cedfb2..a2b04f564eb 100644
--- a/src/test/regress/expected/generated_stored.out
+++ b/src/test/regress/expected/generated_stored.out
@@ -1264,6 +1264,7 @@ CREATE TABLE gtest30 (
b int GENERATED ALWAYS AS (a * 2) STORED
);
CREATE TABLE gtest30_1 () INHERITS (gtest30);
+CREATE TABLE gtest30_1_1 () INHERITS (gtest30_1);
ALTER TABLE gtest30 ALTER COLUMN b DROP EXPRESSION;
\d gtest30
Table "generated_stored_tests.gtest30"
@@ -1280,9 +1281,20 @@ Number of child tables: 1 (Use \d+ to list them.)
a | integer | | |
b | integer | | |
Inherits: gtest30
+Number of child tables: 1 (Use \d+ to list them.)
+
+\d gtest30_1_1
+ Table "generated_stored_tests.gtest30_1_1"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+ b | integer | | |
+Inherits: gtest30_1
DROP TABLE gtest30 CASCADE;
-NOTICE: drop cascades to table gtest30_1
+NOTICE: drop cascades to 2 other objects
+DETAIL: drop cascades to table gtest30_1
+drop cascades to table gtest30_1_1
CREATE TABLE gtest30 (
a int,
b int GENERATED ALWAYS AS (a * 2) STORED
diff --git a/src/test/regress/expected/generated_virtual.out b/src/test/regress/expected/generated_virtual.out
index aca6347babe..50a32996f4f 100644
--- a/src/test/regress/expected/generated_virtual.out
+++ b/src/test/regress/expected/generated_virtual.out
@@ -1232,6 +1232,7 @@ CREATE TABLE gtest30 (
b int GENERATED ALWAYS AS (a * 2) VIRTUAL
);
CREATE TABLE gtest30_1 () INHERITS (gtest30);
+CREATE TABLE gtest30_1_1 () INHERITS (gtest30_1);
ALTER TABLE gtest30 ALTER COLUMN b DROP EXPRESSION;
ERROR: ALTER TABLE / DROP EXPRESSION is not supported for virtual generated columns
DETAIL: Column "b" of relation "gtest30" is a virtual generated column.
@@ -1250,9 +1251,20 @@ Number of child tables: 1 (Use \d+ to list them.)
a | integer | | |
b | integer | | | generated always as (a * 2)
Inherits: gtest30
+Number of child tables: 1 (Use \d+ to list them.)
+
+\d gtest30_1_1
+ Table "generated_virtual_tests.gtest30_1_1"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+-----------------------------
+ a | integer | | |
+ b | integer | | | generated always as (a * 2)
+Inherits: gtest30_1
DROP TABLE gtest30 CASCADE;
-NOTICE: drop cascades to table gtest30_1
+NOTICE: drop cascades to 2 other objects
+DETAIL: drop cascades to table gtest30_1
+drop cascades to table gtest30_1_1
CREATE TABLE gtest30 (
a int,
b int GENERATED ALWAYS AS (a * 2) VIRTUAL
diff --git a/src/test/regress/sql/generated_stored.sql b/src/test/regress/sql/generated_stored.sql
index f56fde8d4e5..3c8931c677e 100644
--- a/src/test/regress/sql/generated_stored.sql
+++ b/src/test/regress/sql/generated_stored.sql
@@ -577,9 +577,11 @@ CREATE TABLE gtest30 (
b int GENERATED ALWAYS AS (a * 2) STORED
);
CREATE TABLE gtest30_1 () INHERITS (gtest30);
+CREATE TABLE gtest30_1_1 () INHERITS (gtest30_1);
ALTER TABLE gtest30 ALTER COLUMN b DROP EXPRESSION;
\d gtest30
\d gtest30_1
+\d gtest30_1_1
DROP TABLE gtest30 CASCADE;
CREATE TABLE gtest30 (
a int,
diff --git a/src/test/regress/sql/generated_virtual.sql b/src/test/regress/sql/generated_virtual.sql
index ba19bc4c701..5696be4b36a 100644
--- a/src/test/regress/sql/generated_virtual.sql
+++ b/src/test/regress/sql/generated_virtual.sql
@@ -628,9 +628,11 @@ CREATE TABLE gtest30 (
b int GENERATED ALWAYS AS (a * 2) VIRTUAL
);
CREATE TABLE gtest30_1 () INHERITS (gtest30);
+CREATE TABLE gtest30_1_1 () INHERITS (gtest30_1);
ALTER TABLE gtest30 ALTER COLUMN b DROP EXPRESSION;
\d gtest30
\d gtest30_1
+\d gtest30_1_1
DROP TABLE gtest30 CASCADE;
CREATE TABLE gtest30 (
a int,
--
2.34.1
Hi!
On Sun, 24 Aug 2025 at 14:05, jian he <jian.universality@gmail.com> wrote:
hi.
--this ALTER COLUMN DROP EXPRESSION work as expected
DROP TABLE IF EXISTS parent cascade;
CREATE TABLE parent (a int, d INT GENERATED ALWAYS AS (11) STORED);
CREATE TABLE child () INHERITS (parent);
ALTER TABLE parent ALTER COLUMN d DROP EXPRESSION;----- the below (ALTER COLUMN DROP EXPRESSION) should also work.
-----
DROP TABLE IF EXISTS parent cascade;
CREATE TABLE parent (a int, d INT GENERATED ALWAYS AS (11) STORED);
CREATE TABLE child () INHERITS (parent);
CREATE TABLE grandchild () INHERITS (child);
ALTER TABLE parent ALTER COLUMN d DROP EXPRESSION;but currently it will generated error:
ERROR: 0A000: ALTER TABLE / DROP EXPRESSION must be applied to child tables too
LOCATION: ATPrepDropExpression, tablecmds.c:8734The attached patch fixes this potential issue.
Good catch, I agree that current behaviour is not correct.
However, I am not terribly sure that your suggested modification is
addressing the issues appropriately.
My understanding is that this if statement protects when user
specifies ONLY option in ALTER TABLE:
if (!recurse && - find_inheritance_children(RelationGetRelid(rel), lockmode)) + find_inheritance_children(RelationGetRelid(rel), lockmode) && + RelationGetRelid(rel) == context->relid)
So we need to detect if the user did ALTER TABLE or ALTER TABLE ONLY.
And we have two parameters passed to ATPrepDropExpression: "recurse"
and "recursing".
First is about whether the user specified ONLY option and second is
about if we are recursing in our AT code. So maybe fix it as in
attached?
===
I also spotted potential enhancement in the error message: we can add
HINT here, akin to partitioned table processing. WHYT?
```
reshke=# begin;
BEGIN
reshke=*# ALTER TABLE parent ALTER COLUMN d DROP EXPRESSION;
ALTER TABLE
reshke=*# rollback ;
ROLLBACK
reshke=# begin;
BEGIN
reshke=*# ALTER TABLE ONLY parent ALTER COLUMN d DROP EXPRESSION;
ERROR: ALTER TABLE / DROP EXPRESSION must be applied to child tables too
HINT: Do not specify the ONLY keyword.
reshke=!# rollback ;
ROLLBACK
```
--
Best regards,
Kirill Reshke
Attachments:
v2-0001-Fix-ALTER-TABLE-DROP-EXPRESSION-with-inheritance-.patchapplication/octet-stream; name=v2-0001-Fix-ALTER-TABLE-DROP-EXPRESSION-with-inheritance-.patchDownload
From 1c3be532822b57af6e2268150f65c68cc6233ae1 Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
Date: Sun, 24 Aug 2025 16:57:01 +0800
Subject: [PATCH v2] Fix ALTER TABLE DROP EXPRESSION with inheritance hierarchy
--this works.
DROP TABLE IF EXISTS parent cascade;
CREATE TABLE parent (a int, d INT GENERATED ALWAYS AS (11) STORED);
CREATE TABLE child () INHERITS (parent);
ALTER TABLE parent ALTER COLUMN d DROP EXPRESSION;
-- so the below should also works.
DROP TABLE IF EXISTS parent cascade;
CREATE TABLE parent (a int, d INT GENERATED ALWAYS AS (11) STORED);
CREATE TABLE child () INHERITS (parent);
CREATE TABLE grandchild () INHERITS (child);
ALTER TABLE parent ALTER COLUMN d DROP EXPRESSION;
discussion: https://postgr.es/m/
---
src/backend/commands/tablecmds.c | 15 +++++++++------
src/test/regress/expected/generated_stored.out | 15 ++++++++++++++-
src/test/regress/expected/generated_virtual.out | 15 ++++++++++++++-
src/test/regress/sql/generated_stored.sql | 2 ++
src/test/regress/sql/generated_virtual.sql | 2 ++
5 files changed, 41 insertions(+), 8 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 082a3575d62..7bda8089fe3 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -521,7 +521,8 @@ static ObjectAddress ATExecDropIdentity(Relation rel, const char *colName, bool
bool recurse, bool recursing);
static ObjectAddress ATExecSetExpression(AlteredTableInfo *tab, Relation rel, const char *colName,
Node *newExpr, LOCKMODE lockmode);
-static void ATPrepDropExpression(Relation rel, AlterTableCmd *cmd, bool recurse, bool recursing, LOCKMODE lockmode);
+static void ATPrepDropExpression(Relation rel, AlterTableCmd *cmd, bool recurse, bool recursing, LOCKMODE lockmode,
+ AlterTableUtilityContext *context);
static ObjectAddress ATExecDropExpression(Relation rel, const char *colName, bool missing_ok, LOCKMODE lockmode);
static ObjectAddress ATExecSetStatistics(Relation rel, const char *colName, int16 colNum,
Node *newValue, LOCKMODE lockmode);
@@ -5024,7 +5025,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
ATSimplePermissions(cmd->subtype, rel,
ATT_TABLE | ATT_PARTITIONED_TABLE | ATT_FOREIGN_TABLE);
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
- ATPrepDropExpression(rel, cmd, recurse, recursing, lockmode);
+ ATPrepDropExpression(rel, cmd, recurse, recursing, lockmode, context);
pass = AT_PASS_DROP;
break;
case AT_SetStatistics: /* ALTER COLUMN SET STATISTICS */
@@ -8717,7 +8718,8 @@ ATExecSetExpression(AlteredTableInfo *tab, Relation rel, const char *colName,
* ALTER TABLE ALTER COLUMN DROP EXPRESSION
*/
static void
-ATPrepDropExpression(Relation rel, AlterTableCmd *cmd, bool recurse, bool recursing, LOCKMODE lockmode)
+ATPrepDropExpression(Relation rel, AlterTableCmd *cmd, bool recurse, bool recursing, LOCKMODE lockmode,
+ AlterTableUtilityContext *context)
{
/*
* Reject ONLY if there are child tables. We could implement this, but it
@@ -8729,11 +8731,12 @@ ATPrepDropExpression(Relation rel, AlterTableCmd *cmd, bool recurse, bool recurs
* tables, somewhat similar to how DROP COLUMN does it, so that the
* resulting state can be properly dumped and restored.
*/
- if (!recurse &&
+ if (!recurse && !recursing &&
find_inheritance_children(RelationGetRelid(rel), lockmode))
ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("ALTER TABLE / DROP EXPRESSION must be applied to child tables too")));
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("ALTER TABLE / DROP EXPRESSION must be applied to child tables too"),
+ errhint("Do not specify the ONLY keyword."));
/*
* Cannot drop generation expression from inherited columns.
diff --git a/src/test/regress/expected/generated_stored.out b/src/test/regress/expected/generated_stored.out
index adac2cedfb2..36af4e1d700 100644
--- a/src/test/regress/expected/generated_stored.out
+++ b/src/test/regress/expected/generated_stored.out
@@ -1264,6 +1264,7 @@ CREATE TABLE gtest30 (
b int GENERATED ALWAYS AS (a * 2) STORED
);
CREATE TABLE gtest30_1 () INHERITS (gtest30);
+CREATE TABLE gtest30_1_1 () INHERITS (gtest30_1);
ALTER TABLE gtest30 ALTER COLUMN b DROP EXPRESSION;
\d gtest30
Table "generated_stored_tests.gtest30"
@@ -1280,9 +1281,20 @@ Number of child tables: 1 (Use \d+ to list them.)
a | integer | | |
b | integer | | |
Inherits: gtest30
+Number of child tables: 1 (Use \d+ to list them.)
+
+\d gtest30_1_1
+ Table "generated_stored_tests.gtest30_1_1"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+ b | integer | | |
+Inherits: gtest30_1
DROP TABLE gtest30 CASCADE;
-NOTICE: drop cascades to table gtest30_1
+NOTICE: drop cascades to 2 other objects
+DETAIL: drop cascades to table gtest30_1
+drop cascades to table gtest30_1_1
CREATE TABLE gtest30 (
a int,
b int GENERATED ALWAYS AS (a * 2) STORED
@@ -1290,6 +1302,7 @@ CREATE TABLE gtest30 (
CREATE TABLE gtest30_1 () INHERITS (gtest30);
ALTER TABLE ONLY gtest30 ALTER COLUMN b DROP EXPRESSION; -- error
ERROR: ALTER TABLE / DROP EXPRESSION must be applied to child tables too
+HINT: Do not specify the ONLY keyword.
\d gtest30
Table "generated_stored_tests.gtest30"
Column | Type | Collation | Nullable | Default
diff --git a/src/test/regress/expected/generated_virtual.out b/src/test/regress/expected/generated_virtual.out
index aca6347babe..565a6357f39 100644
--- a/src/test/regress/expected/generated_virtual.out
+++ b/src/test/regress/expected/generated_virtual.out
@@ -1232,6 +1232,7 @@ CREATE TABLE gtest30 (
b int GENERATED ALWAYS AS (a * 2) VIRTUAL
);
CREATE TABLE gtest30_1 () INHERITS (gtest30);
+CREATE TABLE gtest30_1_1 () INHERITS (gtest30_1);
ALTER TABLE gtest30 ALTER COLUMN b DROP EXPRESSION;
ERROR: ALTER TABLE / DROP EXPRESSION is not supported for virtual generated columns
DETAIL: Column "b" of relation "gtest30" is a virtual generated column.
@@ -1250,9 +1251,20 @@ Number of child tables: 1 (Use \d+ to list them.)
a | integer | | |
b | integer | | | generated always as (a * 2)
Inherits: gtest30
+Number of child tables: 1 (Use \d+ to list them.)
+
+\d gtest30_1_1
+ Table "generated_virtual_tests.gtest30_1_1"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+-----------------------------
+ a | integer | | |
+ b | integer | | | generated always as (a * 2)
+Inherits: gtest30_1
DROP TABLE gtest30 CASCADE;
-NOTICE: drop cascades to table gtest30_1
+NOTICE: drop cascades to 2 other objects
+DETAIL: drop cascades to table gtest30_1
+drop cascades to table gtest30_1_1
CREATE TABLE gtest30 (
a int,
b int GENERATED ALWAYS AS (a * 2) VIRTUAL
@@ -1260,6 +1272,7 @@ CREATE TABLE gtest30 (
CREATE TABLE gtest30_1 () INHERITS (gtest30);
ALTER TABLE ONLY gtest30 ALTER COLUMN b DROP EXPRESSION; -- error
ERROR: ALTER TABLE / DROP EXPRESSION must be applied to child tables too
+HINT: Do not specify the ONLY keyword.
\d gtest30
Table "generated_virtual_tests.gtest30"
Column | Type | Collation | Nullable | Default
diff --git a/src/test/regress/sql/generated_stored.sql b/src/test/regress/sql/generated_stored.sql
index f56fde8d4e5..3c8931c677e 100644
--- a/src/test/regress/sql/generated_stored.sql
+++ b/src/test/regress/sql/generated_stored.sql
@@ -577,9 +577,11 @@ CREATE TABLE gtest30 (
b int GENERATED ALWAYS AS (a * 2) STORED
);
CREATE TABLE gtest30_1 () INHERITS (gtest30);
+CREATE TABLE gtest30_1_1 () INHERITS (gtest30_1);
ALTER TABLE gtest30 ALTER COLUMN b DROP EXPRESSION;
\d gtest30
\d gtest30_1
+\d gtest30_1_1
DROP TABLE gtest30 CASCADE;
CREATE TABLE gtest30 (
a int,
diff --git a/src/test/regress/sql/generated_virtual.sql b/src/test/regress/sql/generated_virtual.sql
index ba19bc4c701..5696be4b36a 100644
--- a/src/test/regress/sql/generated_virtual.sql
+++ b/src/test/regress/sql/generated_virtual.sql
@@ -628,9 +628,11 @@ CREATE TABLE gtest30 (
b int GENERATED ALWAYS AS (a * 2) VIRTUAL
);
CREATE TABLE gtest30_1 () INHERITS (gtest30);
+CREATE TABLE gtest30_1_1 () INHERITS (gtest30_1);
ALTER TABLE gtest30 ALTER COLUMN b DROP EXPRESSION;
\d gtest30
\d gtest30_1
+\d gtest30_1_1
DROP TABLE gtest30 CASCADE;
CREATE TABLE gtest30 (
a int,
--
2.43.0
Looks like no CF entry for this thread.
CF entry [0]https://commitfest.postgresql.org/patch/5992/ created.
[0]: https://commitfest.postgresql.org/patch/5992/
--
Best regards,
Kirill Reshke
On Mon, Aug 25, 2025 at 9:04 PM Kirill Reshke <reshkekirill@gmail.com> wrote:
Hi!
On Sun, 24 Aug 2025 at 14:05, jian he <jian.universality@gmail.com> wrote:
hi.
--this ALTER COLUMN DROP EXPRESSION work as expected
DROP TABLE IF EXISTS parent cascade;
CREATE TABLE parent (a int, d INT GENERATED ALWAYS AS (11) STORED);
CREATE TABLE child () INHERITS (parent);
ALTER TABLE parent ALTER COLUMN d DROP EXPRESSION;----- the below (ALTER COLUMN DROP EXPRESSION) should also work.
-----
DROP TABLE IF EXISTS parent cascade;
CREATE TABLE parent (a int, d INT GENERATED ALWAYS AS (11) STORED);
CREATE TABLE child () INHERITS (parent);
CREATE TABLE grandchild () INHERITS (child);
ALTER TABLE parent ALTER COLUMN d DROP EXPRESSION;but currently it will generated error:
ERROR: 0A000: ALTER TABLE / DROP EXPRESSION must be applied to child tables too
LOCATION: ATPrepDropExpression, tablecmds.c:8734The attached patch fixes this potential issue.
Good catch, I agree that current behaviour is not correct.
However, I am not terribly sure that your suggested modification is
addressing the issues appropriately.My understanding is that this if statement protects when user
specifies ONLY option in ALTER TABLE:if (!recurse && - find_inheritance_children(RelationGetRelid(rel), lockmode)) + find_inheritance_children(RelationGetRelid(rel), lockmode) && + RelationGetRelid(rel) == context->relid)So we need to detect if the user did ALTER TABLE or ALTER TABLE ONLY.
And we have two parameters passed to ATPrepDropExpression: "recurse"
and "recursing".
First is about whether the user specified ONLY option and second is
about if we are recursing in our AT code. So maybe fix it as in
attached?
hi.
if (!recurse && !recursing &&
find_inheritance_children(RelationGetRelid(rel), lockmode))
ereport(ERROR,
errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("ALTER TABLE / DROP EXPRESSION must be applied
to child tables too"),
errhint("Do not specify the ONLY keyword."));
will work, after looking at ATPrepCmd below code, especially ATSimpleRecursion.
case AT_DropExpression: /* ALTER COLUMN DROP EXPRESSION */
ATSimplePermissions(cmd->subtype, rel,
ATT_TABLE | ATT_PARTITIONED_TABLE |
ATT_FOREIGN_TABLE);
ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context);
ATPrepDropExpression(rel, cmd, recurse, recursing,
lockmode, context);
pass = AT_PASS_DROP;
break;
That means, we don't need to change the ATPrepDropExpression function
argument for now?
===
I also spotted potential enhancement in the error message: we can add
HINT here, akin to partitioned table processing. WHYT?
I am ok with it.
On Thu, 28 Aug 2025 at 08:35, jian he <jian.universality@gmail.com> wrote:
That means, we don't need to change the ATPrepDropExpression function
argument for now?
Sure. V3 with this attached, and I think we can move cf entry to RFC
--
Best regards,
Kirill Reshke
Attachments:
v3-0001-Fix-ALTER-TABLE-DROP-EXPRESSION-with-inheritance-.patchapplication/octet-stream; name=v3-0001-Fix-ALTER-TABLE-DROP-EXPRESSION-with-inheritance-.patchDownload
From b70c90b86d5bf810ab2d57c99fa337c14e85bc83 Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
Date: Sun, 24 Aug 2025 16:57:01 +0800
Subject: [PATCH v3] Fix ALTER TABLE DROP EXPRESSION with inheritance hierarchy
--this works.
DROP TABLE IF EXISTS parent cascade;
CREATE TABLE parent (a int, d INT GENERATED ALWAYS AS (11) STORED);
CREATE TABLE child () INHERITS (parent);
ALTER TABLE parent ALTER COLUMN d DROP EXPRESSION;
-- so the below should also works.
DROP TABLE IF EXISTS parent cascade;
CREATE TABLE parent (a int, d INT GENERATED ALWAYS AS (11) STORED);
CREATE TABLE child () INHERITS (parent);
CREATE TABLE grandchild () INHERITS (child);
ALTER TABLE parent ALTER COLUMN d DROP EXPRESSION;
discussion: https://postgr.es/m/
---
src/backend/commands/tablecmds.c | 7 ++++---
src/test/regress/expected/generated_stored.out | 15 ++++++++++++++-
src/test/regress/expected/generated_virtual.out | 15 ++++++++++++++-
src/test/regress/sql/generated_stored.sql | 2 ++
src/test/regress/sql/generated_virtual.sql | 2 ++
5 files changed, 36 insertions(+), 5 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 082a3575d62..b6b48fe1b57 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -8729,11 +8729,12 @@ ATPrepDropExpression(Relation rel, AlterTableCmd *cmd, bool recurse, bool recurs
* tables, somewhat similar to how DROP COLUMN does it, so that the
* resulting state can be properly dumped and restored.
*/
- if (!recurse &&
+ if (!recurse && !recursing &&
find_inheritance_children(RelationGetRelid(rel), lockmode))
ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("ALTER TABLE / DROP EXPRESSION must be applied to child tables too")));
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("ALTER TABLE / DROP EXPRESSION must be applied to child tables too"),
+ errhint("Do not specify the ONLY keyword."));
/*
* Cannot drop generation expression from inherited columns.
diff --git a/src/test/regress/expected/generated_stored.out b/src/test/regress/expected/generated_stored.out
index adac2cedfb2..36af4e1d700 100644
--- a/src/test/regress/expected/generated_stored.out
+++ b/src/test/regress/expected/generated_stored.out
@@ -1264,6 +1264,7 @@ CREATE TABLE gtest30 (
b int GENERATED ALWAYS AS (a * 2) STORED
);
CREATE TABLE gtest30_1 () INHERITS (gtest30);
+CREATE TABLE gtest30_1_1 () INHERITS (gtest30_1);
ALTER TABLE gtest30 ALTER COLUMN b DROP EXPRESSION;
\d gtest30
Table "generated_stored_tests.gtest30"
@@ -1280,9 +1281,20 @@ Number of child tables: 1 (Use \d+ to list them.)
a | integer | | |
b | integer | | |
Inherits: gtest30
+Number of child tables: 1 (Use \d+ to list them.)
+
+\d gtest30_1_1
+ Table "generated_stored_tests.gtest30_1_1"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+---------
+ a | integer | | |
+ b | integer | | |
+Inherits: gtest30_1
DROP TABLE gtest30 CASCADE;
-NOTICE: drop cascades to table gtest30_1
+NOTICE: drop cascades to 2 other objects
+DETAIL: drop cascades to table gtest30_1
+drop cascades to table gtest30_1_1
CREATE TABLE gtest30 (
a int,
b int GENERATED ALWAYS AS (a * 2) STORED
@@ -1290,6 +1302,7 @@ CREATE TABLE gtest30 (
CREATE TABLE gtest30_1 () INHERITS (gtest30);
ALTER TABLE ONLY gtest30 ALTER COLUMN b DROP EXPRESSION; -- error
ERROR: ALTER TABLE / DROP EXPRESSION must be applied to child tables too
+HINT: Do not specify the ONLY keyword.
\d gtest30
Table "generated_stored_tests.gtest30"
Column | Type | Collation | Nullable | Default
diff --git a/src/test/regress/expected/generated_virtual.out b/src/test/regress/expected/generated_virtual.out
index aca6347babe..565a6357f39 100644
--- a/src/test/regress/expected/generated_virtual.out
+++ b/src/test/regress/expected/generated_virtual.out
@@ -1232,6 +1232,7 @@ CREATE TABLE gtest30 (
b int GENERATED ALWAYS AS (a * 2) VIRTUAL
);
CREATE TABLE gtest30_1 () INHERITS (gtest30);
+CREATE TABLE gtest30_1_1 () INHERITS (gtest30_1);
ALTER TABLE gtest30 ALTER COLUMN b DROP EXPRESSION;
ERROR: ALTER TABLE / DROP EXPRESSION is not supported for virtual generated columns
DETAIL: Column "b" of relation "gtest30" is a virtual generated column.
@@ -1250,9 +1251,20 @@ Number of child tables: 1 (Use \d+ to list them.)
a | integer | | |
b | integer | | | generated always as (a * 2)
Inherits: gtest30
+Number of child tables: 1 (Use \d+ to list them.)
+
+\d gtest30_1_1
+ Table "generated_virtual_tests.gtest30_1_1"
+ Column | Type | Collation | Nullable | Default
+--------+---------+-----------+----------+-----------------------------
+ a | integer | | |
+ b | integer | | | generated always as (a * 2)
+Inherits: gtest30_1
DROP TABLE gtest30 CASCADE;
-NOTICE: drop cascades to table gtest30_1
+NOTICE: drop cascades to 2 other objects
+DETAIL: drop cascades to table gtest30_1
+drop cascades to table gtest30_1_1
CREATE TABLE gtest30 (
a int,
b int GENERATED ALWAYS AS (a * 2) VIRTUAL
@@ -1260,6 +1272,7 @@ CREATE TABLE gtest30 (
CREATE TABLE gtest30_1 () INHERITS (gtest30);
ALTER TABLE ONLY gtest30 ALTER COLUMN b DROP EXPRESSION; -- error
ERROR: ALTER TABLE / DROP EXPRESSION must be applied to child tables too
+HINT: Do not specify the ONLY keyword.
\d gtest30
Table "generated_virtual_tests.gtest30"
Column | Type | Collation | Nullable | Default
diff --git a/src/test/regress/sql/generated_stored.sql b/src/test/regress/sql/generated_stored.sql
index f56fde8d4e5..3c8931c677e 100644
--- a/src/test/regress/sql/generated_stored.sql
+++ b/src/test/regress/sql/generated_stored.sql
@@ -577,9 +577,11 @@ CREATE TABLE gtest30 (
b int GENERATED ALWAYS AS (a * 2) STORED
);
CREATE TABLE gtest30_1 () INHERITS (gtest30);
+CREATE TABLE gtest30_1_1 () INHERITS (gtest30_1);
ALTER TABLE gtest30 ALTER COLUMN b DROP EXPRESSION;
\d gtest30
\d gtest30_1
+\d gtest30_1_1
DROP TABLE gtest30 CASCADE;
CREATE TABLE gtest30 (
a int,
diff --git a/src/test/regress/sql/generated_virtual.sql b/src/test/regress/sql/generated_virtual.sql
index ba19bc4c701..5696be4b36a 100644
--- a/src/test/regress/sql/generated_virtual.sql
+++ b/src/test/regress/sql/generated_virtual.sql
@@ -628,9 +628,11 @@ CREATE TABLE gtest30 (
b int GENERATED ALWAYS AS (a * 2) VIRTUAL
);
CREATE TABLE gtest30_1 () INHERITS (gtest30);
+CREATE TABLE gtest30_1_1 () INHERITS (gtest30_1);
ALTER TABLE gtest30 ALTER COLUMN b DROP EXPRESSION;
\d gtest30
\d gtest30_1
+\d gtest30_1_1
DROP TABLE gtest30 CASCADE;
CREATE TABLE gtest30 (
a int,
--
2.43.0
Hi all,
I tried to fix a bug in PostgreSQL where ALTER TABLE ... DROP EXPRESSION
fails on multi-level inheritance hierarchies.
Bug:
When a parent table has a generated column and child/grandchild tables
inherit from it, executing:
ALTER TABLE parent ALTER COLUMN d DROP EXPRESSION;
ERROR: ALTER TABLE / DROP EXPRESSION must be applied to child tables too
Fix Details:
-
Updated file: src/backend/commands/tablecmds.c
-
Function modified: ATPrepDropExpression()
-
Change: Added !recursing check to ensure proper recursion across
inheritance.
if (!recurse && !recursing &&
find_inheritance_children(RelationGetRelid(rel), lockmode))
ereport(ERROR,
errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("ALTER TABLE / DROP EXPRESSION must be applied to
child tables too"),
errhint("Do not specify the ONLY keyword."));
-
Test Query DROP TABLE IF EXISTS parent CASCADE;
CREATE TABLE parent (a int, d int GENERATED ALWAYS AS (11) STORED);
CREATE TABLE child () INHERITS (parent);
CREATE TABLE grandchild () INHERITS (child);
ALTER TABLE parent ALTER COLUMN d DROP EXPRESSION;
-
Output ALTER TABLE
On Thu, Aug 28, 2025 at 10:49 AM Kirill Reshke <reshkekirill@gmail.com>
wrote:
Show quoted text
On Thu, 28 Aug 2025 at 08:35, jian he <jian.universality@gmail.com> wrote:
That means, we don't need to change the ATPrepDropExpression function
argument for now?Sure. V3 with this attached, and I think we can move cf entry to RFC
--
Best regards,
Kirill Reshke
Attachments:
0001-Fix-ALTER-TABLE-DROP-EXPRESSION-for-inheritance-hier.patchtext/x-patch; charset=US-ASCII; name=0001-Fix-ALTER-TABLE-DROP-EXPRESSION-for-inheritance-hier.patchDownload
From b28086bee3c7d49b3703cf164813f8cbc20d6938 Mon Sep 17 00:00:00 2001
From: Lakshmi <bharatdbpg@gmail.com>
Date: Wed, 22 Oct 2025 17:15:55 +0530
Subject: [PATCH] Fix ALTER TABLE DROP EXPRESSION for inheritance hierarchy
---
src/backend/commands/tablecmds.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 5fd8b51312c..0f0c104e238 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -8732,12 +8732,12 @@ ATPrepDropExpression(Relation rel, AlterTableCmd *cmd, bool recurse, bool recurs
* tables, somewhat similar to how DROP COLUMN does it, so that the
* resulting state can be properly dumped and restored.
*/
- if (!recurse &&
- find_inheritance_children(RelationGetRelid(rel), lockmode))
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("ALTER TABLE / DROP EXPRESSION must be applied to child tables too")));
-
+ if (!recurse && !recursing &&
+ find_inheritance_children(RelationGetRelid(rel), lockmode))
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("ALTER TABLE / DROP EXPRESSION must be applied to child tables too"),
+ errhint("Do not specify the ONLY keyword.")));
/*
* Cannot drop generation expression from inherited columns.
*/
--
2.39.5
On 25.08.25 15:04, Kirill Reshke wrote:
So we need to detect if the user did ALTER TABLE or ALTER TABLE ONLY.
And we have two parameters passed to ATPrepDropExpression: "recurse"
and "recursing".
First is about whether the user specified ONLY option and second is
about if we are recursing in our AT code. So maybe fix it as in
attached?
I find that tablecmds.c uses these two arguments in not entirely
consistent ways.
I would have expected that if you write a command that is supposed to
recurse (no ONLY) and you are some levels down into the recursing, then
recursing=true, of course, but shouldn't recurse=true as well, to
reflect the command that was written?
Some code works like that, for example ATExecDropColumn() and
ATExecSetNotNull(). I probably originally adapted code from places like
that.
But in ATPrepDropExpression(), when you're recursing, then recurse is
always false. That is hardcoded in the ATPrepCmd() call in
ATSimpleRecursion(). Does that make sense?
I mean, there might be complex reasons, ALTER TABLE code is complicated,
but I don't find this explained anywhere.
If this worked more consistently, then the DROP EXPRESSION code might
actually work correctly as written.
Peter Eisentraut <peter@eisentraut.org> writes:
I find that tablecmds.c uses these two arguments in not entirely
consistent ways.
I would have expected that if you write a command that is supposed to
recurse (no ONLY) and you are some levels down into the recursing, then
recursing=true, of course, but shouldn't recurse=true as well, to
reflect the command that was written?
I think the intent was that
(1) recurse = true is an instruction to recurse down to any child
tables that may exist;
(2) recursing = true is a status flag saying we're already not at
the topmost parent table.
There is no situation where we'd recurse but only for a limited
number of levels, so I can't believe that recurse = false with
recursing = true is a valid state.
But in ATPrepDropExpression(), when you're recursing, then recurse is
always false. That is hardcoded in the ATPrepCmd() call in
ATSimpleRecursion(). Does that make sense?
Seems wrong, but I didn't trace through the code.
regards, tom lane
I wrote:
Peter Eisentraut <peter@eisentraut.org> writes:
But in ATPrepDropExpression(), when you're recursing, then recurse is
always false. That is hardcoded in the ATPrepCmd() call in
ATSimpleRecursion(). Does that make sense?
Seems wrong, but I didn't trace through the code.
Oh: looking closer, the reason is that ATSimpleRecursion already
located all the direct and indirect child tables and will call
ATPrepCmd on each one. Therefore it's correct that ATPrepCmd should
be told recurse = false; we do not want it to look for child tables.
You could argue that passing recursing = true for each rel is bogus,
and we should arrange to pass recursing = false for the original
table and true only for whatever children we found. But I'm not
sure that anything would care. That doesn't sound like it would
help for the current problem, anyway.
If it actually matters for DROP EXPRESSION, then the answer is
probably "we can't use ATSimpleRecursion for DROP EXPRESSION".
ATSimpleRecursion is meant for cases where each table can be
processed independently, regardless of its position in the
hierarchy.
regards, tom lane
On Wed, Nov 5, 2025 at 2:31 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
If it actually matters for DROP EXPRESSION, then the answer is
probably "we can't use ATSimpleRecursion for DROP EXPRESSION".
ATSimpleRecursion is meant for cases where each table can be
processed independently, regardless of its position in the
hierarchy.
ATPrepAlterColumnType will call ATPrepCmd, which will call again
ATPrepAlterColumnType.
Similarly, we can remove ATSimpleRecursion, and let
ATPrepDropExpression call ATPrepCmd
but that will just be the duplication of ATSimpleRecursion, i think.
/*
* Recurse manually by queueing a new command for each child, if
* necessary. We cannot apply ATSimpleRecursion here because we need to
* remap attribute numbers in the USING expression, if any.
ATPrepAlterColumnType has the above comments.
but here, we don't need to do anything between "rel" and "parent_rel".
ATPrepDropExpression logic is quite simple, it only needs to check the
a. the original source relation is species ONLY or not.
b. the original source relation is inherited column or not.
ALTER TABLE ONLY parent ALTER COLUMN d DROP EXPRESSION;
it will skip ATSimpleRecursion, because first time recurse is
false.(keyword ONLY specified)
the first time enter ATPrepDropExpression, both "recurse" and
"recursing" is false.
if (!recurse && !recursing &&
find_inheritance_children(RelationGetRelid(rel), lockmode))
ereport(ERROR,
errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("ALTER TABLE / DROP EXPRESSION must be applied
to child tables too"),
errhint("Do not specify the ONLY keyword."));
so the above code makes sense to me.
Because we only need to do this check once.
Summary:
1. We need ATSimpleRecursion so that AlteredTableInfo structures for child
relations are populated too, so generated expressions are dropped from all
inherited tables.
2. ATPrepDropExpression only checks the original table specified in the ALTER
TABLE command, and does not apply the check to its child relations.
(for example: ALTER TABLE parent ALTER COLUMN d DROP EXPRESSION;
ATPrepDropExpression check only applies to "parent" not its child relation).
On Tue, 11 Nov 2025 at 11:43, jian he <jian.universality@gmail.com> wrote:
On Wed, Nov 5, 2025 at 2:31 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
If it actually matters for DROP EXPRESSION, then the answer is
probably "we can't use ATSimpleRecursion for DROP EXPRESSION".
ATSimpleRecursion is meant for cases where each table can be
processed independently, regardless of its position in the
hierarchy.ATPrepAlterColumnType will call ATPrepCmd, which will call again
ATPrepAlterColumnType.
Similarly, we can remove ATSimpleRecursion, and let
ATPrepDropExpression call ATPrepCmd
but that will just be the duplication of ATSimpleRecursion, i think./*
* Recurse manually by queueing a new command for each child, if
* necessary. We cannot apply ATSimpleRecursion here because we need to
* remap attribute numbers in the USING expression, if any.
ATPrepAlterColumnType has the above comments.
but here, we don't need to do anything between "rel" and "parent_rel".ATPrepDropExpression logic is quite simple, it only needs to check the
a. the original source relation is species ONLY or not.
b. the original source relation is inherited column or not.ALTER TABLE ONLY parent ALTER COLUMN d DROP EXPRESSION;
it will skip ATSimpleRecursion, because first time recurse is
false.(keyword ONLY specified)
the first time enter ATPrepDropExpression, both "recurse" and
"recursing" is false.if (!recurse && !recursing &&
find_inheritance_children(RelationGetRelid(rel), lockmode))
ereport(ERROR,
errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("ALTER TABLE / DROP EXPRESSION must be applied
to child tables too"),
errhint("Do not specify the ONLY keyword."));so the above code makes sense to me.
Because we only need to do this check once.Summary:
1. We need ATSimpleRecursion so that AlteredTableInfo structures for child
relations are populated too, so generated expressions are dropped from all
inherited tables.2. ATPrepDropExpression only checks the original table specified in the ALTER
TABLE command, and does not apply the check to its child relations.
(for example: ALTER TABLE parent ALTER COLUMN d DROP EXPRESSION;
ATPrepDropExpression check only applies to "parent" not its child relation).
Hi!
I did take another look at this thread. I agree this "recurse and
recursing" logic is a little confusing.
Anyway, are you saying that v3 from this thread is a fix you are OK with?
--
Best regards,
Kirill Reshke
On Tue, Dec 30, 2025 at 4:56 AM Kirill Reshke <reshkekirill@gmail.com> wrote:
Hi!
I did take another look at this thread. I agree this "recurse and
recursing" logic is a little confusing.
Anyway, are you saying that v3 from this thread is a fix you are OK with?
Yes.
Maybe we can do something in ATSimpleRecursion.
but ATSimpleRecursion is very generic. adding some ad-hoc code for
AT_DropExpression seems not ideal.
In ATPrepDropExpression
```
if (!recurse && !recursing &&
find_inheritance_children(RelationGetRelid(rel), lockmode))
ereport(ERROR,
errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("ALTER TABLE / DROP EXPRESSION must be applied
to child tables too"),
errhint("Do not specify the ONLY keyword."));
```
is correct, i think.
If ONLY is not specified:
For child relations (see ATSimpleRecursion), the code invokes
ATPrepDropExpression(rel, cmd, false, true, lockmode);
For the parent relation, it invokes
ATPrepDropExpression(rel, cmd, true, false, lockmode);
If ONLY is specified:
The ATSimpleRecursion logic is entirely skipped.
ATPrepDropExpression is invoked exactly once as
ATPrepDropExpression(rel, cmd, false, false, lockmode);