Обсуждение: 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.

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] 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

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