Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Obtain required table lock during cross-table constraint updates.

  2. Add assertions that we hold some relevant lock during relation open.

  1. 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-28T07:00:02Z

    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.

  2. Re: BUG #18970: Atempt to alter type of table column used in row type with check leads to assertion failure

    jian he <jian.universality@gmail.com> — 2025-06-29T05:28:05Z

    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.

  3. Re: BUG #18970: Atempt to alter type of table column used in row type with check leads to assertion failure

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-06-29T17:01:53Z

    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

  4. Re: BUG #18970: Atempt to alter type of table column used in row type with check leads to assertion failure

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-06-29T17:32:40Z

    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

  5. Re: BUG #18970: Atempt to alter type of table column used in row type with check leads to assertion failure

    jian he <jian.universality@gmail.com> — 2025-06-30T01:47:31Z

    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.

  6. Re: BUG #18970: Atempt to alter type of table column used in row type with check leads to assertion failure

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-06-30T02:23:30Z

    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

  7. Re: BUG #18970: Atempt to alter type of table column used in row type with check leads to assertion failure

    jian he <jian.universality@gmail.com> — 2025-06-30T06:04:25Z

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

  8. Re: BUG #18970: Atempt to alter type of table column used in row type with check leads to assertion failure

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-06-30T15:14:43Z

    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

  9. Re: BUG #18970: Atempt to alter type of table column used in row type with check leads to assertion failure

    jian he <jian.universality@gmail.com> — 2025-07-01T03:25:20Z

    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.

  10. Re: BUG #18970: Atempt to alter type of table column used in row type with check leads to assertion failure

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-07-01T19:52:57Z

    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

  11. Re: BUG #18970: Atempt to alter type of table column used in row type with check leads to assertion failure

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-07-03T17:48:14Z

    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