From 3e26eeeff901dae96b1da9f33e1b5f70cfac9169 Mon Sep 17 00:00:00 2001 From: jian he Date: Mon, 30 Jun 2025 13:47:23 +0800 Subject: [PATCH v2 1/1] fix-bug-18970 ALTER TYPE or SET EXPRESSION may necessitate rebuilding constraints, indexes, or statistics for related tables. As we may not acquire locks on these related relations before ATPostAlterTypeCleanup, we obtain appropriate locks here. This also means that for virtual generated columns, we still need to call RememberAllDependentForRebuilding, even though indexes and statistics cannot currently be created on them. When ALTER TYPE or SET EXPRESSION triggers a statistics rebuild for another table, we use ShareUpdateExclusiveLock to lock the related table, consistent with the locking approach in RemoveStatisticsById, AT_SetStatistics, and CreateStatistics. discussion: https://postgr.es/m/18970-a7d1cfe1f8d5d8d9@postgresql.org --- src/backend/commands/tablecmds.c | 66 +++++++++++++------ src/test/regress/expected/alter_table.out | 8 +++ .../regress/expected/generated_stored.out | 12 ++++ .../regress/expected/generated_virtual.out | 12 ++++ src/test/regress/sql/alter_table.sql | 8 +++ src/test/regress/sql/generated_stored.sql | 13 ++++ src/test/regress/sql/generated_virtual.sql | 12 ++++ 7 files changed, 111 insertions(+), 20 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index b8837f26cb4..feea6eace5d 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -185,7 +185,16 @@ typedef struct AlteredTableInfo List *subcmds[AT_NUM_PASSES]; /* Lists of AlterTableCmd */ /* Information saved by Phases 1/2 for Phase 3: */ List *constraints; /* List of NewConstraint */ + + /* + * List of NewColumnValue, representing new non-null default values + * (including generated columns). This could be either a new column with a + * non-null default, or a column that we're changing the type of). Note + * that this does not imply table rewite. check "rewrite" to determine table + * rewrite will occur. + */ List *newvals; /* List of NewColumnValue */ + List *afterStmts; /* List of utility command parsetrees */ bool verify_new_notnull; /* T if we should recheck NOT NULL */ int rewrite; /* Reason for forced rewrite, if any */ @@ -5843,8 +5852,9 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode, * If we change column data types, the operation has to be propagated * to tables that use this table's rowtype as a column type. * tab->newvals will also be non-NULL in the case where we're adding a - * column with a default. We choose to forbid that case as well, - * since composite types might eventually support defaults. + * column with a default (including generated columns). We choose to + * forbid that case as well, since composite types might eventually + * support defaults. * * (Eventually we'll probably need to check for composite type * dependencies even when we're just scanning the table without a @@ -8645,15 +8655,15 @@ ATExecSetExpression(AlteredTableInfo *tab, Relation rel, const char *colName, /* make sure we don't conflict with later attribute modifications */ CommandCounterIncrement(); - - /* - * Find everything that depends on the column (constraints, indexes, - * etc), and record enough information to let us recreate the objects - * after rewrite. - */ - RememberAllDependentForRebuilding(tab, AT_SetExpression, rel, attnum, colName); } + /* + * Find everything that depends on the column (constraints, indexes, + * etc), and record enough information to let us recreate the objects + * after rewrite. + */ + RememberAllDependentForRebuilding(tab, AT_SetExpression, rel, attnum, colName); + /* * Drop the dependency records of the GENERATED expression, in particular * its INTERNAL dependency on the column, which would otherwise cause @@ -8689,19 +8699,23 @@ ATExecSetExpression(AlteredTableInfo *tab, Relation rel, const char *colName, /* Make above new expression visible */ CommandCounterIncrement(); + /* + * Changing a virtual generated column's expression is akin to altering its + * type, requiring a call to find_composite_type_dependencies to check if + * the virtual generated column is used in any table. + * Therefore we need add this defval to tab->newvals for virtual generated + * column too, so Phase3 will call find_composite_type_dependencies. + */ + defval = (Expr *) build_column_default(rel, attnum); + newval = (NewColumnValue *) palloc0(sizeof(NewColumnValue)); + newval->attnum = attnum; + newval->expr = expression_planner(defval); + newval->is_generated = true; + tab->newvals = lappend(tab->newvals, newval); + + /* Prepare for table rewrite */ if (rewrite) - { - /* Prepare for table rewrite */ - defval = (Expr *) build_column_default(rel, attnum); - - newval = (NewColumnValue *) palloc0(sizeof(NewColumnValue)); - newval->attnum = attnum; - newval->expr = expression_planner(defval); - newval->is_generated = true; - - tab->newvals = lappend(tab->newvals, newval); tab->rewrite |= AT_REWRITE_DEFAULT_VAL; - } /* Drop any pg_statistic entry for the column */ RemoveStatistics(RelationGetRelid(rel), attnum); @@ -15488,6 +15502,9 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode) Oid relid; relid = IndexGetRelation(oldId, false); + + if (relid != tab->relid) + LockRelationOid(relid, AccessExclusiveLock); ATPostAlterTypeParse(oldId, relid, InvalidOid, (char *) lfirst(def_item), wqueue, lockmode, tab->rewrite); @@ -15504,6 +15521,15 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode) Oid relid; relid = StatisticsGetRelation(oldId, false); + + /* + * We use ShareUpdateExclusiveLock to lock the relation for which we + * will rebuild statistics, aligning with the lock level used in + * CreateStatistics and RemoveStatisticsById. + */ + if (relid != tab->relid) + LockRelationOid(relid, ShareUpdateExclusiveLock); + ATPostAlterTypeParse(oldId, relid, InvalidOid, (char *) lfirst(def_item), wqueue, lockmode, tab->rewrite); diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index 750efc042d8..cf2235328f1 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -4750,6 +4750,14 @@ create table attbl(a int); create table atref(b attbl check ((b).a is not null)); alter table attbl alter column a type numeric; -- someday this should work ERROR: cannot alter table "attbl" because column "atref.b" uses its row type +alter table atref drop constraint atref_b_check; +create statistics atref_stat ON ((b).a is not null) from atref; +alter table attbl alter column a type numeric; --error +ERROR: cannot alter table "attbl" because column "atref.b" uses its row type +drop statistics atref_stat; +create index atref_idx on atref (((b).a)); +alter table attbl alter column a type numeric; --error +ERROR: cannot alter table "attbl" because column "atref.b" uses its row type drop table attbl, atref; /* End test case for bug #18970 */ -- Test that ALTER TABLE rewrite preserves a clustered index diff --git a/src/test/regress/expected/generated_stored.out b/src/test/regress/expected/generated_stored.out index 16de30ab191..11c856ddc01 100644 --- a/src/test/regress/expected/generated_stored.out +++ b/src/test/regress/expected/generated_stored.out @@ -1313,6 +1313,18 @@ CREATE TABLE gtest31_1 (a int, b text GENERATED ALWAYS AS ('hello') STORED, c te CREATE TABLE gtest31_2 (x int, y gtest31_1); ALTER TABLE gtest31_1 ALTER COLUMN b TYPE varchar; -- fails ERROR: cannot alter table "gtest31_1" because column "gtest31_2.y" uses its row type +-- bug #18970, https://postgr.es/m/18970-a7d1cfe1f8d5d8d9@postgresql.org +ALTER TABLE gtest31_2 ADD CONSTRAINT cc CHECK ((y).b IS NOT NULL); +ALTER TABLE gtest31_1 ALTER COLUMN b SET EXPRESSION AS ( 'hello'); --error +ERROR: cannot alter table "gtest31_1" because column "gtest31_2.y" uses its row type +ALTER TABLE gtest31_2 DROP CONSTRAINT cc; +CREATE STATISTICS gtest31_2_stat ON ((y).b is not null) FROM gtest31_2; +ALTER TABLE gtest31_1 ALTER COLUMN b SET EXPRESSION AS ( 'hello'); --error +ERROR: cannot alter table "gtest31_1" because column "gtest31_2.y" uses its row type +DROP STATISTICS gtest31_2_stat; +CREATE INDEX gtest31_2_y_idx ON gtest31_2(((y).b)); +ALTER TABLE gtest31_1 ALTER COLUMN b SET EXPRESSION AS ( 'hello'); --error +ERROR: cannot alter table "gtest31_1" because column "gtest31_2.y" uses its row type DROP TABLE gtest31_1, gtest31_2; -- Check it for a partitioned table, too CREATE TABLE gtest31_1 (a int, b text GENERATED ALWAYS AS ('hello') STORED, c text) PARTITION BY LIST (a); diff --git a/src/test/regress/expected/generated_virtual.out b/src/test/regress/expected/generated_virtual.out index df704b5166f..b2e1456a937 100644 --- a/src/test/regress/expected/generated_virtual.out +++ b/src/test/regress/expected/generated_virtual.out @@ -1283,6 +1283,18 @@ CREATE TABLE gtest31_1 (a int, b text GENERATED ALWAYS AS ('hello') VIRTUAL, c t CREATE TABLE gtest31_2 (x int, y gtest31_1); ALTER TABLE gtest31_1 ALTER COLUMN b TYPE varchar; -- fails ERROR: cannot alter table "gtest31_1" because column "gtest31_2.y" uses its row type +-- bug #18970, https://postgr.es/m/18970-a7d1cfe1f8d5d8d9@postgresql.org +ALTER TABLE gtest31_2 ADD CONSTRAINT cc CHECK ((y).b IS NOT NULL); +ALTER TABLE gtest31_1 ALTER COLUMN b SET EXPRESSION AS ( 'hello'); --error +ERROR: cannot alter table "gtest31_1" because column "gtest31_2.y" uses its row type +ALTER TABLE gtest31_2 DROP CONSTRAINT cc; +CREATE STATISTICS gtest31_2_stat ON ((y).b is not null) FROM gtest31_2; +ALTER TABLE gtest31_1 ALTER COLUMN b SET EXPRESSION AS ( 'hello'); --error +ERROR: cannot alter table "gtest31_1" because column "gtest31_2.y" uses its row type +DROP STATISTICS gtest31_2_stat; +CREATE INDEX gtest31_2_y_idx ON gtest31_2(((y).b)); +ALTER TABLE gtest31_1 ALTER COLUMN b SET EXPRESSION AS ( 'hello'); --error +ERROR: cannot alter table "gtest31_1" because column "gtest31_2.y" uses its row type DROP TABLE gtest31_1, gtest31_2; -- Check it for a partitioned table, too CREATE TABLE gtest31_1 (a int, b text GENERATED ALWAYS AS ('hello') VIRTUAL, c text) PARTITION BY LIST (a); diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index 41cff198e18..fcf522eb493 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -3074,6 +3074,14 @@ drop table attbl, atref; create table attbl(a int); create table atref(b attbl check ((b).a is not null)); alter table attbl alter column a type numeric; -- someday this should work + +alter table atref drop constraint atref_b_check; +create statistics atref_stat ON ((b).a is not null) from atref; +alter table attbl alter column a type numeric; --error +drop statistics atref_stat; + +create index atref_idx on atref (((b).a)); +alter table attbl alter column a type numeric; --error drop table attbl, atref; /* End test case for bug #18970 */ diff --git a/src/test/regress/sql/generated_stored.sql b/src/test/regress/sql/generated_stored.sql index 4ec155f2da9..e58a339e530 100644 --- a/src/test/regress/sql/generated_stored.sql +++ b/src/test/regress/sql/generated_stored.sql @@ -595,6 +595,19 @@ ALTER TABLE gtest30_1 ALTER COLUMN b DROP EXPRESSION; -- error CREATE TABLE gtest31_1 (a int, b text GENERATED ALWAYS AS ('hello') STORED, c text); CREATE TABLE gtest31_2 (x int, y gtest31_1); ALTER TABLE gtest31_1 ALTER COLUMN b TYPE varchar; -- fails + +-- bug #18970, https://postgr.es/m/18970-a7d1cfe1f8d5d8d9@postgresql.org +ALTER TABLE gtest31_2 ADD CONSTRAINT cc CHECK ((y).b IS NOT NULL); +ALTER TABLE gtest31_1 ALTER COLUMN b SET EXPRESSION AS ( 'hello'); --error +ALTER TABLE gtest31_2 DROP CONSTRAINT cc; + +CREATE STATISTICS gtest31_2_stat ON ((y).b is not null) FROM gtest31_2; +ALTER TABLE gtest31_1 ALTER COLUMN b SET EXPRESSION AS ( 'hello'); --error +DROP STATISTICS gtest31_2_stat; + +CREATE INDEX gtest31_2_y_idx ON gtest31_2(((y).b)); +ALTER TABLE gtest31_1 ALTER COLUMN b SET EXPRESSION AS ( 'hello'); --error + DROP TABLE gtest31_1, gtest31_2; -- Check it for a partitioned table, too diff --git a/src/test/regress/sql/generated_virtual.sql b/src/test/regress/sql/generated_virtual.sql index 6fa986515b9..137251ff1a9 100644 --- a/src/test/regress/sql/generated_virtual.sql +++ b/src/test/regress/sql/generated_virtual.sql @@ -646,6 +646,18 @@ ALTER TABLE gtest30_1 ALTER COLUMN b DROP EXPRESSION; -- error CREATE TABLE gtest31_1 (a int, b text GENERATED ALWAYS AS ('hello') VIRTUAL, c text); CREATE TABLE gtest31_2 (x int, y gtest31_1); ALTER TABLE gtest31_1 ALTER COLUMN b TYPE varchar; -- fails + +-- bug #18970, https://postgr.es/m/18970-a7d1cfe1f8d5d8d9@postgresql.org +ALTER TABLE gtest31_2 ADD CONSTRAINT cc CHECK ((y).b IS NOT NULL); +ALTER TABLE gtest31_1 ALTER COLUMN b SET EXPRESSION AS ( 'hello'); --error +ALTER TABLE gtest31_2 DROP CONSTRAINT cc; + +CREATE STATISTICS gtest31_2_stat ON ((y).b is not null) FROM gtest31_2; +ALTER TABLE gtest31_1 ALTER COLUMN b SET EXPRESSION AS ( 'hello'); --error +DROP STATISTICS gtest31_2_stat; + +CREATE INDEX gtest31_2_y_idx ON gtest31_2(((y).b)); +ALTER TABLE gtest31_1 ALTER COLUMN b SET EXPRESSION AS ( 'hello'); --error DROP TABLE gtest31_1, gtest31_2; -- Check it for a partitioned table, too -- 2.34.1