Re: Fix ALTER TABLE DROP EXPRESSION with inheritance hierarchy

On Mon, Aug 25, 2025 at 9:04 PM Kirill Reshke <reshkekirill(at)gmail(dot)com> wrote:
>
> Hi!
>
> On Sun, 24 Aug 2025 at 14:05, jian he <jian(dot)universality(at)gmail(dot)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.