public inbox for [email protected]help / color / mirror / Atom feed
Fix SET EXPRESSION for virtual columns with whole-row dependencies 9+ messages / 3 participants [nested] [flat]
* Fix SET EXPRESSION for virtual columns with whole-row dependencies @ 2026-06-09 06:22 Chao Li <[email protected]> 0 siblings, 1 reply; 9+ messages in thread From: Chao Li @ 2026-06-09 06:22 UTC (permalink / raw) To: Postgres hackers <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; jian he <[email protected]> Hi, While testing “[f80bedd52] Allow ALTER COLUMN SET EXPRESSION on virtual columns”, I found that the feature missed handling whole-row check constraints. Here is a repro: ``` evantest=# create table t( evantest(# a int, evantest(# b int generated always as (a*2) virtual, evantest(# constraint row_c check (t is not null) evantest(# ); CREATE TABLE evantest=# insert into t(a) values(1); INSERT 0 1 evantest=# alter table t alter b set expression as (nullif(a, 1)); ALTER TABLE evantest=# select * from t; a | b ---+--- 1 | (1 row) ``` The ALTER TABLE should fail, because it makes b become NULL, which breaks the constraint row_c. For comparison, if we use the nullif expression at table creation time, we cannot insert a row with a = 1: ``` evantest=# create table t1( evantest(# a int, evantest(# b int generated always as (nullif(a,1)) virtual, evantest(# constraint row_c check (t1 is not null) evantest(# ); CREATE TABLE evantest=# insert into t1(a) values(1); ERROR: new row for relation "t1" violates check constraint "row_c" DETAIL: Failing row contains (1, virtual). ``` I think the problem is that ATExecSetExpression() only calls RememberAllDependentForRebuilding(tab, AT_SetExpression, rel, attnum, colName); to find dependent objects. In this repro, attnum is 2, which is b’s attnum, and colName is b, so it doesn’t find the whole-row constraint row_c. The same problem applies to whole-row indexes as well. Since RememberAllDependentForRebuilding() is only used by AT_AlterColumnType and AT_SetExpression, I enhanced it to handle attnum == 0 for AT_SetExpression, so that whole-row dependent constraints and indexes are remembered for a virtual generated column. See the attached patch for details. With the fix, rerunning the repro makes ALTER TABLE SET EXPRESSION fail as expected: ``` evantest=# alter table t alter b set expression as (nullif(a, 1)); ERROR: check constraint "row_c" of relation "t" is violated by some row ``` Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ Attachments: [application/octet-stream] v1-0001-Fix-SET-EXPRESSION-with-whole-row-virtual-column-.patch (7.5K, ../../[email protected]/2-v1-0001-Fix-SET-EXPRESSION-with-whole-row-virtual-column-.patch) download | inline diff: From 54088e2c329dfed17e861cbe8d397e5b707485ed Mon Sep 17 00:00:00 2001 From: "Chao Li (Evan)" <[email protected]> Date: Tue, 9 Jun 2026 14:05:00 +0800 Subject: [PATCH v1] Fix SET EXPRESSION with whole-row virtual column dependencies ALTER COLUMN SET EXPRESSION on a virtual generated column only looked for dependencies on the column's attnum. Whole-row expressions, such as "rel IS NOT NULL", depend on the relation as a whole instead, but they can still observe the virtual column's computed value. Teach the SET EXPRESSION path to also scan whole-relation dependencies for virtual generated columns. This lets whole-row CHECK constraints be revalidated and whole-row expression indexes be rebuilt after the expression changes. Add regression coverage for a whole-row CHECK constraint, a whole-row expression index, and an ordinary index that should not be rebuilt. Author: Chao Li <[email protected]> --- src/backend/commands/tablecmds.c | 40 +++++++++++++++++-- .../regress/expected/generated_virtual.out | 19 +++++++++ src/test/regress/sql/generated_virtual.sql | 8 ++++ 3 files changed, 64 insertions(+), 3 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index a33e22e8e61..594aabfa295 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -8765,6 +8765,15 @@ ATExecSetExpression(AlteredTableInfo *tab, Relation rel, const char *colName, * and record enough information to let us recreate the objects. */ RememberAllDependentForRebuilding(tab, AT_SetExpression, rel, attnum, colName); + if (attgenerated == ATTRIBUTE_GENERATED_VIRTUAL) + { + /* + * Whole-row expressions, such as "rel IS NOT NULL", depend on the + * relation as a whole rather than on the virtual generated column's + * attnum, but they can observe the column's new computed value. + */ + RememberAllDependentForRebuilding(tab, AT_SetExpression, rel, 0, NULL); + } /* * Drop the dependency records of the GENERATED expression, in particular @@ -15293,7 +15302,9 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel, /* * Subroutine for ATExecAlterColumnType and ATExecSetExpression: Find everything * that depends on the column (constraints, indexes, etc), and record enough - * information to let us recreate the objects. + * information to let us recreate the objects. For virtual generated columns, + * ATExecSetExpression can also pass attnum == 0 to find whole-row expression + * dependencies that observe the column's computed value. */ static void RememberAllDependentForRebuilding(AlteredTableInfo *tab, AlterTableType subtype, @@ -15305,6 +15316,7 @@ RememberAllDependentForRebuilding(AlteredTableInfo *tab, AlterTableType subtype, HeapTuple depTup; Assert(subtype == AT_AlterColumnType || subtype == AT_SetExpression); + Assert(attnum != 0 || subtype == AT_SetExpression); depRel = table_open(DependRelationId, RowExclusiveLock); @@ -15355,6 +15367,15 @@ RememberAllDependentForRebuilding(AlteredTableInfo *tab, AlterTableType subtype, } else { + /* + * In a whole-relation scan for SET EXPRESSION, ignore + * relation dependencies that are not indexes. Objects + * with expressions that need action, such as + * constraints, are handled by their own cases. + */ + if (attnum == 0) + break; + /* Not expecting any other direct dependencies... */ elog(ERROR, "unexpected object depending on column: %s", getObjectDescription(&foundObject, false)); @@ -15364,7 +15385,14 @@ RememberAllDependentForRebuilding(AlteredTableInfo *tab, AlterTableType subtype, case ConstraintRelationId: Assert(foundObject.objectSubId == 0); - RememberConstraintForRebuilding(foundObject.objectId, tab); + if (attnum == 0) + { + /* Whole-row CHECK constraints may need to be revalidated */ + if (get_constraint_type(foundObject.objectId) == CONSTRAINT_CHECK) + RememberConstraintForRebuilding(foundObject.objectId, tab); + } + else + RememberConstraintForRebuilding(foundObject.objectId, tab); break; case ProcedureRelationId: @@ -15505,8 +15533,14 @@ RememberAllDependentForRebuilding(AlteredTableInfo *tab, AlterTableType subtype, /* * We don't expect any other sorts of objects to depend on a - * column. + * column. A whole-relation scan can find the relation's row + * type, which doesn't need rebuilding for SET EXPRESSION. */ + if (attnum == 0 && + foundObject.classId == TypeRelationId && + get_typ_typrelid(foundObject.objectId) == RelationGetRelid(rel)) + continue; + elog(ERROR, "unexpected object depending on column: %s", getObjectDescription(&foundObject, false)); break; diff --git a/src/test/regress/expected/generated_virtual.out b/src/test/regress/expected/generated_virtual.out index 24d5dbf46ca..625b4025828 100644 --- a/src/test/regress/expected/generated_virtual.out +++ b/src/test/regress/expected/generated_virtual.out @@ -694,6 +694,25 @@ INSERT INTO gtest20c VALUES (1); -- ok INSERT INTO gtest20c VALUES (NULL); -- fails ERROR: new row for relation "gtest20c" violates check constraint "whole_row_check" DETAIL: Failing row contains (null, virtual). +ALTER TABLE gtest20c ALTER COLUMN b SET EXPRESSION AS (nullif(a, 1)); -- fails +ERROR: check constraint "whole_row_check" of relation "gtest20c" is violated by some row +CREATE INDEX gtest20c_expr_idx ON gtest20c ((gtest20c IS NOT NULL)); +CREATE INDEX gtest20c_a_idx ON gtest20c (a); +SELECT pg_relation_filenode('gtest20c_expr_idx') AS gtest20c_expr_idx_filenode \gset +SELECT pg_relation_filenode('gtest20c_a_idx') AS gtest20c_a_idx_filenode \gset +ALTER TABLE gtest20c ALTER COLUMN b SET EXPRESSION AS (a * 3); -- ok, rebuilds index +SELECT pg_relation_filenode('gtest20c_expr_idx') <> :gtest20c_expr_idx_filenode AS whole_row_index_rebuilt; + whole_row_index_rebuilt +------------------------- + t +(1 row) + +SELECT pg_relation_filenode('gtest20c_a_idx') = :gtest20c_a_idx_filenode AS ordinary_index_not_rebuilt; + ordinary_index_not_rebuilt +---------------------------- + t +(1 row) + -- not-null constraints CREATE TABLE gtest21a (a int PRIMARY KEY, b int GENERATED ALWAYS AS (nullif(a, 0)) VIRTUAL NOT NULL); INSERT INTO gtest21a (a) VALUES (1); -- ok diff --git a/src/test/regress/sql/generated_virtual.sql b/src/test/regress/sql/generated_virtual.sql index 9c2bb6590b3..14fa47b6173 100644 --- a/src/test/regress/sql/generated_virtual.sql +++ b/src/test/regress/sql/generated_virtual.sql @@ -347,6 +347,14 @@ CREATE TABLE gtest20c (a int, b int GENERATED ALWAYS AS (a * 2) VIRTUAL); ALTER TABLE gtest20c ADD CONSTRAINT whole_row_check CHECK (gtest20c IS NOT NULL); INSERT INTO gtest20c VALUES (1); -- ok INSERT INTO gtest20c VALUES (NULL); -- fails +ALTER TABLE gtest20c ALTER COLUMN b SET EXPRESSION AS (nullif(a, 1)); -- fails +CREATE INDEX gtest20c_expr_idx ON gtest20c ((gtest20c IS NOT NULL)); +CREATE INDEX gtest20c_a_idx ON gtest20c (a); +SELECT pg_relation_filenode('gtest20c_expr_idx') AS gtest20c_expr_idx_filenode \gset +SELECT pg_relation_filenode('gtest20c_a_idx') AS gtest20c_a_idx_filenode \gset +ALTER TABLE gtest20c ALTER COLUMN b SET EXPRESSION AS (a * 3); -- ok, rebuilds index +SELECT pg_relation_filenode('gtest20c_expr_idx') <> :gtest20c_expr_idx_filenode AS whole_row_index_rebuilt; +SELECT pg_relation_filenode('gtest20c_a_idx') = :gtest20c_a_idx_filenode AS ordinary_index_not_rebuilt; -- not-null constraints CREATE TABLE gtest21a (a int PRIMARY KEY, b int GENERATED ALWAYS AS (nullif(a, 0)) VIRTUAL NOT NULL); -- 2.50.1 (Apple Git-155) ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Fix SET EXPRESSION for virtual columns with whole-row dependencies @ 2026-06-09 07:23 jian he <[email protected]> parent: Chao Li <[email protected]> 0 siblings, 1 reply; 9+ messages in thread From: jian he @ 2026-06-09 07:23 UTC (permalink / raw) To: Chao Li <[email protected]>; +Cc: Postgres hackers <[email protected]>; Peter Eisentraut <[email protected]> On Tue, Jun 9, 2026 at 2:23 PM Chao Li <[email protected]> wrote: > > Hi, > > While testing “[f80bedd52] Allow ALTER COLUMN SET EXPRESSION on virtual columns”, I found that the feature missed handling whole-row check constraints. > > Here is a repro: > ``` > evantest=# create table t( > evantest(# a int, > evantest(# b int generated always as (a*2) virtual, > evantest(# constraint row_c check (t is not null) > evantest(# ); > CREATE TABLE > evantest=# insert into t(a) values(1); > INSERT 0 1 > evantest=# alter table t alter b set expression as (nullif(a, 1)); > ALTER TABLE > evantest=# select * from t; > a | b > ---+--- > 1 | > (1 row) > ``` > > The ALTER TABLE should fail, because it makes b become NULL, which breaks the constraint row_c. > Hi. In case you are wondering whole-row objects: ALTER TABLE DROP COLUMN and ALTER COLUMN SET DATA TYPE are covered by https://commitfest.postgresql.org/patch/5988 and https://commitfest.postgresql.org/patch/6055. Meanwhile, ALTER TABLE SET EXPRESSION is being handled over in https://commitfest.postgresql.org/patch/6755 ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Fix SET EXPRESSION for virtual columns with whole-row dependencies @ 2026-06-09 07:28 Chao Li <[email protected]> parent: jian he <[email protected]> 0 siblings, 1 reply; 9+ messages in thread From: Chao Li @ 2026-06-09 07:28 UTC (permalink / raw) To: jian he <[email protected]>; +Cc: Postgres hackers <[email protected]>; Peter Eisentraut <[email protected]> > On Jun 9, 2026, at 15:23, jian he <[email protected]> wrote: > > On Tue, Jun 9, 2026 at 2:23 PM Chao Li <[email protected]> wrote: >> >> Hi, >> >> While testing “[f80bedd52] Allow ALTER COLUMN SET EXPRESSION on virtual columns”, I found that the feature missed handling whole-row check constraints. >> >> Here is a repro: >> ``` >> evantest=# create table t( >> evantest(# a int, >> evantest(# b int generated always as (a*2) virtual, >> evantest(# constraint row_c check (t is not null) >> evantest(# ); >> CREATE TABLE >> evantest=# insert into t(a) values(1); >> INSERT 0 1 >> evantest=# alter table t alter b set expression as (nullif(a, 1)); >> ALTER TABLE >> evantest=# select * from t; >> a | b >> ---+--- >> 1 | >> (1 row) >> ``` >> >> The ALTER TABLE should fail, because it makes b become NULL, which breaks the constraint row_c. >> > > Hi. > > In case you are wondering whole-row objects: > ALTER TABLE DROP COLUMN and ALTER COLUMN SET DATA TYPE are covered by > https://commitfest.postgresql.org/patch/5988 and > https://commitfest.postgresql.org/patch/6055. > > Meanwhile, ALTER TABLE SET EXPRESSION is being handled over in > https://commitfest.postgresql.org/patch/6755 Thanks for pointing out that. I will review 6755. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Fix SET EXPRESSION for virtual columns with whole-row dependencies @ 2026-06-10 09:05 Chao Li <[email protected]> parent: Chao Li <[email protected]> 0 siblings, 2 replies; 9+ messages in thread From: Chao Li @ 2026-06-10 09:05 UTC (permalink / raw) To: jian he <[email protected]>; +Cc: Postgres hackers <[email protected]>; Peter Eisentraut <[email protected]> > On Jun 9, 2026, at 15:28, Chao Li <[email protected]> wrote: > > > >> On Jun 9, 2026, at 15:23, jian he <[email protected]> wrote: >> >> On Tue, Jun 9, 2026 at 2:23 PM Chao Li <[email protected]> wrote: >>> >>> Hi, >>> >>> While testing “[f80bedd52] Allow ALTER COLUMN SET EXPRESSION on virtual columns”, I found that the feature missed handling whole-row check constraints. >>> >>> Here is a repro: >>> ``` >>> evantest=# create table t( >>> evantest(# a int, >>> evantest(# b int generated always as (a*2) virtual, >>> evantest(# constraint row_c check (t is not null) >>> evantest(# ); >>> CREATE TABLE >>> evantest=# insert into t(a) values(1); >>> INSERT 0 1 >>> evantest=# alter table t alter b set expression as (nullif(a, 1)); >>> ALTER TABLE >>> evantest=# select * from t; >>> a | b >>> ---+--- >>> 1 | >>> (1 row) >>> ``` >>> >>> The ALTER TABLE should fail, because it makes b become NULL, which breaks the constraint row_c. >>> >> >> Hi. >> >> In case you are wondering whole-row objects: >> ALTER TABLE DROP COLUMN and ALTER COLUMN SET DATA TYPE are covered by >> https://commitfest.postgresql.org/patch/5988 and >> https://commitfest.postgresql.org/patch/6055. >> >> Meanwhile, ALTER TABLE SET EXPRESSION is being handled over in >> https://commitfest.postgresql.org/patch/6755 > > Thanks for pointing out that. I will review 6755. > > Best regards, > -- > Chao Li (Evan) > HighGo Software Co., Ltd. > https://www.highgo.com/ > After reading the other implementation in [1], I realized that I had missed the partial-index case, so I added coverage for that. I am still sending an updated version of this patch because my implementation is different from the one in [1]. I would like people to compare the two approaches and decide which direction is better. [1] https://postgr.es/m/CAJTYsWXOkyeDVbzymWc9sKrq7Y_MUv6XJXN4H9GfsBOPd3NJ+w@mail.gmail.com Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Fix SET EXPRESSION for virtual columns with whole-row dependencies @ 2026-06-10 09:12 Chao Li <[email protected]> parent: Chao Li <[email protected]> 1 sibling, 1 reply; 9+ messages in thread From: Chao Li @ 2026-06-10 09:12 UTC (permalink / raw) To: jian he <[email protected]>; +Cc: Postgres hackers <[email protected]>; Peter Eisentraut <[email protected]> > On Jun 10, 2026, at 17:05, Chao Li <[email protected]> wrote: > > > >> On Jun 9, 2026, at 15:28, Chao Li <[email protected]> wrote: >> >> >> >>> On Jun 9, 2026, at 15:23, jian he <[email protected]> wrote: >>> >>> On Tue, Jun 9, 2026 at 2:23 PM Chao Li <[email protected]> wrote: >>>> >>>> Hi, >>>> >>>> While testing “[f80bedd52] Allow ALTER COLUMN SET EXPRESSION on virtual columns”, I found that the feature missed handling whole-row check constraints. >>>> >>>> Here is a repro: >>>> ``` >>>> evantest=# create table t( >>>> evantest(# a int, >>>> evantest(# b int generated always as (a*2) virtual, >>>> evantest(# constraint row_c check (t is not null) >>>> evantest(# ); >>>> CREATE TABLE >>>> evantest=# insert into t(a) values(1); >>>> INSERT 0 1 >>>> evantest=# alter table t alter b set expression as (nullif(a, 1)); >>>> ALTER TABLE >>>> evantest=# select * from t; >>>> a | b >>>> ---+--- >>>> 1 | >>>> (1 row) >>>> ``` >>>> >>>> The ALTER TABLE should fail, because it makes b become NULL, which breaks the constraint row_c. >>>> >>> >>> Hi. >>> >>> In case you are wondering whole-row objects: >>> ALTER TABLE DROP COLUMN and ALTER COLUMN SET DATA TYPE are covered by >>> https://commitfest.postgresql.org/patch/5988 and >>> https://commitfest.postgresql.org/patch/6055. >>> >>> Meanwhile, ALTER TABLE SET EXPRESSION is being handled over in >>> https://commitfest.postgresql.org/patch/6755 >> >> Thanks for pointing out that. I will review 6755. >> >> Best regards, >> -- >> Chao Li (Evan) >> HighGo Software Co., Ltd. >> https://www.highgo.com/ >> > > After reading the other implementation in [1], I realized that I had missed the partial-index case, so I added coverage for that. > > I am still sending an updated version of this patch because my implementation is different from the one in [1]. I would like people to compare the two approaches and decide which direction is better. > > [1] https://postgr.es/m/CAJTYsWXOkyeDVbzymWc9sKrq7Y_MUv6XJXN4H9GfsBOPd3NJ+w@mail.gmail.com > > Best regards, > -- > Chao Li (Evan) > HighGo Software Co., Ltd. > https://www.highgo.com/ > > Oops! Forgot the attachment. Here comes it. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ Attachments: [application/octet-stream] v2-0001-Fix-SET-EXPRESSION-with-whole-row-virtual-column-.patch (13.7K, ../../[email protected]/2-v2-0001-Fix-SET-EXPRESSION-with-whole-row-virtual-column-.patch) download | inline diff: From 19b577b3e5ea2c1ec2e88e6b3009e5375f7b7c61 Mon Sep 17 00:00:00 2001 From: "Chao Li (Evan)" <[email protected]> Date: Tue, 9 Jun 2026 14:05:00 +0800 Subject: [PATCH v2] Fix SET EXPRESSION with whole-row virtual column dependencies ALTER COLUMN SET EXPRESSION on a virtual generated column only looked for dependencies on the column's attnum. Whole-row expressions, such as "rel IS NOT NULL", depend on the relation as a whole instead, but they can still observe the virtual column's computed value. Teach the SET EXPRESSION path to also scan whole-relation dependencies for generated columns. This lets whole-row CHECK constraints be revalidated and whole-row expression indexes be rebuilt after the expression changes. Add regression coverage for a whole-row CHECK constraint, a whole-row expression index, and an ordinary index that should not be rebuilt. Author: Chao Li <[email protected]> Discussion: https://postgr.es/m/[email protected] Discussion: https://postgr.es/m/CAJTYsWXOkyeDVbzymWc9sKrq7Y_MUv6XJXN4H9GfsBOPd3NJ+w@mail.gmail.com --- src/backend/commands/tablecmds.c | 123 +++++++++++++++++- .../regress/expected/generated_stored.out | 11 ++ .../regress/expected/generated_virtual.out | 27 ++++ src/test/regress/sql/generated_stored.sql | 5 + src/test/regress/sql/generated_virtual.sql | 11 ++ 5 files changed, 174 insertions(+), 3 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index a33e22e8e61..a4b80fa596b 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -43,6 +43,7 @@ #include "catalog/pg_extension_d.h" #include "catalog/pg_foreign_table.h" #include "catalog/pg_inherits.h" +#include "catalog/pg_index.h" #include "catalog/pg_largeobject.h" #include "catalog/pg_largeobject_metadata.h" #include "catalog/pg_namespace.h" @@ -686,6 +687,7 @@ static ObjectAddress ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel, AlterTableCmd *cmd, LOCKMODE lockmode); static void RememberAllDependentForRebuilding(AlteredTableInfo *tab, AlterTableType subtype, Relation rel, AttrNumber attnum, const char *colName); +static void RememberWholeRowIndexesForRebuilding(AlteredTableInfo *tab, Relation rel); static void RememberConstraintForRebuilding(Oid conoid, AlteredTableInfo *tab); static void RememberIndexForRebuilding(Oid indoid, AlteredTableInfo *tab); static void RememberStatisticsForRebuilding(Oid stxoid, AlteredTableInfo *tab); @@ -8766,6 +8768,20 @@ ATExecSetExpression(AlteredTableInfo *tab, Relation rel, const char *colName, */ RememberAllDependentForRebuilding(tab, AT_SetExpression, rel, attnum, colName); + /* + * Whole-row expressions, such as "rel IS NOT NULL", depend on the + * relation as a whole rather than on the generated column's attnum, but + * they can observe the column's new computed value. + */ + RememberAllDependentForRebuilding(tab, AT_SetExpression, rel, 0, NULL); + + /* + * recordDependencyOnSingleRelExpr() does not emit dependencies for + * whole-row Vars, so indexes with ordinary key columns and whole-row + * predicates might not have a whole-relation dependency to find above. + */ + RememberWholeRowIndexesForRebuilding(tab, rel); + /* * Drop the dependency records of the GENERATED expression, in particular * its INTERNAL dependency on the column, which would otherwise cause @@ -15290,10 +15306,88 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel, return address; } +/* + * Return true if the expression contains a whole-row Var. + */ +static bool +expr_has_whole_row_var(Node *expr) +{ + Bitmapset *expr_attrs = NULL; + bool result; + + pull_varattnos(expr, 1, &expr_attrs); + result = bms_is_member(InvalidAttrNumber - FirstLowInvalidHeapAttributeNumber, + expr_attrs); + bms_free(expr_attrs); + + return result; +} + +/* + * Find indexes whose expressions or predicates contain whole-row Vars, and + * remember them for rebuilding. + */ +static void +RememberWholeRowIndexesForRebuilding(AlteredTableInfo *tab, Relation rel) +{ + Relation pg_index; + ScanKeyData key; + SysScanDesc scan; + HeapTuple indexTuple; + + pg_index = table_open(IndexRelationId, AccessShareLock); + + ScanKeyInit(&key, + Anum_pg_index_indrelid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(RelationGetRelid(rel))); + + scan = systable_beginscan(pg_index, IndexIndrelidIndexId, true, + NULL, 1, &key); + + while (HeapTupleIsValid(indexTuple = systable_getnext(scan))) + { + Form_pg_index index = (Form_pg_index) GETSTRUCT(indexTuple); + bool remember = false; + Datum exprDatum; + char *exprString; + bool isnull; + + exprDatum = fastgetattr(indexTuple, Anum_pg_index_indexprs, + RelationGetDescr(pg_index), &isnull); + if (!isnull) + { + exprString = TextDatumGetCString(exprDatum); + remember = expr_has_whole_row_var((Node *) stringToNode(exprString)); + pfree(exprString); + } + + if (!remember) + { + exprDatum = fastgetattr(indexTuple, Anum_pg_index_indpred, + RelationGetDescr(pg_index), &isnull); + if (!isnull) + { + exprString = TextDatumGetCString(exprDatum); + remember = expr_has_whole_row_var((Node *) stringToNode(exprString)); + pfree(exprString); + } + } + + if (remember) + RememberIndexForRebuilding(index->indexrelid, tab); + } + + systable_endscan(scan); + table_close(pg_index, AccessShareLock); +} + /* * Subroutine for ATExecAlterColumnType and ATExecSetExpression: Find everything * that depends on the column (constraints, indexes, etc), and record enough - * information to let us recreate the objects. + * information to let us recreate the objects. ATExecSetExpression can also + * pass attnum == 0 to find whole-row expression dependencies that observe a + * generated column's computed value. */ static void RememberAllDependentForRebuilding(AlteredTableInfo *tab, AlterTableType subtype, @@ -15305,6 +15399,7 @@ RememberAllDependentForRebuilding(AlteredTableInfo *tab, AlterTableType subtype, HeapTuple depTup; Assert(subtype == AT_AlterColumnType || subtype == AT_SetExpression); + Assert(attnum != 0 || subtype == AT_SetExpression); depRel = table_open(DependRelationId, RowExclusiveLock); @@ -15355,6 +15450,15 @@ RememberAllDependentForRebuilding(AlteredTableInfo *tab, AlterTableType subtype, } else { + /* + * In a whole-relation scan for SET EXPRESSION, ignore + * relation dependencies that are not indexes. Objects + * with expressions that need action, such as + * constraints, are handled by their own cases. + */ + if (attnum == 0) + break; + /* Not expecting any other direct dependencies... */ elog(ERROR, "unexpected object depending on column: %s", getObjectDescription(&foundObject, false)); @@ -15364,7 +15468,14 @@ RememberAllDependentForRebuilding(AlteredTableInfo *tab, AlterTableType subtype, case ConstraintRelationId: Assert(foundObject.objectSubId == 0); - RememberConstraintForRebuilding(foundObject.objectId, tab); + if (attnum == 0) + { + /* Whole-row CHECK constraints may need to be revalidated */ + if (get_constraint_type(foundObject.objectId) == CONSTRAINT_CHECK) + RememberConstraintForRebuilding(foundObject.objectId, tab); + } + else + RememberConstraintForRebuilding(foundObject.objectId, tab); break; case ProcedureRelationId: @@ -15505,8 +15616,14 @@ RememberAllDependentForRebuilding(AlteredTableInfo *tab, AlterTableType subtype, /* * We don't expect any other sorts of objects to depend on a - * column. + * column. A whole-relation scan can find the relation's row + * type, which doesn't need rebuilding for SET EXPRESSION. */ + if (attnum == 0 && + foundObject.classId == TypeRelationId && + get_typ_typrelid(foundObject.objectId) == RelationGetRelid(rel)) + continue; + elog(ERROR, "unexpected object depending on column: %s", getObjectDescription(&foundObject, false)); break; diff --git a/src/test/regress/expected/generated_stored.out b/src/test/regress/expected/generated_stored.out index 7866ae0ebbe..b6ec6819fd9 100644 --- a/src/test/regress/expected/generated_stored.out +++ b/src/test/regress/expected/generated_stored.out @@ -688,6 +688,17 @@ INSERT INTO gtest20c VALUES (1); -- ok INSERT INTO gtest20c VALUES (NULL); -- fails ERROR: new row for relation "gtest20c" violates check constraint "whole_row_check" DETAIL: Failing row contains (null, null). +ALTER TABLE gtest20c ALTER COLUMN b SET EXPRESSION AS (nullif(a, 1)); -- fails +ERROR: check constraint "whole_row_check" of relation "gtest20c" is violated by some row +CREATE INDEX gtest20c_expr_idx ON gtest20c ((gtest20c IS NOT NULL)); +SELECT pg_relation_filenode('gtest20c_expr_idx') AS gtest20c_expr_idx_filenode \gset +ALTER TABLE gtest20c ALTER COLUMN b SET EXPRESSION AS (a * 3); -- ok, rebuilds index +SELECT pg_relation_filenode('gtest20c_expr_idx') <> :gtest20c_expr_idx_filenode AS whole_row_index_rebuilt; + whole_row_index_rebuilt +------------------------- + t +(1 row) + -- not-null constraints CREATE TABLE gtest21a (a int PRIMARY KEY, b int GENERATED ALWAYS AS (nullif(a, 0)) STORED NOT NULL); INSERT INTO gtest21a (a) VALUES (1); -- ok diff --git a/src/test/regress/expected/generated_virtual.out b/src/test/regress/expected/generated_virtual.out index 24d5dbf46ca..fb6b8976af2 100644 --- a/src/test/regress/expected/generated_virtual.out +++ b/src/test/regress/expected/generated_virtual.out @@ -694,6 +694,33 @@ INSERT INTO gtest20c VALUES (1); -- ok INSERT INTO gtest20c VALUES (NULL); -- fails ERROR: new row for relation "gtest20c" violates check constraint "whole_row_check" DETAIL: Failing row contains (null, virtual). +ALTER TABLE gtest20c ALTER COLUMN b SET EXPRESSION AS (nullif(a, 1)); -- fails +ERROR: check constraint "whole_row_check" of relation "gtest20c" is violated by some row +CREATE INDEX gtest20c_expr_idx ON gtest20c ((gtest20c IS NOT NULL)); +CREATE INDEX gtest20c_pred_idx ON gtest20c (a) WHERE gtest20c = ROW(1, 2); +CREATE INDEX gtest20c_a_idx ON gtest20c (a); +SELECT pg_relation_filenode('gtest20c_expr_idx') AS gtest20c_expr_idx_filenode \gset +SELECT pg_relation_filenode('gtest20c_pred_idx') AS gtest20c_pred_idx_filenode \gset +SELECT pg_relation_filenode('gtest20c_a_idx') AS gtest20c_a_idx_filenode \gset +ALTER TABLE gtest20c ALTER COLUMN b SET EXPRESSION AS (a * 3); -- ok, rebuilds index +SELECT pg_relation_filenode('gtest20c_expr_idx') <> :gtest20c_expr_idx_filenode AS whole_row_index_rebuilt; + whole_row_index_rebuilt +------------------------- + t +(1 row) + +SELECT pg_relation_filenode('gtest20c_pred_idx') <> :gtest20c_pred_idx_filenode AS whole_row_predicate_index_rebuilt; + whole_row_predicate_index_rebuilt +----------------------------------- + t +(1 row) + +SELECT pg_relation_filenode('gtest20c_a_idx') = :gtest20c_a_idx_filenode AS ordinary_index_not_rebuilt; + ordinary_index_not_rebuilt +---------------------------- + t +(1 row) + -- not-null constraints CREATE TABLE gtest21a (a int PRIMARY KEY, b int GENERATED ALWAYS AS (nullif(a, 0)) VIRTUAL NOT NULL); INSERT INTO gtest21a (a) VALUES (1); -- ok diff --git a/src/test/regress/sql/generated_stored.sql b/src/test/regress/sql/generated_stored.sql index 6746cd4632b..24dc42545ad 100644 --- a/src/test/regress/sql/generated_stored.sql +++ b/src/test/regress/sql/generated_stored.sql @@ -341,6 +341,11 @@ CREATE TABLE gtest20c (a int, b int GENERATED ALWAYS AS (a * 2) STORED); ALTER TABLE gtest20c ADD CONSTRAINT whole_row_check CHECK (gtest20c IS NOT NULL); INSERT INTO gtest20c VALUES (1); -- ok INSERT INTO gtest20c VALUES (NULL); -- fails +ALTER TABLE gtest20c ALTER COLUMN b SET EXPRESSION AS (nullif(a, 1)); -- fails +CREATE INDEX gtest20c_expr_idx ON gtest20c ((gtest20c IS NOT NULL)); +SELECT pg_relation_filenode('gtest20c_expr_idx') AS gtest20c_expr_idx_filenode \gset +ALTER TABLE gtest20c ALTER COLUMN b SET EXPRESSION AS (a * 3); -- ok, rebuilds index +SELECT pg_relation_filenode('gtest20c_expr_idx') <> :gtest20c_expr_idx_filenode AS whole_row_index_rebuilt; -- not-null constraints CREATE TABLE gtest21a (a int PRIMARY KEY, b int GENERATED ALWAYS AS (nullif(a, 0)) STORED NOT NULL); diff --git a/src/test/regress/sql/generated_virtual.sql b/src/test/regress/sql/generated_virtual.sql index 9c2bb6590b3..26a14a9294a 100644 --- a/src/test/regress/sql/generated_virtual.sql +++ b/src/test/regress/sql/generated_virtual.sql @@ -347,6 +347,17 @@ CREATE TABLE gtest20c (a int, b int GENERATED ALWAYS AS (a * 2) VIRTUAL); ALTER TABLE gtest20c ADD CONSTRAINT whole_row_check CHECK (gtest20c IS NOT NULL); INSERT INTO gtest20c VALUES (1); -- ok INSERT INTO gtest20c VALUES (NULL); -- fails +ALTER TABLE gtest20c ALTER COLUMN b SET EXPRESSION AS (nullif(a, 1)); -- fails +CREATE INDEX gtest20c_expr_idx ON gtest20c ((gtest20c IS NOT NULL)); +CREATE INDEX gtest20c_pred_idx ON gtest20c (a) WHERE gtest20c = ROW(1, 2); +CREATE INDEX gtest20c_a_idx ON gtest20c (a); +SELECT pg_relation_filenode('gtest20c_expr_idx') AS gtest20c_expr_idx_filenode \gset +SELECT pg_relation_filenode('gtest20c_pred_idx') AS gtest20c_pred_idx_filenode \gset +SELECT pg_relation_filenode('gtest20c_a_idx') AS gtest20c_a_idx_filenode \gset +ALTER TABLE gtest20c ALTER COLUMN b SET EXPRESSION AS (a * 3); -- ok, rebuilds index +SELECT pg_relation_filenode('gtest20c_expr_idx') <> :gtest20c_expr_idx_filenode AS whole_row_index_rebuilt; +SELECT pg_relation_filenode('gtest20c_pred_idx') <> :gtest20c_pred_idx_filenode AS whole_row_predicate_index_rebuilt; +SELECT pg_relation_filenode('gtest20c_a_idx') = :gtest20c_a_idx_filenode AS ordinary_index_not_rebuilt; -- not-null constraints CREATE TABLE gtest21a (a int PRIMARY KEY, b int GENERATED ALWAYS AS (nullif(a, 0)) VIRTUAL NOT NULL); -- 2.50.1 (Apple Git-155) ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Fix SET EXPRESSION for virtual columns with whole-row dependencies @ 2026-06-13 02:42 jian he <[email protected]> parent: Chao Li <[email protected]> 1 sibling, 1 reply; 9+ messages in thread From: jian he @ 2026-06-13 02:42 UTC (permalink / raw) To: Chao Li <[email protected]>; +Cc: Postgres hackers <[email protected]>; Peter Eisentraut <[email protected]> On Wed, Jun 10, 2026 at 5:06 PM Chao Li <[email protected]> wrote: > > > After reading the other implementation in [1], I realized that I had missed the partial-index case, so I added coverage for that. > > I am still sending an updated version of this patch because my implementation is different from the one in [1]. I would like people to compare the two approaches and decide which direction is better. > I tried your patch before but abandoned it due to many regression test failures. RememberAllDependentForRebuilding { default: /* * We don't expect any other sorts of objects to depend on a * column. A whole-relation scan can find the relation's row * type, which doesn't need rebuilding for SET EXPRESSION. */ if (attnum == 0 && foundObject.classId == TypeRelationId && get_typ_typrelid(foundObject.objectId) == RelationGetRelid(rel)) continue; elog(ERROR, "unexpected object depending on column: %s", getObjectDescription(&foundObject, false)); break; } RememberAllDependentForRebuilding(tab, AT_SetExpression, rel, 0, NULL) scans for all dependencies on the relation (not just a specific column, since attnum=0). This is much broader than a column-level scan. The above DEFAULT branch in the SWITCH currently only exempts the relation's own row type (TypeRelationId). But a table can have many other kinds of dependents — indexes, triggers, foreign keys, views, etc. — and any of them that hitting this DEFAULT branch would incorrectly fire: elog(ERROR, "unexpected object depending on column: %s", ...) We need to check that every possible object type that can depend on a relation and ensure none of them fall through to that `elog(ERROR, ...)`. In RememberAllDependentForRebuilding, we have `if (subtype == AT_AlterColumnType)` guards against AT_SetExpression, — those branches silently skip AT_SetExpression, which is good. But the real risk is the DEFAULT branch, which lacks such a guard and will error on anything unexpected. ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Fix SET EXPRESSION for virtual columns with whole-row dependencies @ 2026-06-13 06:01 Chao Li <[email protected]> parent: jian he <[email protected]> 0 siblings, 1 reply; 9+ messages in thread From: Chao Li @ 2026-06-13 06:01 UTC (permalink / raw) To: jian he <[email protected]>; +Cc: Postgres hackers <[email protected]>; Peter Eisentraut <[email protected]> > On Jun 13, 2026, at 10:42, jian he <[email protected]> wrote: > > On Wed, Jun 10, 2026 at 5:06 PM Chao Li <[email protected]> wrote: >> >> >> After reading the other implementation in [1], I realized that I had missed the partial-index case, so I added coverage for that. >> >> I am still sending an updated version of this patch because my implementation is different from the one in [1]. I would like people to compare the two approaches and decide which direction is better. >> > > I tried your patch before but abandoned it due to many regression test failures. > Thanks for looking. But I wonder which tests I missed? I ran “make check-world” locally and passed. The CI tests all passed on the CF: https://commitfest.postgresql.org/patch/6867/ Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Fix SET EXPRESSION for virtual columns with whole-row dependencies @ 2026-06-15 06:43 jian he <[email protected]> parent: Chao Li <[email protected]> 0 siblings, 0 replies; 9+ messages in thread From: jian he @ 2026-06-15 06:43 UTC (permalink / raw) To: Chao Li <[email protected]>; +Cc: Postgres hackers <[email protected]>; Peter Eisentraut <[email protected]> On Sat, Jun 13, 2026 at 2:02 PM Chao Li <[email protected]> wrote: > > > On Jun 13, 2026, at 10:42, jian he <[email protected]> wrote: > > > > On Wed, Jun 10, 2026 at 5:06 PM Chao Li <[email protected]> wrote: > >> > >> > >> After reading the other implementation in [1], I realized that I had missed the partial-index case, so I added coverage for that. > >> > >> I am still sending an updated version of this patch because my implementation is different from the one in [1]. I would like people to compare the two approaches and decide which direction is better. > >> > > > > I tried your patch before but abandoned it due to many regression test failures. > > > > Thanks for looking. But I wonder which tests I missed? > > I ran “make check-world” locally and passed. The CI tests all passed on the CF: https://commitfest.postgresql.org/patch/6867/ > Apologies for the confusion. I meant that I actually tried the same idea earlier, but abandoned it after seeing many regression test failures. Your v2 patch passes all the regression tests because of the special handling added to the DEFAULT branch in RememberAllDependentForRebuilding. However, the point of my previous message was that I am worried this special handling below might not be bulletproof (if foundObject.classId is not TypeRelationId, then elog(ERROR) will be reached): /* * We don't expect any other sorts of objects to depend on a - * column. + * column. A whole-relation scan can find the relation's row + * type, which doesn't need rebuilding for SET EXPRESSION. */ + if (attnum == 0 && + foundObject.classId == TypeRelationId && + get_typ_typrelid(foundObject.objectId) == RelationGetRelid(rel)) + continue; + ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Fix SET EXPRESSION for virtual columns with whole-row dependencies @ 2026-07-08 18:20 Peter Eisentraut <[email protected]> parent: Chao Li <[email protected]> 0 siblings, 0 replies; 9+ messages in thread From: Peter Eisentraut @ 2026-07-08 18:20 UTC (permalink / raw) To: Chao Li <[email protected]>; jian he <[email protected]>; +Cc: Postgres hackers <[email protected]> On 10.06.26 11:12, Chao Li wrote: >> fter reading the other implementation in [1], I realized that I had missed the partial-index case, so I added coverage for that. >> >> I am still sending an updated version of this patch because my implementation is different from the one in [1]. I would like people to compare the two approaches and decide which direction is better. >> >> [1]https://postgr.es/m/ >> CAJTYsWXOkyeDVbzymWc9sKrq7Y_MUv6XJXN4H9GfsBOPd3NJ+w@mail.gmail.com >> >> Best regards, >> -- >> Chao Li (Evan) >> HighGo Software Co., Ltd. >> https://www.highgo.com/ >> >> > Oops! Forgot the attachment. Here comes it. This has been addressed via the patch in the other thread. ^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2026-07-08 18:20 UTC | newest] Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-06-09 06:22 Fix SET EXPRESSION for virtual columns with whole-row dependencies Chao Li <[email protected]> 2026-06-09 07:23 ` jian he <[email protected]> 2026-06-09 07:28 ` Chao Li <[email protected]> 2026-06-10 09:05 ` Chao Li <[email protected]> 2026-06-10 09:12 ` Chao Li <[email protected]> 2026-07-08 18:20 ` Peter Eisentraut <[email protected]> 2026-06-13 02:42 ` jian he <[email protected]> 2026-06-13 06:01 ` Chao Li <[email protected]> 2026-06-15 06:43 ` jian he <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox