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

Hi,

On Tue, 29 Sept 2026 at 00:15, Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
>
>
> 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.
> >
> > 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).

Thanks, the split looks right to me.

On 0002, remembering the new constraint OID and calling
validateDomainCheckConstraint() directly is better than what I suggested.
Using the OID avoids resolving the domain and constraint by name again in
phase 3, and the standalone-composite case needs the new loop not to skip
relations without storage.  One small thing: the new loop doesn't
CommandCounterIncrement() between constraints.  Probably fine today, but
the FK loop and afterStmts do. And I think 0001 and 0002 can be clubbed
together (though that can be done whilst committing)

I've only looked closely at 0001 and 0002 so far, which fix the reported
case for me across branches.  I'll come back on 0003.

Regards,
Ayush