Обсуждение: bug, ALTER TABLE call ATPostAlterTypeCleanup twice for the same relation

hi.

bug demo:
drop table if exists gtest25;
CREATE TABLE gtest25 (a0 int, a int, b int GENERATED ALWAYS AS (a * 2
+ a0) STORED);
alter table gtest25 add constraint cc check (b > 0);

alter table gtest25  alter column b set data type int8, ALTER COLUMN b
SET EXPRESSION AS (a * 3 + a0);
ERROR:  cache lookup failed for constraint 18432

ALTER COLUMN SET EXPRESSION only in 17, so it's a 17 and up related bug.
The reason is ATRewriteCatalogs->ATPostAlterTypeCleanup is called
twice for the same relation.
the second time you call it, the constraint cc is already dropped,
then the "cache lookup failed" error will happen.


While at it, maybe we can also polish the comment below in ATRewriteCatalogs.
            /*
             * After the ALTER TYPE or SET EXPRESSION pass, do cleanup work
             * (this is not done in ATExecAlterColumnType since it should be
             * done only once if multiple columns of a table are altered).
             */
but I didn't do it...

On Sat, Sep 27, 2025 at 5:54 PM jian he <jian.universality@gmail.com> wrote:
>
> While at it, maybe we can also polish the comment below in ATRewriteCatalogs.
>             /*
>              * After the ALTER TYPE or SET EXPRESSION pass, do cleanup work
>              * (this is not done in ATExecAlterColumnType since it should be
>              * done only once if multiple columns of a table are altered).
>              */
> but I didn't do it...

I add one sentence:
Note: ATPostAlterTypeCleanup must be called only once per relation!

I also polished the regress tests.

On Sat, 27 Sept 2025 at 21:54, jian he <jian.universality@gmail.com> wrote:
>  if (pass == AT_PASS_ALTER_TYPE || pass == AT_PASS_SET_EXPRESSION)
> - ATPostAlterTypeCleanup(wqueue, tab, lockmode);
> + {
> + if (!list_member_oid(relids, tab->relid))
> + {
> + ATPostAlterTypeCleanup(wqueue, tab, lockmode);
> + relids = lappend_oid(relids, tab->relid);
> + }
> + }

I've not studied this long enough to know what the correct fix should
be, but I have studied it long enough to know what you're proposing
isn't it.

You can't just forego the subsequent call to ATPostAlterTypeCleanup()
because you've done it during the AT_PASS_ALTER_TYPE pass as that
doesn't account for the fact that there might be more work to do (i.e
more constraints added since AT_PASS_ALTER_TYPE) in the
AT_PASS_SET_EXPRESSION pass.

With your patch applied, if I adapt your example case to alter the
type of an unrelated column so that the constraint related to that
column is adjusted first and the constraint related to the expression
column is only added to the changedConstraintOids list after the first
call to ATPostAlterTypeCleanup(), you'll see that "cc" isn't recreated
because your code thinks nothing further needs to be done:

drop table if exists gtest25;
CREATE TABLE gtest25 (a0 int, z int, a int, b int GENERATED ALWAYS AS
(a * 2 + a0) STORED);
alter table gtest25 add constraint check_z check (z > 0);
alter table gtest25 add constraint cc check (b > 0);

select oid, conname,conbin from pg_constraint where
conrelid='gtest25'::regclass;

alter table gtest25  alter column z set data type numeric, ALTER
COLUMN b SET EXPRESSION AS (z + 0);
select oid, conname,conbin from pg_constraint where
conrelid='gtest25'::regclass;

and that's certainly wrong as if I separate the ALTER TABLE into two
separate commands, then "cc" does get recreated.

I do wonder what the exact reason was that AT_PASS_ALTER_TYPE and
AT_PASS_SET_EXPRESSION are separate passes. I'd have expected that's
maybe so that it's possible to alter the type of a column used within
a generated column, but looking at the following error message makes
me think I might not be correct in that thinking:

create table test1 (a int, b int generated always as (a + 1) stored);
alter table test1 alter column a set data type bigint;
ERROR:  cannot alter type of a column used by a generated column
DETAIL:  Column "a" is used by generated column "b".

There's probably some good explanation for the separate pass, but I'm
not sure of it for now. I'm including in the author and committer of
the patch which added this code to see if we can figure this out.

David

On Mon, Sep 29, 2025 at 5:36 AM David Rowley <dgrowleyml@gmail.com> wrote:
>
> I do wonder what the exact reason was that AT_PASS_ALTER_TYPE and
> AT_PASS_SET_EXPRESSION are separate passes. I'd have expected that's
> maybe so that it's possible to alter the type of a column used within
> a generated column, but looking at the following error message makes
> me think I might not be correct in that thinking:
>
> create table test1 (a int, b int generated always as (a + 1) stored);
> alter table test1 alter column a set data type bigint;
> ERROR:  cannot alter type of a column used by a generated column
> DETAIL:  Column "a" is used by generated column "b".
>
> There's probably some good explanation for the separate pass, but I'm
> not sure of it for now. I'm including in the author and committer of
> the patch which added this code to see if we can figure this out.
>

I found the explanation link:
https://www.postgresql.org/message-id/CAAJ_b96TYsRrqm%2BveXCBb6CJuHJh0opmAvnhw8iMvhCMFKO-Tg%40mail.gmail.com

AT_PASS_SET_EXPRESSION should come after AT_PASS_ADD_COL,
otherwise cases like below would fail.
create table t1 (a int, b int generated always as (a + 1) stored);
alter table t1 add column c int, alter column b set expression as (a + c);

 >AT_PASS_ALTER_TYPE and AT_PASS_ADD_COL cannot be together, the ALTER TYPE
> cannot see that column.
I guess this is the reason why we have two seperate PASS.


please also check the attached patch.
The idea is that if both generation expression and type are being changed,
only call ATPostAlterTypeCleanup while the current pass is
AT_PASS_SET_EXPRESSION.

On Mon, Sep 29, 2025 at 5:21 PM Chao Li <li.evan.chao@gmail.com> wrote:
>
> Hi Jian,
>
> On Mon, Sep 29, 2025 at 3:43 PM jian he <jian.universality@gmail.com> wrote:
>>
>>
>> please also check the attached patch.
>> The idea is that if both generation expression and type are being changed,
>> only call ATPostAlterTypeCleanup while the current pass is
>> AT_PASS_SET_EXPRESSION.
>
>
> I think your implementation is similar to my previous dirty fix, the main idea is to postpone the cleanup to after
AT_PASS_SET_EXPRESSION.
>
> But I feel we don't need an extra loop to find tabs that have both ALTER TYPE and SET EXPRESSION, and do the if-else
check,the logic is a bit difficult to understand. 
>
> I am attaching my version and please see if you like it. My version just records tabs to cleanup in an array, then
runsthe cleanup after the AT_PASS_SET_EXPRESSION pass. 
>
hi.

we can simply change from

            if (pass == AT_PASS_ALTER_TYPE || pass == AT_PASS_SET_EXPRESSION)
                ATPostAlterTypeCleanup(wqueue, tab, lockmode);

to
            if (pass == AT_PASS_SET_EXPRESSION)
                ATPostAlterTypeCleanup(wqueue, tab, lockmode);
            else if (pass == AT_PASS_ALTER_TYPE &&
                     tab->subcmds[AT_PASS_SET_EXPRESSION] == NIL)
                ATPostAlterTypeCleanup(wqueue, tab, lockmode);