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...
Attachments:
v1-0001-ALTER-TABLE-each-relation-call-ATPostAlterTypeCleanup-only-once.patchtext/x-patch; charset=US-ASCII; name=v1-0001-ALTER-TABLE-each-relation-call-ATPostAlterTypeCleanup-only-once.patchDownload
From 452f38f84fb61b08e200c7bd0d96663701df299c Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
Date: Sat, 27 Sep 2025 17:46:45 +0800
Subject: [PATCH v1 1/1] ALTER TABLE each relation call ATPostAlterTypeCleanup
only once
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
The reason it can only call once is:
ATPostAlterTypeCleanup searches the system catalog for recreate objects such as
STATISTICS, CONSTRAINTS. After that, the original object will be dropped via
performMultipleDeletions.
If ATPostAlterTypeCleanup is called a second time, the catalog cache lookup will
fail because those objects were already removed during the first call.
discussion: https://postgr.es/m/
---
src/backend/commands/tablecmds.c | 9 ++++++++-
src/test/regress/expected/generated_stored.out | 2 ++
src/test/regress/expected/generated_virtual.out | 2 ++
src/test/regress/sql/generated_stored.sql | 2 ++
src/test/regress/sql/generated_virtual.sql | 2 ++
5 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3be2e051d32..39b32af4b7e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5296,6 +5296,7 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
AlterTableUtilityContext *context)
{
ListCell *ltab;
+ List *relids = NIL;
/*
* We process all the tables "in parallel", one pass at a time. This is
@@ -5334,7 +5335,13 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
* done only once if multiple columns of a table are altered).
*/
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);
+ }
+ }
if (tab->rel)
{
diff --git a/src/test/regress/expected/generated_stored.out b/src/test/regress/expected/generated_stored.out
index adac2cedfb2..b0bc33576ca 100644
--- a/src/test/regress/expected/generated_stored.out
+++ b/src/test/regress/expected/generated_stored.out
@@ -1120,6 +1120,8 @@ SELECT * FROM gtest25 ORDER BY a;
Indexes:
"gtest25_pkey" PRIMARY KEY, btree (a)
+ALTER TABLE gtest25 ADD CONSTRAINT cc CHECK(b > 9) NOT VALID;
+ALTER TABLE gtest25 ALTER COLUMN b SET DATA TYPE INT8, ALTER COLUMN b SET EXPRESSION AS (a * 3); --ok
-- ALTER TABLE ... ALTER COLUMN
CREATE TABLE gtest27 (
a int,
diff --git a/src/test/regress/expected/generated_virtual.out b/src/test/regress/expected/generated_virtual.out
index d8645192351..1417369213c 100644
--- a/src/test/regress/expected/generated_virtual.out
+++ b/src/test/regress/expected/generated_virtual.out
@@ -1082,6 +1082,8 @@ SELECT * FROM gtest25 ORDER BY a;
Indexes:
"gtest25_pkey" PRIMARY KEY, btree (a)
+ALTER TABLE gtest25 ADD CONSTRAINT cc CHECK(b > 9) NOT VALID;
+ALTER TABLE gtest25 ALTER COLUMN b SET DATA TYPE INT8, ALTER COLUMN b SET EXPRESSION AS (a * 3); --ok
-- ALTER TABLE ... ALTER COLUMN
CREATE TABLE gtest27 (
a int,
diff --git a/src/test/regress/sql/generated_stored.sql b/src/test/regress/sql/generated_stored.sql
index f56fde8d4e5..e0c9ac32158 100644
--- a/src/test/regress/sql/generated_stored.sql
+++ b/src/test/regress/sql/generated_stored.sql
@@ -516,6 +516,8 @@ ALTER TABLE gtest25 ALTER COLUMN d SET DATA TYPE float8,
ADD COLUMN y float8 GENERATED ALWAYS AS (d * 4) STORED;
SELECT * FROM gtest25 ORDER BY a;
\d gtest25
+ALTER TABLE gtest25 ADD CONSTRAINT cc CHECK(b > 9) NOT VALID;
+ALTER TABLE gtest25 ALTER COLUMN b SET DATA TYPE INT8, ALTER COLUMN b SET EXPRESSION AS (a * 3); --ok
-- ALTER TABLE ... ALTER COLUMN
CREATE TABLE gtest27 (
diff --git a/src/test/regress/sql/generated_virtual.sql b/src/test/regress/sql/generated_virtual.sql
index adfe88d74ae..b5d785734f4 100644
--- a/src/test/regress/sql/generated_virtual.sql
+++ b/src/test/regress/sql/generated_virtual.sql
@@ -559,6 +559,8 @@ ALTER TABLE gtest25 ALTER COLUMN d SET DATA TYPE float8,
ADD COLUMN y float8 GENERATED ALWAYS AS (d * 4) VIRTUAL;
SELECT * FROM gtest25 ORDER BY a;
\d gtest25
+ALTER TABLE gtest25 ADD CONSTRAINT cc CHECK(b > 9) NOT VALID;
+ALTER TABLE gtest25 ALTER COLUMN b SET DATA TYPE INT8, ALTER COLUMN b SET EXPRESSION AS (a * 3); --ok
-- ALTER TABLE ... ALTER COLUMN
CREATE TABLE gtest27 (
--
2.34.1
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.
Attachments:
v2-0001-ALTER-TABLE-each-relation-call-ATPostAlterTypeCleanup.patchtext/x-patch; charset=US-ASCII; name=v2-0001-ALTER-TABLE-each-relation-call-ATPostAlterTypeCleanup.patchDownload
From a750440b9405a0fc4122583f11735295fe97e1e6 Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
Date: Sat, 27 Sep 2025 22:17:04 +0800
Subject: [PATCH v2 1/1] ALTER TABLE each relation call ATPostAlterTypeCleanup
only once
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
The reason it can only call once is:
ATPostAlterTypeCleanup searches the system catalog for recreate objects such as
STATISTICS, CONSTRAINTS. After that, the original object will be dropped via
performMultipleDeletions.
If ATPostAlterTypeCleanup is called a second time, the catalog cache lookup will
fail because those objects were already removed during the first call.
discussion: https://postgr.es/m/CACJufxHZsgn3zM5g-x7YmtFGzNDnRwR07S+GYfiUs+tZ45MDDw@mail.gmail.com
---
src/backend/commands/tablecmds.c | 10 +++++++++-
src/test/regress/expected/generated_stored.out | 9 +++++++++
src/test/regress/expected/generated_virtual.out | 9 +++++++++
src/test/regress/sql/generated_stored.sql | 3 +++
src/test/regress/sql/generated_virtual.sql | 3 +++
5 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 3be2e051d32..cd5d1ca6251 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5296,6 +5296,7 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
AlterTableUtilityContext *context)
{
ListCell *ltab;
+ List *relids = NIL;
/*
* We process all the tables "in parallel", one pass at a time. This is
@@ -5332,9 +5333,16 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
* 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).
+ * Note: ATPostAlterTypeCleanup must be called only once per relation!
*/
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);
+ }
+ }
if (tab->rel)
{
diff --git a/src/test/regress/expected/generated_stored.out b/src/test/regress/expected/generated_stored.out
index adac2cedfb2..cd1685fd90f 100644
--- a/src/test/regress/expected/generated_stored.out
+++ b/src/test/regress/expected/generated_stored.out
@@ -1120,6 +1120,15 @@ SELECT * FROM gtest25 ORDER BY a;
Indexes:
"gtest25_pkey" PRIMARY KEY, btree (a)
+ALTER TABLE gtest25 ADD CONSTRAINT cc CHECK(b < 9) NOT VALID;
+ALTER TABLE gtest25 ALTER COLUMN b SET DATA TYPE INT8, ALTER COLUMN b SET EXPRESSION AS (a * 4); --ok
+SELECT * FROM gtest25 ORDER BY a;
+ a | b | c | x | d | y
+---+----+----+-----+-----+-----
+ 3 | 12 | 42 | 168 | 101 | 404
+ 4 | 16 | 42 | 168 | 101 | 404
+(2 rows)
+
-- ALTER TABLE ... ALTER COLUMN
CREATE TABLE gtest27 (
a int,
diff --git a/src/test/regress/expected/generated_virtual.out b/src/test/regress/expected/generated_virtual.out
index d8645192351..db34851b041 100644
--- a/src/test/regress/expected/generated_virtual.out
+++ b/src/test/regress/expected/generated_virtual.out
@@ -1082,6 +1082,15 @@ SELECT * FROM gtest25 ORDER BY a;
Indexes:
"gtest25_pkey" PRIMARY KEY, btree (a)
+ALTER TABLE gtest25 ADD CONSTRAINT cc CHECK(b < 9) NOT VALID;
+ALTER TABLE gtest25 ALTER COLUMN b SET DATA TYPE INT8, ALTER COLUMN b SET EXPRESSION AS (a * 4); --ok
+SELECT * FROM gtest25 ORDER BY a;
+ a | b | c | x | d | y
+---+----+----+-----+-----+-----
+ 3 | 12 | 42 | 168 | 101 | 404
+ 4 | 16 | 42 | 168 | 101 | 404
+(2 rows)
+
-- ALTER TABLE ... ALTER COLUMN
CREATE TABLE gtest27 (
a int,
diff --git a/src/test/regress/sql/generated_stored.sql b/src/test/regress/sql/generated_stored.sql
index f56fde8d4e5..d4066cc9056 100644
--- a/src/test/regress/sql/generated_stored.sql
+++ b/src/test/regress/sql/generated_stored.sql
@@ -516,6 +516,9 @@ ALTER TABLE gtest25 ALTER COLUMN d SET DATA TYPE float8,
ADD COLUMN y float8 GENERATED ALWAYS AS (d * 4) STORED;
SELECT * FROM gtest25 ORDER BY a;
\d gtest25
+ALTER TABLE gtest25 ADD CONSTRAINT cc CHECK(b < 9) NOT VALID;
+ALTER TABLE gtest25 ALTER COLUMN b SET DATA TYPE INT8, ALTER COLUMN b SET EXPRESSION AS (a * 4); --ok
+SELECT * FROM gtest25 ORDER BY a;
-- ALTER TABLE ... ALTER COLUMN
CREATE TABLE gtest27 (
diff --git a/src/test/regress/sql/generated_virtual.sql b/src/test/regress/sql/generated_virtual.sql
index adfe88d74ae..250d3dd6a49 100644
--- a/src/test/regress/sql/generated_virtual.sql
+++ b/src/test/regress/sql/generated_virtual.sql
@@ -559,6 +559,9 @@ ALTER TABLE gtest25 ALTER COLUMN d SET DATA TYPE float8,
ADD COLUMN y float8 GENERATED ALWAYS AS (d * 4) VIRTUAL;
SELECT * FROM gtest25 ORDER BY a;
\d gtest25
+ALTER TABLE gtest25 ADD CONSTRAINT cc CHECK(b < 9) NOT VALID;
+ALTER TABLE gtest25 ALTER COLUMN b SET DATA TYPE INT8, ALTER COLUMN b SET EXPRESSION AS (a * 4); --ok
+SELECT * FROM gtest25 ORDER BY a;
-- ALTER TABLE ... ALTER COLUMN
CREATE TABLE gtest27 (
--
2.34.1
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 Sep 29, 2025, at 05:36, David Rowley <dgrowleyml@gmail.com> wrote:
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); + } + }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.
Based on the comment of ATPostAlterTypeCleanup(), it says “cleanup after we’ve finished all the ALTER TYPE or SET EXPRESSION operations for a particular relation”, I tried this dirty fix:
```
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index fc89352b661..5c9c6d2a7db 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5296,6 +5296,8 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
AlterTableUtilityContext *context)
{
ListCell *ltab;
+ bool needAlterTypeCleanup = false;
+ AlteredTableInfo *tabToCleanup = NULL; /*
* We process all the tables "in parallel", one pass at a time. This is
@@ -5313,6 +5315,13 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
List *subcmds = tab->subcmds[pass];
ListCell *lcmd;
+ if (pass == AT_PASS_OLD_INDEX && needAlterTypeCleanup)
+ {
+ ATPostAlterTypeCleanup(wqueue, tabToCleanup, lockmode);
+ needAlterTypeCleanup = false;
+ tabToCleanup = NULL;
+ }
+
if (subcmds == NIL)
continue;@@ -5334,7 +5343,10 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
* done only once if multiple columns of a table are altered).
*/
if (pass == AT_PASS_ALTER_TYPE || pass == AT_PASS_SET_EXPRESSION)
- ATPostAlterTypeCleanup(wqueue, tab, lockmode);
+ {
+ needAlterTypeCleanup = true;
+ tabToCleanup = tab;
+ } if (tab->rel)
{
```
It postpones the cleanup to after all “alter type” and “set expression”. With this fix, David’s test case also passes:
```
evantest=# select oid, conname,conbin from pg_constraint where
evantest-# conrelid='gtest25'::regclass;
oid | conname | conbin
-------+---------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
32778 | check_z | {OPEXPR :opno 521 :opfuncid 147 :opresulttype 16 :opretset false :opcollid 0 :inputcollid 0 :args ({VAR :varno 1 :varattno 2 :vartype 23 :vartypmod -1 :varcollid 0 :varnullingrels (b) :varlevelsup 0 :varreturningtype 0 :varnosyn 1 :varattnosyn 2 :location -1} {CONST :consttype 23 :consttypmod -1 :constcollid 0 :constlen 4 :constbyval true :constisnull false :location -1 :constvalue 4 [ 0 0 0 0 0 0 0 0 ]}) :location -1}
32779 | cc | {OPEXPR :opno 521 :opfuncid 147 :opresulttype 16 :opretset false :opcollid 0 :inputcollid 0 :args ({VAR :varno 1 :varattno 4 :vartype 23 :vartypmod -1 :varcollid 0 :varnullingrels (b) :varlevelsup 0 :varreturningtype 0 :varnosyn 1 :varattnosyn 4 :location -1} {CONST :consttype 23 :consttypmod -1 :constcollid 0 :constlen 4 :constbyval true :constisnull false :location -1 :constvalue 4 [ 0 0 0 0 0 0 0 0 ]}) :location -1}
(2 rows)
evantest=# alter table gtest25 alter column z set data type numeric, ALTER
evantest-# COLUMN b SET EXPRESSION AS (z + 0);
ALTER TABLE
evantest=# select oid, conname,conbin from pg_constraint where
evantest-# conrelid='gtest25'::regclass;
oid | conname | conbin
-------+---------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
32781 | check_z | {OPEXPR :opno 1756 :opfuncid 1720 :opresulttype 16 :opretset false :opcollid 0 :inputcollid 0 :args ({VAR :varno 1 :varattno 2 :vartype 1700 :vartypmod -1 :varcollid 0 :varnullingrels (b) :varlevelsup 0 :varreturningtype 0 :varnosyn 1 :varattnosyn 2 :location -1} {FUNCEXPR :funcid 1740 :funcresulttype 1700 :funcretset false :funcvariadic false :funcformat 2 :funccollid 0 :inputcollid 0 :args ({CONST :consttype 23 :consttypmod -1 :constcollid 0 :constlen 4 :constbyval true :constisnull false :location -1 :constvalue 4 [ 0 0 0 0 0 0 0 0 ]}) :location -1}) :location -1}
32782 | cc | {OPEXPR :opno 521 :opfuncid 147 :opresulttype 16 :opretset false :opcollid 0 :inputcollid 0 :args ({VAR :varno 1 :varattno 4 :vartype 23 :vartypmod -1 :varcollid 0 :varnullingrels (b) :varlevelsup 0 :varreturningtype 0 :varnosyn 1 :varattnosyn 4 :location -1} {CONST :consttype 23 :consttypmod -1 :constcollid 0 :constlen 4 :constbyval true :constisnull false :location -1 :constvalue 4 [ 0 0 0 0 0 0 0 0 ]}) :location -1}
(2 rows)
evantest=#
```
We can see both check_z and check_cc got rebuilt.
This fix uses the last “tab” with the cleanup function. In that way, TBH, I haven’t dig deeper enough to see if anything is missed in cleanup.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
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:
/messages/by-id/CAAJ_b96TYsRrqm+veXCBb6CJuHJh0opmAvnhw8iMvhCMFKO-Tg@mail.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.
Attachments:
v3-0001-avoid-call-ATPostAlterTypeCleanup-twice.patchtext/x-patch; charset=US-ASCII; name=v3-0001-avoid-call-ATPostAlterTypeCleanup-twice.patchDownload
From 44966ed78830d31ce6e3d64bf387dc380d12982e Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
Date: Mon, 29 Sep 2025 15:39:54 +0800
Subject: [PATCH v3 1/1] avoid call ATPostAlterTypeCleanup twice
discussion: https://postgr.es/m/CACJufxHZsgn3zM5g-x7YmtFGzNDnRwR07S+GYfiUs+tZ45MDDw@mail.gmail.com
---
src/backend/commands/tablecmds.c | 27 ++++++++++++++++++-
.../regress/expected/generated_stored.out | 12 +++++++++
.../regress/expected/generated_virtual.out | 12 +++++++++
src/test/regress/sql/generated_stored.sql | 6 +++++
src/test/regress/sql/generated_virtual.sql | 6 +++++
5 files changed, 62 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index fc89352b661..4443b69c156 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5296,6 +5296,20 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
AlterTableUtilityContext *context)
{
ListCell *ltab;
+ List *relids = NIL;
+
+ /*
+ * Collect relation that both type and generation expression are changed.
+ */
+ foreach(ltab, *wqueue)
+ {
+ AlteredTableInfo *tab = (AlteredTableInfo *) lfirst(ltab);
+ List *subcmds1 = tab->subcmds[AT_PASS_SET_EXPRESSION];
+ List *subcmds2 = tab->subcmds[AT_PASS_ALTER_TYPE];
+
+ if (subcmds1 != NIL && subcmds2 != NIL)
+ relids = lappend_oid(relids, tab->relid);
+ }
/*
* We process all the tables "in parallel", one pass at a time. This is
@@ -5333,7 +5347,16 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
* (this is not done in ATExecAlterColumnType since it should be
* done only once if multiple columns of a table are altered).
*/
- if (pass == AT_PASS_ALTER_TYPE || pass == AT_PASS_SET_EXPRESSION)
+ if (list_member_oid(relids, tab->relid))
+ {
+ /*
+ * If both type and generation expression being changed, do the
+ * cleanup at AT_PASS_SET_EXPRESSION
+ */
+ if (pass == AT_PASS_SET_EXPRESSION)
+ ATPostAlterTypeCleanup(wqueue, tab, lockmode);
+ }
+ else if (pass == AT_PASS_ALTER_TYPE || pass == AT_PASS_SET_EXPRESSION)
ATPostAlterTypeCleanup(wqueue, tab, lockmode);
if (tab->rel)
@@ -15578,6 +15601,8 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
free_object_addresses(objects);
+
+
/*
* The objects will get recreated during subsequent passes over the work
* queue.
diff --git a/src/test/regress/expected/generated_stored.out b/src/test/regress/expected/generated_stored.out
index adac2cedfb2..cf9c2836daa 100644
--- a/src/test/regress/expected/generated_stored.out
+++ b/src/test/regress/expected/generated_stored.out
@@ -1120,6 +1120,18 @@ SELECT * FROM gtest25 ORDER BY a;
Indexes:
"gtest25_pkey" PRIMARY KEY, btree (a)
+ALTER TABLE gtest25 ADD COLUMN z INT DEFAULT 11;
+ALTER TABLE gtest25 ADD CONSTRAINT cc CHECK(b > 9) NOT VALID;
+ALTER TABLE gtest25 ADD CONSTRAINT check_z CHECK(z > 9) NOT VALID;
+ALTER TABLE gtest25 ALTER COLUMN z SET DATA TYPE numeric, ALTER COLUMN b SET EXPRESSION AS (z + 0);
+ALTER TABLE gtest25 ALTER COLUMN b SET DATA TYPE numeric, ALTER COLUMN b SET EXPRESSION AS (a * 3);
+SELECT * FROM gtest25 ORDER BY a;
+ a | b | c | x | d | y | z
+---+----+----+-----+-----+-----+----
+ 3 | 9 | 42 | 168 | 101 | 404 | 11
+ 4 | 12 | 42 | 168 | 101 | 404 | 11
+(2 rows)
+
-- ALTER TABLE ... ALTER COLUMN
CREATE TABLE gtest27 (
a int,
diff --git a/src/test/regress/expected/generated_virtual.out b/src/test/regress/expected/generated_virtual.out
index c861bd36c5a..e97ea287b73 100644
--- a/src/test/regress/expected/generated_virtual.out
+++ b/src/test/regress/expected/generated_virtual.out
@@ -1082,6 +1082,18 @@ SELECT * FROM gtest25 ORDER BY a;
Indexes:
"gtest25_pkey" PRIMARY KEY, btree (a)
+ALTER TABLE gtest25 ADD COLUMN z INT DEFAULT 11;
+-- ALTER TABLE gtest25 ADD CONSTRAINT cc CHECK(b > 9) NOT VALID;
+-- ALTER TABLE gtest25 ADD CONSTRAINT check_z CHECK(z > 9) NOT VALID;
+ALTER TABLE gtest25 ALTER COLUMN z SET DATA TYPE numeric, ALTER COLUMN b SET EXPRESSION AS (z + 0);
+ALTER TABLE gtest25 ALTER COLUMN b SET DATA TYPE numeric, ALTER COLUMN b SET EXPRESSION AS (a * 3);
+SELECT * FROM gtest25 ORDER BY a;
+ a | b | c | x | d | y | z
+---+----+----+-----+-----+-----+----
+ 3 | 9 | 42 | 168 | 101 | 404 | 11
+ 4 | 12 | 42 | 168 | 101 | 404 | 11
+(2 rows)
+
-- ALTER TABLE ... ALTER COLUMN
CREATE TABLE gtest27 (
a int,
diff --git a/src/test/regress/sql/generated_stored.sql b/src/test/regress/sql/generated_stored.sql
index f56fde8d4e5..6d5359fdcee 100644
--- a/src/test/regress/sql/generated_stored.sql
+++ b/src/test/regress/sql/generated_stored.sql
@@ -516,6 +516,12 @@ ALTER TABLE gtest25 ALTER COLUMN d SET DATA TYPE float8,
ADD COLUMN y float8 GENERATED ALWAYS AS (d * 4) STORED;
SELECT * FROM gtest25 ORDER BY a;
\d gtest25
+ALTER TABLE gtest25 ADD COLUMN z INT DEFAULT 11;
+ALTER TABLE gtest25 ADD CONSTRAINT cc CHECK(b > 9) NOT VALID;
+ALTER TABLE gtest25 ADD CONSTRAINT check_z CHECK(z > 9) NOT VALID;
+ALTER TABLE gtest25 ALTER COLUMN z SET DATA TYPE numeric, ALTER COLUMN b SET EXPRESSION AS (z + 0);
+ALTER TABLE gtest25 ALTER COLUMN b SET DATA TYPE numeric, ALTER COLUMN b SET EXPRESSION AS (a * 3);
+SELECT * FROM gtest25 ORDER BY a;
-- ALTER TABLE ... ALTER COLUMN
CREATE TABLE gtest27 (
diff --git a/src/test/regress/sql/generated_virtual.sql b/src/test/regress/sql/generated_virtual.sql
index adfe88d74ae..9a94a2e311c 100644
--- a/src/test/regress/sql/generated_virtual.sql
+++ b/src/test/regress/sql/generated_virtual.sql
@@ -559,6 +559,12 @@ ALTER TABLE gtest25 ALTER COLUMN d SET DATA TYPE float8,
ADD COLUMN y float8 GENERATED ALWAYS AS (d * 4) VIRTUAL;
SELECT * FROM gtest25 ORDER BY a;
\d gtest25
+ALTER TABLE gtest25 ADD COLUMN z INT DEFAULT 11;
+-- ALTER TABLE gtest25 ADD CONSTRAINT cc CHECK(b > 9) NOT VALID;
+-- ALTER TABLE gtest25 ADD CONSTRAINT check_z CHECK(z > 9) NOT VALID;
+ALTER TABLE gtest25 ALTER COLUMN z SET DATA TYPE numeric, ALTER COLUMN b SET EXPRESSION AS (z + 0);
+ALTER TABLE gtest25 ALTER COLUMN b SET DATA TYPE numeric, ALTER COLUMN b SET EXPRESSION AS (a * 3);
+SELECT * FROM gtest25 ORDER BY a;
-- ALTER TABLE ... ALTER COLUMN
CREATE TABLE gtest27 (
--
2.34.1
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 runs the cleanup after the
AT_PASS_SET_EXPRESSION pass.
A nit comment is:
```
@@ -15578,6 +15601,8 @@ ATPostAlterTypeCleanup(List **wqueue,
AlteredTableInfo *tab, LOCKMODE lockmode)
free_object_addresses(objects);
+
+
```
Why add two newlines here? Seems a typo.Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
v4-0002-The-other-implementation.patchapplication/octet-stream; name=v4-0002-The-other-implementation.patchDownload
From 068900d2c41aa7383363b304636c1f11cd9c1f8b Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <lic@highgo.com>
Date: Mon, 29 Sep 2025 17:11:25 +0800
Subject: [PATCH v4 2/2] The other implementation.
Author: Chao Li <lic@highgo.com>
---
src/backend/commands/tablecmds.c | 47 ++++++++++++--------------------
1 file changed, 17 insertions(+), 30 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 4443b69c156..3791666834c 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5296,20 +5296,7 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
AlterTableUtilityContext *context)
{
ListCell *ltab;
- List *relids = NIL;
-
- /*
- * Collect relation that both type and generation expression are changed.
- */
- foreach(ltab, *wqueue)
- {
- AlteredTableInfo *tab = (AlteredTableInfo *) lfirst(ltab);
- List *subcmds1 = tab->subcmds[AT_PASS_SET_EXPRESSION];
- List *subcmds2 = tab->subcmds[AT_PASS_ALTER_TYPE];
-
- if (subcmds1 != NIL && subcmds2 != NIL)
- relids = lappend_oid(relids, tab->relid);
- }
+ AlteredTableInfo **tabsForAltTypeCleanup = palloc0(sizeof(AlteredTableInfo *) * list_length(*wqueue));
/*
* We process all the tables "in parallel", one pass at a time. This is
@@ -5320,6 +5307,8 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
*/
for (AlterTablePass pass = 0; pass < AT_NUM_PASSES; pass++)
{
+ int tabIdx = -1;
+
/* Go through each table that needs to be processed */
foreach(ltab, *wqueue)
{
@@ -5327,6 +5316,17 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
List *subcmds = tab->subcmds[pass];
ListCell *lcmd;
+ /*
+ * Cleanup work for ALTER TYPE or SET EXPRESSION. We must perform
+ * this cleanup after both ALTER TYPE and SET EXPRESSION are done.
+ */
+ tabIdx += 1;
+ if (pass == AT_PASS_SET_EXPRESSION+1 && tabsForAltTypeCleanup[tabIdx] != NULL)
+ {
+ ATPostAlterTypeCleanup(wqueue, tabsForAltTypeCleanup[tabIdx], lockmode);
+ tabsForAltTypeCleanup[tabIdx] = NULL;
+ }
+
if (subcmds == NIL)
continue;
@@ -5342,22 +5342,8 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
lfirst_node(AlterTableCmd, lcmd),
lockmode, pass, context);
- /*
- * 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).
- */
- if (list_member_oid(relids, tab->relid))
- {
- /*
- * If both type and generation expression being changed, do the
- * cleanup at AT_PASS_SET_EXPRESSION
- */
- if (pass == AT_PASS_SET_EXPRESSION)
- ATPostAlterTypeCleanup(wqueue, tab, lockmode);
- }
- else if (pass == AT_PASS_ALTER_TYPE || pass == AT_PASS_SET_EXPRESSION)
- ATPostAlterTypeCleanup(wqueue, tab, lockmode);
+ if (pass == AT_PASS_ALTER_TYPE || pass == AT_PASS_SET_EXPRESSION)
+ tabsForAltTypeCleanup[tabIdx] = tab;
if (tab->rel)
{
@@ -5366,6 +5352,7 @@ ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode,
}
}
}
+ pfree(tabsForAltTypeCleanup);
/* Check to see if a toast table must be added. */
foreach(ltab, *wqueue)
--
2.39.5 (Apple Git-154)
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 runs the 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);
On Wed, Oct 1, 2025 at 11:04 AM jian he <jian.universality@gmail.com> wrote:
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);
That will not work if AT_PASS_ALTER_TYPE triggers the creation of a new
AT_PASS_SET_EXPRESSION entry.
For example, consider:
CREATE TABLE gtest25 (a int, b int GENERATED ALWAYS AS (a * 2) STORED);
If we alter the type of column "a", the column b’s generation
expression would need
to be rebuilt (which is currently not supported). In that case, the current
logic would not be able to handle it.
I think the correct fix is within ATPostAlterTypeCleanup.
at the end of ATPostAlterTypeCleanup, we already delete these changed
objects via
``performMultipleDeletions(objects, DROP_RESTRICT, PERFORM_DELETION_INTERNAL);``
we can just list_free these (the below) objects too:
tab->changedConstraintOids
tab->changedConstraintDefs
tab->changedIndexOids
tab->changedIndexDefs
tab->changedStatisticsOids
tab->changedStatisticsDefs
since this information would become stale if those objects have
already been dropped.
Attachments:
v5-0001-avoid-call-ATPostAlterTypeCleanup-twice.patchtext/x-patch; charset=US-ASCII; name=v5-0001-avoid-call-ATPostAlterTypeCleanup-twice.patchDownload
From 4ab21b82da7ff32b8b37a51a1f8ee5079ee2b601 Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
Date: Mon, 13 Oct 2025 16:05:12 +0800
Subject: [PATCH v5 1/1] avoid call ATPostAlterTypeCleanup twice
should we use list_free_deep for tab->changedConstraintDefs,
tab->changedIndexDefs, tab->changedStatisticsDefs?
discussion: https://postgr.es/m/CACJufxHZsgn3zM5g-x7YmtFGzNDnRwR07S+GYfiUs+tZ45MDDw@mail.gmail.com
---
src/backend/commands/tablecmds.c | 18 ++++++++++++
.../regress/expected/generated_stored.out | 29 +++++++++++++++++++
.../regress/expected/generated_virtual.out | 26 +++++++++++++++++
src/test/regress/sql/generated_stored.sql | 8 +++++
src/test/regress/sql/generated_virtual.sql | 7 +++++
5 files changed, 88 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 5fd8b51312c..5c86fba48d9 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -15577,6 +15577,24 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
free_object_addresses(objects);
+ /*
+ * We have already deleted the dependent objects; now remove these objects
+ * themselves to avoid deleting them twice.
+ */
+ list_free(tab->changedConstraintOids);
+ list_free(tab->changedConstraintDefs);
+ list_free(tab->changedIndexOids);
+ list_free(tab->changedIndexDefs);
+ list_free(tab->changedStatisticsOids);
+ list_free(tab->changedStatisticsDefs);
+
+ tab->changedConstraintOids = NIL;
+ tab->changedConstraintDefs = NIL;
+ tab->changedIndexOids = NIL;
+ tab->changedIndexDefs = NIL;
+ tab->changedStatisticsOids = NIL;
+ tab->changedStatisticsDefs = NIL;
+
/*
* The objects will get recreated during subsequent passes over the work
* queue.
diff --git a/src/test/regress/expected/generated_stored.out b/src/test/regress/expected/generated_stored.out
index adac2cedfb2..d34a5839680 100644
--- a/src/test/regress/expected/generated_stored.out
+++ b/src/test/regress/expected/generated_stored.out
@@ -1120,6 +1120,35 @@ SELECT * FROM gtest25 ORDER BY a;
Indexes:
"gtest25_pkey" PRIMARY KEY, btree (a)
+ALTER TABLE gtest25 ADD COLUMN z INT DEFAULT 11;
+ALTER TABLE gtest25 ADD CONSTRAINT cc CHECK(b > 9) NOT VALID;
+ALTER TABLE gtest25 ADD CONSTRAINT check_z CHECK(z > 9) NOT VALID;
+ALTER TABLE gtest25 ALTER COLUMN z SET DATA TYPE numeric, ALTER COLUMN b SET EXPRESSION AS (z + 1);
+ALTER TABLE gtest25 ALTER COLUMN b SET DATA TYPE numeric, ALTER COLUMN b SET EXPRESSION AS (z + 2);
+SELECT * FROM gtest25 ORDER BY a;
+ a | b | c | x | d | y | z
+---+----+----+-----+-----+-----+----
+ 3 | 13 | 42 | 168 | 101 | 404 | 11
+ 4 | 13 | 42 | 168 | 101 | 404 | 11
+(2 rows)
+
+\d gtest25
+ Table "generated_stored_tests.gtest25"
+ Column | Type | Collation | Nullable | Default
+--------+------------------+-----------+----------+------------------------------------------------------
+ a | integer | | not null |
+ b | numeric | | | generated always as (z + 2::numeric) stored
+ c | integer | | | 42
+ x | integer | | | generated always as (c * 4) stored
+ d | double precision | | | 101
+ y | double precision | | | generated always as (d * 4::double precision) stored
+ z | numeric | | | 11
+Indexes:
+ "gtest25_pkey" PRIMARY KEY, btree (a)
+Check constraints:
+ "cc" CHECK (b > 9::numeric) NOT VALID
+ "check_z" CHECK (z > 9::numeric) NOT VALID
+
-- ALTER TABLE ... ALTER COLUMN
CREATE TABLE gtest27 (
a int,
diff --git a/src/test/regress/expected/generated_virtual.out b/src/test/regress/expected/generated_virtual.out
index c861bd36c5a..faac85bd769 100644
--- a/src/test/regress/expected/generated_virtual.out
+++ b/src/test/regress/expected/generated_virtual.out
@@ -1082,6 +1082,32 @@ SELECT * FROM gtest25 ORDER BY a;
Indexes:
"gtest25_pkey" PRIMARY KEY, btree (a)
+ALTER TABLE gtest25 ADD COLUMN z INT DEFAULT 11;
+-- ALTER TABLE gtest25 ADD CONSTRAINT cc CHECK(b > 9) NOT VALID; --ok
+-- ALTER TABLE gtest25 ADD CONSTRAINT check_z CHECK(z > 9) NOT VALID; --ok
+ALTER TABLE gtest25 ALTER COLUMN z SET DATA TYPE numeric, ALTER COLUMN b SET EXPRESSION AS (z + 1);
+ALTER TABLE gtest25 ALTER COLUMN b SET DATA TYPE numeric, ALTER COLUMN b SET EXPRESSION AS (z + 2);
+SELECT * FROM gtest25 ORDER BY a;
+ a | b | c | x | d | y | z
+---+----+----+-----+-----+-----+----
+ 3 | 13 | 42 | 168 | 101 | 404 | 11
+ 4 | 13 | 42 | 168 | 101 | 404 | 11
+(2 rows)
+
+\d gtest25
+ Table "generated_virtual_tests.gtest25"
+ Column | Type | Collation | Nullable | Default
+--------+------------------+-----------+----------+-----------------------------------------------
+ a | integer | | not null |
+ b | numeric | | | generated always as (z + 2::numeric)
+ c | integer | | | 42
+ x | integer | | | generated always as (c * 4)
+ d | double precision | | | 101
+ y | double precision | | | generated always as (d * 4::double precision)
+ z | numeric | | | 11
+Indexes:
+ "gtest25_pkey" PRIMARY KEY, btree (a)
+
-- ALTER TABLE ... ALTER COLUMN
CREATE TABLE gtest27 (
a int,
diff --git a/src/test/regress/sql/generated_stored.sql b/src/test/regress/sql/generated_stored.sql
index f56fde8d4e5..9fe2d2ca04c 100644
--- a/src/test/regress/sql/generated_stored.sql
+++ b/src/test/regress/sql/generated_stored.sql
@@ -516,6 +516,14 @@ ALTER TABLE gtest25 ALTER COLUMN d SET DATA TYPE float8,
ADD COLUMN y float8 GENERATED ALWAYS AS (d * 4) STORED;
SELECT * FROM gtest25 ORDER BY a;
\d gtest25
+ALTER TABLE gtest25 ADD COLUMN z INT DEFAULT 11;
+ALTER TABLE gtest25 ADD CONSTRAINT cc CHECK(b > 9) NOT VALID;
+ALTER TABLE gtest25 ADD CONSTRAINT check_z CHECK(z > 9) NOT VALID;
+ALTER TABLE gtest25 ALTER COLUMN z SET DATA TYPE numeric, ALTER COLUMN b SET EXPRESSION AS (z + 1);
+ALTER TABLE gtest25 ALTER COLUMN b SET DATA TYPE numeric, ALTER COLUMN b SET EXPRESSION AS (z + 2);
+SELECT * FROM gtest25 ORDER BY a;
+\d gtest25
+
-- ALTER TABLE ... ALTER COLUMN
CREATE TABLE gtest27 (
diff --git a/src/test/regress/sql/generated_virtual.sql b/src/test/regress/sql/generated_virtual.sql
index adfe88d74ae..0e19d0640d9 100644
--- a/src/test/regress/sql/generated_virtual.sql
+++ b/src/test/regress/sql/generated_virtual.sql
@@ -559,6 +559,13 @@ ALTER TABLE gtest25 ALTER COLUMN d SET DATA TYPE float8,
ADD COLUMN y float8 GENERATED ALWAYS AS (d * 4) VIRTUAL;
SELECT * FROM gtest25 ORDER BY a;
\d gtest25
+ALTER TABLE gtest25 ADD COLUMN z INT DEFAULT 11;
+-- ALTER TABLE gtest25 ADD CONSTRAINT cc CHECK(b > 9) NOT VALID; --ok
+-- ALTER TABLE gtest25 ADD CONSTRAINT check_z CHECK(z > 9) NOT VALID; --ok
+ALTER TABLE gtest25 ALTER COLUMN z SET DATA TYPE numeric, ALTER COLUMN b SET EXPRESSION AS (z + 1);
+ALTER TABLE gtest25 ALTER COLUMN b SET DATA TYPE numeric, ALTER COLUMN b SET EXPRESSION AS (z + 2);
+SELECT * FROM gtest25 ORDER BY a;
+\d gtest25
-- ALTER TABLE ... ALTER COLUMN
CREATE TABLE gtest27 (
--
2.34.1
On Oct 13, 2025, at 16:08, jian he <jian.universality@gmail.com> wrote:
On Wed, Oct 1, 2025 at 11:04 AM jian he <jian.universality@gmail.com> wrote:
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);That will not work if AT_PASS_ALTER_TYPE triggers the creation of a new
AT_PASS_SET_EXPRESSION entry.
For example, consider:
CREATE TABLE gtest25 (a int, b int GENERATED ALWAYS AS (a * 2) STORED);If we alter the type of column "a", the column b’s generation
expression would need
to be rebuilt (which is currently not supported). In that case, the current
logic would not be able to handle it.I think the correct fix is within ATPostAlterTypeCleanup.
at the end of ATPostAlterTypeCleanup, we already delete these changed
objects via
``performMultipleDeletions(objects, DROP_RESTRICT, PERFORM_DELETION_INTERNAL);``we can just list_free these (the below) objects too:
tab->changedConstraintOids
tab->changedConstraintDefs
tab->changedIndexOids
tab->changedIndexDefs
tab->changedStatisticsOids
tab->changedStatisticsDefssince this information would become stale if those objects have
already been dropped.
<v5-0001-avoid-call-ATPostAlterTypeCleanup-twice.patch>
I think the correct solution should be my proposed v4 that has a fundamental difference from your current change is that:
* The current implementation as well as your current change, they do ATPostAlterTypeCleanup() in either ALTER_TYPE pass or SET_EXPRESSION pass. Say a table has columns a and b, and a constraint (a+b>5), then I alter both columns types in a single command, then it will alter a’s type, then rebuild the constant, and alter b’s type, then rebuild the constraint again. For the first rebuild, because c’s type has not been updated, the rebuild may potentially fail.
* My v4 defers ATPostAlterTypeCleanup() to a later pass, which allows all "set data type” and “set expression” to finish, then start to rebuild constraints. My v4 does the cleanup in next pass of SET_EXPRESSION, which might not be the best solution, instead, it’s just a quick PoC to demo my idea. Perhaps, we may add a new pass.
But to handle the complicated case that I described above, v4 is still not enough. All “changedXXXXX” stuffs should be moved to an upper level of tab. Still using the same example, the constrain depends on both columns an and b, so we should remember the constraint only once.
WDYT?
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On Oct 13, 2025, at 16:08, jian he <jian.universality@gmail.com> wrote:
On Wed, Oct 1, 2025 at 11:04 AM jian he <jian.universality@gmail.com> wrote:
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);That will not work if AT_PASS_ALTER_TYPE triggers the creation of a new
AT_PASS_SET_EXPRESSION entry.
For example, consider:
CREATE TABLE gtest25 (a int, b int GENERATED ALWAYS AS (a * 2) STORED);If we alter the type of column "a", the column b’s generation
expression would need
to be rebuilt (which is currently not supported). In that case, the current
logic would not be able to handle it.I think the correct fix is within ATPostAlterTypeCleanup.
at the end of ATPostAlterTypeCleanup, we already delete these changed
objects via
``performMultipleDeletions(objects, DROP_RESTRICT, PERFORM_DELETION_INTERNAL);``we can just list_free these (the below) objects too:
tab->changedConstraintOids
tab->changedConstraintDefs
tab->changedIndexOids
tab->changedIndexDefs
tab->changedStatisticsOids
tab->changedStatisticsDefssince this information would become stale if those objects have
already been dropped.
<v5-0001-avoid-call-ATPostAlterTypeCleanup-twice.patch>
Now I fully understand what your concern is, so that I understand your solution also. So, I take back my previous reply and re-reply.
Your concern comes from an assumed solution to support altering a column’s type when it has a dependent generated column. Your assumed solution is like:
1. SQL: ALTER TABLE gtest25 ALTER COLUMN a TYPE bigint; # generated column depends on a
2. Before alter column a’s type, b’s expression is remembered
3. After alter column a’s type, ATPostAlterTypeClean() is called as there is no SET EXPRESSION. ATPostAlterTypeClean() will add a SET EXPRESSION subcmd to rebuild b
4. In SET_EXPRESSION PASS, b is recreated
5. ATPostAlterTypeClean() is called again, if we don’t cleanup those tab->changedXXX lists, then remembered objects will be rebuilt again, which may lead to errors. What’s why your solution of cleaning up table->changedXXX works.
In my opinion, in step 2, we don’t have follow the same mechanism to remember generated expressions as constraints. We can directly check if b has SET EXPRESSION cmd, if yes, we don’t need to do anything; otherwise, we get the b’s expression and inject a SET EXPRESSION subcmd for b. In this case, your worried problem will not exist. With this approach, ATPostAlterTypeClean() will always be called only once, so that execution path is simpler.
But anyway, we haven’t decided to support that yet. So for the current bug, your solution of:
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);
should work.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/