public inbox for [email protected]
help / color / mirror / Atom feedFrom: Chao Li <[email protected]>
To: jian he <[email protected]>
Cc: Postgres hackers <[email protected]>
Cc: Peter Eisentraut <[email protected]>
Subject: Re: Fix SET EXPRESSION for virtual columns with whole-row dependencies
Date: Wed, 10 Jun 2026 17:12:58 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<CACJufxHTK0Qk7AwdrHK=y8OtK--zEVY-Ossxx4UUgxaE2NCAXA@mail.gmail.com>
<[email protected]>
<[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)
view thread (9+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Fix SET EXPRESSION for virtual columns with whole-row dependencies
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox