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