Re: [PATCH v1] Fix for Bug#19724 - ALTER TYPE ... ALTER ATTRIBUTE triggers internal error for base type of domain with check

Hi,

I've reviewed and tested Nitin's v1 as well, and it works as expected.
The reported case and the variants I tried go through, and the rebuilt
constraint is still enforced. For a domain constraint the relid only
decides which relation gets locked and which work queue entry gets the
AT_ReAddDomainConstraint command, so tab->relid seems right to me.

On Mon Sep 28, 2026 at 11:31 AM -03, Ayush Tiwari wrote:
> Thanks for working on this. Switching to tab->relid looks right to me,
> since a domain constraint doesn't belong to any relation anyway.
>
> I think there's still a problem when a table that uses the domain gets
> rewritten by the same ALTER, though. The re-added constraint is
> validated right away in phase 2, before phase 3 has rewritten those
> tables, so their old rows are read with the new tuple descriptor. With
> the patch this crashes for me:
>

Yes, I found the same problem while testing v1, plus a second one.
Both date back to af20e2d72 and can already be hit on master with a
domain over a composite type, but with v1 they are reachable for any
domain whose check expression references a composite type.

1. Re-added domain constraints are validated before the rewrite.

As you describe, AT_ReAddDomainConstraint calls
AlterDomainAddConstraint(), which validates the constraint immediately,
while tables using the domain may still be pending a rewrite. A simple
way to see it:

create function show(int) returns bool language plpgsql as
$$ begin raise notice 'domain check sees value %', $1; return true; end $$;
create table t (c int);
create domain dt as int check (show(value) and (null::t).c is null);
alter table t add column d dt;
insert into t values (1, 5), (2, 7);
alter table t alter column c type bigint;
NOTICE: domain check sees value 0
NOTICE: domain check sees value 700

Depending on the data this gives garbage values, spurious "contains
values that violate the new constraint" errors, errors like "type with
OID 4294967295 does not exist" (domain over composite, on master), or
a crash like in your example.

2. Rebuilding the constraint requires ownership of the domain.

AlterDomainAddConstraint() calls checkDomainOwner(), so if another
user's domain has a constraint that depends on your type, you can't
alter your own type:

-- as alice
create type rt as (i int);
-- as bob
create domain dt as int check ((row(value)::rt).i > 0);
-- as alice
alter type rt alter attribute i type bigint;
ERROR: must be owner of type dt

Since types grant USAGE to PUBLIC by default, any user can create such
a dependency. The matching drop is done without permission checks, and
rebuilding another user's table CHECK constraint doesn't check
ownership either. A related case: if the constraint has a comment, it
is restored via CommentObject(), which also requires ownership. That
affects table constraints too, even without domains:

-- as bob
create table tt (x int constraint k check ((row(x)::rt).i > 0));
comment on constraint k on tt is 'hi';
-- as alice
alter type rt alter attribute i type bigint;
ERROR: must be owner of relation tt

> create table dtab (i int);
> create domain dtext as text check ((row(1)::dtab).i > 0 and md5(value) <>
> '');
> create table dtab_child (t dtext) inherits (dtab);
> insert into dtab_child values (1, repeat('x', 200));
> alter table dtab alter column i type bigint;
>
> On master it just fails with the elog. (FWIW master, and 14 too as far
> as I checked, can already crash like this with a domain over the
> parent's rowtype stored in the child, so it's not entirely new.)
>
> Maybe the constraint could be re-added as NOT VALID in phase 2, and then
> validated once phase 3 is done, e.g. with AlterDomainValidateConstraint()
> next to the FK checks at the end of ATRewriteTables()?
>

I considered that, but I think it's simpler to skip validation when
re-adding, remember the new constraint's OID in the work queue entry,
and call validateDomainCheckConstraint() on it once all tables have
been rewritten. My reasons:

- AlterDomainValidateConstraint() also calls checkDomainOwner(), so it
has issue 2 as well. Unlike AlterDomainAddConstraint() it has no
is_readd flag, so we'd need to add a new parameter to it too.

- It looks up the domain and the constraint by name. Using the OID
avoids resolving the names again in phase 3; ATPostAlterTypeCleanup()
already works from OIDs for similar reasons.

- We'd still need to remember which constraints were valid before the
rebuild, so that a NOT VALID constraint isn't validated. That's the
same bookkeeping as queuing the OID.

- The FK validation loop skips relations without storage. For ALTER TYPE
on a standalone composite type the only work queue entry is the type
itself, so validating there would silently skip it. For example, with
a stored value of 40000, changing an attribute from int to smallint
must still fail with "smallint out of range". The new loop on the
attached 0002 patch runs over the whole work queue for this reason.

One more thing I noticed, not addressed by these patches: if a domain
check uses ROW(value)::t and a column of the domain is later added to
t, the deparsed constraint becomes ROW(VALUE, NULL)::t. Re-parsing
that coerces the NULL to the domain itself, so the rebuilt constraint
refers to its own domain and fails with "stack depth limit exceeded".
The same happens with a hand-written ALTER DOMAIN ... ADD CONSTRAINT
using ROW(value, null)::t, so it's not specific to this code path,
which is why the tests use (null::t).c instead.

Attached are:

- v2-0001: Nitin's v1, unchanged.

- v2-0002: validate re-added domain constraints after the rewrites
(issue 1).

- v2-0003: don't require ownership when rebuilding constraints and their
comments (issue 2).

--
Matheus Alcantara
EDB: https://www.enterprisedb.com