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

Hi Nitin,

I reviewed and tested v1. The fix looks right to me. For a domain
constraint, the relation OID in ATPostAlterTypeCleanup() only decides
whether another relation gets locked and which work-queue entry gets
the AT_ReAddDomainConstraint command. The AlterDomainStmt names the
domain itself, so using tab->relid is fine.

Testing (macOS arm64, meson debug build with assertions, master at
ad36e3608c plus v1): v1 applies cleanly with git am, and the
regression tests pass. I also ran these cases on master and on the
patched build. Master fails on all of them with "could not identify
relation associated with constraint". With v1 the type change goes
through and the constraint is rebuilt:

- ALTER TABLE ... ALTER COLUMN ... TYPE on a table whose row type is
used in the check of a domain over int
- a domain over int[], and a domain over a domain over int
- a typed table, with ALTER TYPE ... CASCADE
- one domain check that uses both a parent's and a child's row type,
with ALTER TABLE on the parent
- a table column of the domain type with data in it (the constraint
is revalidated)
- a comment on the constraint (kept) and a NOT VALID constraint
(stays NOT VALID)

Where I checked, the rebuilt constraint still rejects bad values.

Comments:

1. The bug isn't specific to ALTER TYPE. ALTER TABLE ... ALTER
COLUMN ... TYPE hits it too, as in the first case above. I'd add
a test for that and mention it in the commit message.

2. The commit message says the domain is "defined over a scalar
type". Arrays and domains over domains are affected too, so "a
domain whose base type isn't composite" would be more accurate.

3. In the tests, I'd describe the float8 case instead of referring to
"Tom Lane's 2017 email".

4. One side effect: for a domain over a different composite type, as
in

CREATE DOMAIN dd AS r2 CHECK ((ROW((VALUE).b)::r1).a > 0);
ALTER TYPE r1 ALTER ATTRIBUTE a TYPE bigint;

master takes an AccessExclusiveLock on r2 and queues the rebuild
there, while v1 does neither. I think that's fine, since the
rebuilt constraint belongs to the domain and doesn't touch r2.

5. af20e2d72 went into v11, so every supported branch has this bug.
v1, tests included, applies cleanly to REL_14_STABLE through
REL_19_STABLE, so +1 for back-patching.

I couldn't find this in the open commitfest (PG20-3). Could you add
it? I'm happy to be listed as a reviewer.

Regards,
Rahul Yadav