pg.ddx.io  pgsql-bugs@postgresql.org mailing list archive  
help / color / mirror / Atom feed
BUG #18970: Atempt to alter type of table column used in row type with check leads to assertion failure
11+ messages / 3 participants
[nested] [flat]

* BUG #18970: Atempt to alter type of table column used in row type with check leads to assertion failure
@ 2025-06-28 07:00  PG Bug reporting form <noreply@postgresql.org>
  0 siblings, 1 reply; 11+ messages in thread

From: PG Bug reporting form @ 2025-06-28 07:00 UTC (permalink / raw)
  To: pgsql-bugs@lists.postgresql.org; +Cc: exclusion@gmail.com

The following bug has been logged on the website:

Bug reference:      18970
Logged by:          Alexander Lakhin
Email address:      exclusion@gmail.com
PostgreSQL version: 18beta1
Operating system:   Ubuntu 24.04
Description:        

The following script:
CREATE TABLE t1(a int);
CREATE TABLE t2(b t1 CHECK ((b).a IS NOT NULL));
ALTER TABLE t1 ALTER COLUMN a TYPE numeric;
triggers
2025-06-28 06:52:21.201 UTC [2233016] LOG:  statement: ALTER TABLE t1 ALTER
COLUMN a TYPE numeric;
TRAP: failed Assert("lockmode != NoLock || IsBootstrapProcessingMode() ||
CheckRelationLockedByMe(r, AccessShareLock, true)"), File: "relation.c",
Line: 67, PID: 2233016
ExceptionalCondition at assert.c:52:13
relation_open at relation.c:72:6
transformAlterTableStmt at parse_utilcmd.c:3543:8
ATPostAlterTypeParse at tablecmds.c:15600:20
ATPostAlterTypeCleanup at tablecmds.c:15478:3
ATRewriteCatalogs at tablecmds.c:5336:11
ATController at tablecmds.c:4882:2
AlterTable at tablecmds.c:4535:1
...
with an assert-enabled build, and fails with just
ERROR:  cannot alter table "t1" because column "t2.b" uses its row type
with no asserts.
Reproduced starting from commit b04aeb0a0, which added the Assert.



^ permalink  raw  reply  [nested|flat] 11+ messages in thread

* Re: BUG #18970: Atempt to alter type of table column used in row type with check leads to assertion failure
@ 2025-06-29 05:28  jian he <jian.universality@gmail.com>
  parent: PG Bug reporting form <noreply@postgresql.org>
  0 siblings, 1 reply; 11+ messages in thread

From: jian he @ 2025-06-29 05:28 UTC (permalink / raw)
  To: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org

On Sun, Jun 29, 2025 at 1:35 AM PG Bug reporting form
<noreply@postgresql.org> wrote:
>
> The following bug has been logged on the website:
>
> Bug reference:      18970
> Logged by:          Alexander Lakhin
> Email address:      exclusion@gmail.com
> PostgreSQL version: 18beta1
> Operating system:   Ubuntu 24.04
> Description:
>
> The following script:
> CREATE TABLE t1(a int);
> CREATE TABLE t2(b t1 CHECK ((b).a IS NOT NULL));
> ALTER TABLE t1 ALTER COLUMN a TYPE numeric;
> triggers
> 2025-06-28 06:52:21.201 UTC [2233016] LOG:  statement: ALTER TABLE t1 ALTER
> COLUMN a TYPE numeric;
> TRAP: failed Assert("lockmode != NoLock || IsBootstrapProcessingMode() ||
> CheckRelationLockedByMe(r, AccessShareLock, true)"), File: "relation.c",
> Line: 67, PID: 2233016
> ExceptionalCondition at assert.c:52:13
> relation_open at relation.c:72:6
> transformAlterTableStmt at parse_utilcmd.c:3543:8
> ATPostAlterTypeParse at tablecmds.c:15600:20
> ATPostAlterTypeCleanup at tablecmds.c:15478:3
> ATRewriteCatalogs at tablecmds.c:5336:11
> ATController at tablecmds.c:4882:2
> AlterTable at tablecmds.c:4535:1
> ...
> with an assert-enabled build, and fails with just
> ERROR:  cannot alter table "t1" because column "t2.b" uses its row type
> with no asserts.
> Reproduced starting from commit b04aeb0a0, which added the Assert.
>

hi.
this bug can be triggered by exclusion constraints too.

drop table if exists t1,t2;
CREATE TABLE t1(a int);
CREATE TABLE t2(b t1);
ALTER TABLE t2 ADD CONSTRAINT xxn EXCLUDE USING btree (((b).a) WITH =);
ALTER TABLE t1 ALTER COLUMN a TYPE numeric;

in ATPostAlterTypeCleanup
        /*
         * When rebuilding an FK constraint that references the table we're
         * modifying, we might not yet have any lock on the FK's table, so get
         * one now.  We'll need AccessExclusiveLock for the DROP CONSTRAINT
         * step, so there's no value in asking for anything weaker.
         */
        if (relid != tab->relid && contype == CONSTRAINT_FOREIGN)
            LockRelationOid(relid, AccessExclusiveLock);

we can change to
        if (relid != tab->relid)
            LockRelationOid(relid, AccessExclusiveLock);
obviously, the comments need to be updated.

When altering the data type of a column in one relation causes a constraint of
another table rebuild, the other table should be locked with
AccessExclusiveLock.  but in ATPostAlterTypeCleanup, we only know that
tab->relid is locked with the specified lock mode.
RememberConstraintForRebuilding only records the constraint information—it
doesn't acquire a lock on pg_constraint.conrelid.

so in ATPostAlterTypeCleanup, we should lock the tab->changedConstraintOids
associated pg_constraint.conrelid.





^ permalink  raw  reply  [nested|flat] 11+ messages in thread

* Re: BUG #18970: Atempt to alter type of table column used in row type with check leads to assertion failure
@ 2025-06-29 17:01  Tom Lane <tgl@sss.pgh.pa.us>
  parent: jian he <jian.universality@gmail.com>
  0 siblings, 1 reply; 11+ messages in thread

From: Tom Lane @ 2025-06-29 17:01 UTC (permalink / raw)
  To: jian he <jian.universality@gmail.com>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org

jian he <jian.universality@gmail.com> writes:
> On Sun, Jun 29, 2025 at 1:35 AM PG Bug reporting form
> <noreply@postgresql.org> wrote:
>> CREATE TABLE t1(a int);
>> CREATE TABLE t2(b t1 CHECK ((b).a IS NOT NULL));
>> ALTER TABLE t1 ALTER COLUMN a TYPE numeric;
>> triggers
>> 2025-06-28 06:52:21.201 UTC [2233016] LOG:  statement: ALTER TABLE t1 ALTER
>> COLUMN a TYPE numeric;
>> TRAP: failed Assert("lockmode != NoLock || IsBootstrapProcessingMode() ||
>> CheckRelationLockedByMe(r, AccessShareLock, true)"), File: "relation.c",
>> Line: 67, PID: 2233016

> in ATPostAlterTypeCleanup
>         /*
>          * When rebuilding an FK constraint that references the table we're
>          * modifying, we might not yet have any lock on the FK's table, so get
>          * one now.  We'll need AccessExclusiveLock for the DROP CONSTRAINT
>          * step, so there's no value in asking for anything weaker.
>          */
>         if (relid != tab->relid && contype == CONSTRAINT_FOREIGN)
>             LockRelationOid(relid, AccessExclusiveLock);

> we can change to
>         if (relid != tab->relid)
>             LockRelationOid(relid, AccessExclusiveLock);
> obviously, the comments need to be updated.

Yeah, I came to the same conclusion after studying it for awhile.
The problem is that because of the "(b).a" column reference in
the CHECK expression, we record a direct dependency of t2's CHECK
constraint on t1.a, and ATPostAlterTypeCleanup is evidently not
expecting that.

> When altering the data type of a column in one relation causes a constraint of
> another table rebuild, the other table should be locked with
> AccessExclusiveLock.

I wonder if there might be cases where a lesser lock is sufficient.
However, this is apparently a very edgy edge case, so it's probably
not worth obsessing over the lock level too much.

It's somewhat annoying that we're just going to fail later.
I thought about whether find_composite_type_dependencies
ought to be run sooner, so that we'd error out before potentially
doing a lot of work.  But I think the design idea is that
someday find_composite_type_dependencies would actually propagate
the changes, in which case its current placement is correct.

			regards, tom lane





^ permalink  raw  reply  [nested|flat] 11+ messages in thread

* Re: BUG #18970: Atempt to alter type of table column used in row type with check leads to assertion failure
@ 2025-06-29 17:32  Tom Lane <tgl@sss.pgh.pa.us>
  parent: Tom Lane <tgl@sss.pgh.pa.us>
  0 siblings, 1 reply; 11+ messages in thread

From: Tom Lane @ 2025-06-29 17:32 UTC (permalink / raw)
  To: jian he <jian.universality@gmail.com>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org

I wrote:
> jian he <jian.universality@gmail.com> writes:
>> we can change to
>> 	if (relid != tab->relid)
>> 		LockRelationOid(relid, AccessExclusiveLock);
>> obviously, the comments need to be updated.

> Yeah, I came to the same conclusion after studying it for awhile.

Here's a patch for that.  Working on back-patching it ...

			regards, tom lane

Attachments:

  [text/x-diff] fix-bug-18970.patch (4.2K, ../../2537374.1751218360@sss.pgh.pa.us/2-fix-bug-18970.patch)
  download | inline diff:
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 991bc946ffc..b8837f26cb4 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -15415,9 +15415,12 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
 	/*
 	 * Re-parse the index and constraint definitions, and attach them to the
 	 * appropriate work queue entries.  We do this before dropping because in
-	 * the case of a FOREIGN KEY constraint, we might not yet have exclusive
-	 * lock on the table the constraint is attached to, and we need to get
-	 * that before reparsing/dropping.
+	 * the case of a constraint on another table, we might not yet have
+	 * exclusive lock on the table the constraint is attached to, and we need
+	 * to get that before reparsing/dropping.  (That's possible at least for
+	 * FOREIGN KEY, CHECK, and EXCLUSION constraints; in non-FK cases it
+	 * requires a dependency on the target table's composite type in the other
+	 * table's constraint expressions.)
 	 *
 	 * We can't rely on the output of deparsing to tell us which relation to
 	 * operate on, because concurrent activity might have made the name
@@ -15433,7 +15436,6 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
 		Form_pg_constraint con;
 		Oid			relid;
 		Oid			confrelid;
-		char		contype;
 		bool		conislocal;
 
 		tup = SearchSysCache1(CONSTROID, ObjectIdGetDatum(oldId));
@@ -15450,7 +15452,6 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
 				elog(ERROR, "could not identify relation associated with constraint %u", oldId);
 		}
 		confrelid = con->confrelid;
-		contype = con->contype;
 		conislocal = con->conislocal;
 		ReleaseSysCache(tup);
 
@@ -15468,12 +15469,12 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
 			continue;
 
 		/*
-		 * When rebuilding an FK constraint that references the table we're
-		 * modifying, we might not yet have any lock on the FK's table, so get
-		 * one now.  We'll need AccessExclusiveLock for the DROP CONSTRAINT
-		 * step, so there's no value in asking for anything weaker.
+		 * When rebuilding another table's constraint that references the
+		 * table we're modifying, we might not yet have any lock on the other
+		 * table, so get one now.  We'll need AccessExclusiveLock for the DROP
+		 * CONSTRAINT step, so there's no value in asking for anything weaker.
 		 */
-		if (relid != tab->relid && contype == CONSTRAINT_FOREIGN)
+		if (relid != tab->relid)
 			LockRelationOid(relid, AccessExclusiveLock);
 
 		ATPostAlterTypeParse(oldId, relid, confrelid,
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 476266e3f4b..750efc042d8 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -4745,6 +4745,13 @@ alter table attbl alter column p1 set data type bigint;
 alter table atref alter column c1 set data type bigint;
 drop table attbl, atref;
 /* End test case for bug #17409 */
+/* Test case for bug #18970 */
+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
+drop table attbl, atref;
+/* End test case for bug #18970 */
 -- Test that ALTER TABLE rewrite preserves a clustered index
 -- for normal indexes and indexes on constraints.
 create table alttype_cluster (a int);
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 5ce9d1e429f..41cff198e18 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -3069,6 +3069,15 @@ drop table attbl, atref;
 
 /* End test case for bug #17409 */
 
+/* Test case for bug #18970 */
+
+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
+drop table attbl, atref;
+
+/* End test case for bug #18970 */
+
 -- Test that ALTER TABLE rewrite preserves a clustered index
 -- for normal indexes and indexes on constraints.
 create table alttype_cluster (a int);

^ permalink  raw  reply  [nested|flat] 11+ messages in thread

* Re: BUG #18970: Atempt to alter type of table column used in row type with check leads to assertion failure
@ 2025-06-30 01:47  jian he <jian.universality@gmail.com>
  parent: Tom Lane <tgl@sss.pgh.pa.us>
  0 siblings, 1 reply; 11+ messages in thread

From: jian he @ 2025-06-30 01:47 UTC (permalink / raw)
  To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org

On Mon, Jun 30, 2025 at 1:32 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
> > jian he <jian.universality@gmail.com> writes:
> >> we can change to
> >>      if (relid != tab->relid)
> >>              LockRelationOid(relid, AccessExclusiveLock);
> >> obviously, the comments need to be updated.
>
> > Yeah, I came to the same conclusion after studying it for awhile.
>

This issue also applies to tab->changedStatisticsOids
and tab->changedIndexOids in ATPostAlterTypeCleanup.

drop table if exists t1,t2;
CREATE TABLE t1(a int);
CREATE TABLE t2(b t1);
CREATE STATISTICS XXX ON ((b).a is not null) FROM t2;
ALTER TABLE t1 ALTER COLUMN a TYPE numeric;


drop table if exists t1,t2;
CREATE TABLE t1(a int);
CREATE TABLE t2(b t1);
CREATE INDEX XXX ON t2(((b).a));
ALTER TABLE t1 ALTER COLUMN a TYPE numeric;

We likely need to do the same as well.





^ permalink  raw  reply  [nested|flat] 11+ messages in thread

* Re: BUG #18970: Atempt to alter type of table column used in row type with check leads to assertion failure
@ 2025-06-30 02:23  Tom Lane <tgl@sss.pgh.pa.us>
  parent: jian he <jian.universality@gmail.com>
  0 siblings, 1 reply; 11+ messages in thread

From: Tom Lane @ 2025-06-30 02:23 UTC (permalink / raw)
  To: jian he <jian.universality@gmail.com>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org

jian he <jian.universality@gmail.com> writes:
> This issue also applies to tab->changedStatisticsOids
> and tab->changedIndexOids in ATPostAlterTypeCleanup.

Oh, indeed :-(

> We likely need to do the same as well.

I'm about done for the day --- you want to prepare the patch?

			regards, tom lane





^ permalink  raw  reply  [nested|flat] 11+ messages in thread

* Re: BUG #18970: Atempt to alter type of table column used in row type with check leads to assertion failure
@ 2025-06-30 06:04  jian he <jian.universality@gmail.com>
  parent: Tom Lane <tgl@sss.pgh.pa.us>
  0 siblings, 1 reply; 11+ messages in thread

From: jian he @ 2025-06-30 06:04 UTC (permalink / raw)
  To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org

On Mon, Jun 30, 2025 at 10:23 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> jian he <jian.universality@gmail.com> writes:
> > This issue also applies to tab->changedStatisticsOids
> > and tab->changedIndexOids in ATPostAlterTypeCleanup.
>
> Oh, indeed :-(
>
> > We likely need to do the same as well.
>
> I'm about done for the day --- you want to prepare the patch?
>
hi.

I added 2 more error case tests in alter_table.sql

SET EXPRESSION both can have this issue.
so i also add more test cases for stored/virtual generated columns.

ALTER TYPE | SET EXPRESSION
trigger related related index build, we use AccessExclusiveLock to
lock the related relation
trigger related related statistic build, we use
ShareUpdateExclusiveLock to lock the related relation

-----------------the following are attached patch commit messages
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.
----------------

Attachments:

  [text/x-patch] v2-0001-fix-bug-18970.patch (13.4K, ../../CACJufxH2NDhi4nTx_csqBao9i4bfxCyCXi7B8G=_XnWu8ru2TQ@mail.gmail.com/2-v2-0001-fix-bug-18970.patch)
  download | inline diff:
From 3e26eeeff901dae96b1da9f33e1b5f70cfac9169 Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
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



^ permalink  raw  reply  [nested|flat] 11+ messages in thread

* Re: BUG #18970: Atempt to alter type of table column used in row type with check leads to assertion failure
@ 2025-06-30 15:14  Tom Lane <tgl@sss.pgh.pa.us>
  parent: jian he <jian.universality@gmail.com>
  0 siblings, 1 reply; 11+ messages in thread

From: Tom Lane @ 2025-06-30 15:14 UTC (permalink / raw)
  To: jian he <jian.universality@gmail.com>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org

jian he <jian.universality@gmail.com> writes:
> SET EXPRESSION both can have this issue.
> so i also add more test cases for stored/virtual generated columns.

This patch is a lot larger than I was expecting, and I think it's
misguided.  You argue that

+     * 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.

but I think that is in fact wrong.  The implementation restriction
we have is that we lack code to run around and physically change
the stored values of columns that are not top-level table columns.
However, in the case of a virtual column we don't need to change
anything about the storage: once we've fixed the catalog metadata,
we're done.  So I'm not seeing the need to add all this stuff
other than the additional locking calls.

			regards, tom lane





^ permalink  raw  reply  [nested|flat] 11+ messages in thread

* Re: BUG #18970: Atempt to alter type of table column used in row type with check leads to assertion failure
@ 2025-07-01 03:25  jian he <jian.universality@gmail.com>
  parent: Tom Lane <tgl@sss.pgh.pa.us>
  0 siblings, 1 reply; 11+ messages in thread

From: jian he @ 2025-07-01 03:25 UTC (permalink / raw)
  To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org

On Mon, Jun 30, 2025 at 11:14 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> jian he <jian.universality@gmail.com> writes:
> > SET EXPRESSION both can have this issue.
> > so i also add more test cases for stored/virtual generated columns.
>
> This patch is a lot larger than I was expecting, and I think it's
> misguided.  You argue that
>
> +     * 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.
>
> but I think that is in fact wrong.  The implementation restriction
> we have is that we lack code to run around and physically change
> the stored values of columns that are not top-level table columns.
> However, in the case of a virtual column we don't need to change
> anything about the storage: once we've fixed the catalog metadata,
> we're done.  So I'm not seeing the need to add all this stuff
> other than the additional locking calls.
>

CREATE TABLE t1(a int generated always as ('1') stored);
CREATE TABLE t2(b t1 CHECK ((b).a IS NOT NULL));
INSERT INTO t2 default values returning *;
ERROR:  new row for relation "t2" violates check constraint "t2_b_check"
DETAIL:  Failing row contains (null).
INSERT INTO t2 values '(1)' returning *;

ALTER TABLE t1 ALTER COLUMN a SET EXPRESSION AS (NULL);
Currently we disallow this "SET EXPRESSION AS".
I am wondering why we disallow it?

at that time, I was mainly initrucugged by this comment about
composite type default
in ATRewriteTables:
         * 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.


I’ve added some tests to generated_stored.sql, but not to generated_virtual.sql.

Attachments:

  [text/x-patch] v3-0001-fix-bug-18970.patch (6.7K, ../../CACJufxEZ2DzyAKNBLqW+-2BqFAbT5Wnxkgkw6ogcbBBsndkD=Q@mail.gmail.com/2-v3-0001-fix-bug-18970.patch)
  download | inline diff:
From d368f253b2bd0c1dfe61ee9c9d6ffa88940bbe9a Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
Date: Tue, 1 Jul 2025 11:17:11 +0800
Subject: [PATCH v3 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.

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               | 12 ++++++++++++
 src/test/regress/expected/alter_table.out      |  8 ++++++++
 src/test/regress/expected/generated_stored.out | 12 ++++++++++++
 src/test/regress/sql/alter_table.sql           |  8 ++++++++
 src/test/regress/sql/generated_stored.sql      | 13 +++++++++++++
 5 files changed, 53 insertions(+)

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index b8837f26cb4..f174a14d86b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -15488,6 +15488,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 +15507,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..373a37b3838 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/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..16ce1142058 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
-- 
2.34.1



^ permalink  raw  reply  [nested|flat] 11+ messages in thread

* Re: BUG #18970: Atempt to alter type of table column used in row type with check leads to assertion failure
@ 2025-07-01 19:52  Tom Lane <tgl@sss.pgh.pa.us>
  parent: jian he <jian.universality@gmail.com>
  0 siblings, 1 reply; 11+ messages in thread

From: Tom Lane @ 2025-07-01 19:52 UTC (permalink / raw)
  To: jian he <jian.universality@gmail.com>; +Cc: Peter Eisentraut <peter@eisentraut.org>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org

jian he <jian.universality@gmail.com> writes:
> On Mon, Jun 30, 2025 at 11:14 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> ... I think that is in fact wrong.  The implementation restriction
>> we have is that we lack code to run around and physically change
>> the stored values of columns that are not top-level table columns.
>> However, in the case of a virtual column we don't need to change
>> anything about the storage: once we've fixed the catalog metadata,
>> we're done.  So I'm not seeing the need to add all this stuff
>> other than the additional locking calls.

> CREATE TABLE t1(a int generated always as ('1') stored);
> CREATE TABLE t2(b t1 CHECK ((b).a IS NOT NULL));
> INSERT INTO t2 default values returning *;
> ERROR:  new row for relation "t2" violates check constraint "t2_b_check"
> DETAIL:  Failing row contains (null).
> INSERT INTO t2 values '(1)' returning *;

> ALTER TABLE t1 ALTER COLUMN a SET EXPRESSION AS (NULL);
> Currently we disallow this "SET EXPRESSION AS".
> I am wondering why we disallow it?

That is an interesting question.  AFAICT we treat the GENERATED
clause as being like a constraint.  We don't apply constraints
of a table to uses of its composite type, so in the above example
t2's b.a does not pay any attention to the GENERATED clause; it
just acts as an ordinary "int" column.  So it's not very clear
why we should forbid ALTER COLUMN a SET EXPRESSION because of t2.

If the idea was to leave the door open for a future change of
that behavior (that is, to start propagating the GENERATED clause to
uses of the composite type), I think that ship has already sailed.

So, rather than add a new restriction for VIRTUAL generated
columns, I'd be inclined to remove the one for STORED columns.
At least if the argument is to make them act the same.  If we're
okay with them working differently then we don't have to do
anything.

> at that time, I was mainly initrucugged by this comment about
> composite type default
> in ATRewriteTables:
>          * 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.

That (propagating defaults to composite types) again strikes me as a
ship that sailed long ago.  Quite aside from the implementation issues
that would arise, we'd be changing longstanding behavior.

Peter, what's your opinion?

			regards, tom lane





^ permalink  raw  reply  [nested|flat] 11+ messages in thread

* Re: BUG #18970: Atempt to alter type of table column used in row type with check leads to assertion failure
@ 2025-07-03 17:48  Tom Lane <tgl@sss.pgh.pa.us>
  parent: Tom Lane <tgl@sss.pgh.pa.us>
  0 siblings, 0 replies; 11+ messages in thread

From: Tom Lane @ 2025-07-03 17:48 UTC (permalink / raw)
  To: jian he <jian.universality@gmail.com>; +Cc: Peter Eisentraut <peter@eisentraut.org>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org

For the moment I've just gone ahead and pushed the bug fix,
with test cases that demonstrate the inconsistency in
behavior between stored and virtual generated columns.
Whether we should change that is probably a matter for
a separate thread.

			regards, tom lane





^ permalink  raw  reply  [nested|flat] 11+ messages in thread


end of thread, other threads:[~2025-07-03 17:48 UTC | newest]

Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-06-28 07:00 BUG #18970: Atempt to alter type of table column used in row type with check leads to assertion failure PG Bug reporting form <noreply@postgresql.org>
2025-06-29 05:28 ` jian he <jian.universality@gmail.com>
2025-06-29 17:01   ` Tom Lane <tgl@sss.pgh.pa.us>
2025-06-29 17:32     ` Tom Lane <tgl@sss.pgh.pa.us>
2025-06-30 01:47       ` jian he <jian.universality@gmail.com>
2025-06-30 02:23         ` Tom Lane <tgl@sss.pgh.pa.us>
2025-06-30 06:04           ` jian he <jian.universality@gmail.com>
2025-06-30 15:14             ` Tom Lane <tgl@sss.pgh.pa.us>
2025-07-01 03:25               ` jian he <jian.universality@gmail.com>
2025-07-01 19:52                 ` Tom Lane <tgl@sss.pgh.pa.us>
2025-07-03 17:48                   ` Tom Lane <tgl@sss.pgh.pa.us>

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox