Обсуждение: Fix ALTER TABLE DROP EXPRESSION with inheritance hierarchy

Поиск
Список
Период
Сортировка

Fix ALTER TABLE DROP EXPRESSION with inheritance hierarchy

От
jian he
Дата:
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.
Вложения

Re: Fix ALTER TABLE DROP EXPRESSION with inheritance hierarchy

От
Kirill Reshke
Дата:
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
Вложения

Re: Fix ALTER TABLE DROP EXPRESSION with inheritance hierarchy

От
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

Re: Fix ALTER TABLE DROP EXPRESSION with inheritance hierarchy

От
jian he
Дата:
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.

Re: Fix ALTER TABLE DROP EXPRESSION with inheritance hierarchy

От
Kirill Reshke
Дата:
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
Вложения