Fix ALTER TABLE DROP EXPRESSION with inheritance hierarchy

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:8734

The 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

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:8734

The 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

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:

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);

--
jian
https://www.enterprisedb.com