Thank you for reviewing the patch!
On 28/09/26 20:02, Zsolt Parragi wrote:
> Hello
>
> + /*
> + * Check it's a domain and check user has permission for ALTER DOMAIN.
> + * When re-adding a constraint during ALTER TABLE, skip the permission
> + * check since the constraint already existed, and the user altering a
> + * column it depends on need not own the domain.
> + */
> + if (is_readd)
> + Assert(typTup->typtype == TYPTYPE_DOMAIN);
> + else
> + checkDomainOwner(tup);
>
> That assertion can fire with two concurrent sessions, it should be a
> proper error message, similar to what's inside checkDomainOwner.
>
> See the following isolation test:
>
> setup
> {
> CREATE TYPE ct AS (i int);
> CREATE DOMAIN d AS ct CONSTRAINT d_check CHECK ((VALUE).i > 0);
> CREATE TABLE t2 (x int CONSTRAINT t2_check CHECK ((row(x)::ct).i > 0));
> INSERT INTO t2 VALUES (1);
> }
>
> teardown
> {
> DROP TABLE IF EXISTS t2;
> DROP TYPE IF EXISTS d CASCADE;
> DROP DOMAIN IF EXISTS d_old CASCADE;
> DROP TYPE IF EXISTS ct CASCADE;
> }
>
> session s1
> step a_alter { ALTER TYPE ct ALTER ATTRIBUTE i TYPE bigint; }
>
> session s2
> step b_begin { BEGIN; SELECT count(*) FROM t2; }
> step b_commit { COMMIT; }
>
> session s3
> step c_swap { ALTER DOMAIN d RENAME TO d_old; CREATE TYPE d AS (z int); }
>
> permutation b_begin a_alter c_swap b_commit
>
>
Good catch. I've changed to use ereport like checkDomainOwner().
But I don't think that's enough. The underlying issue is that the re-add
looks the domain up by the name saved in its definition, so the name can
point to a different type by the time the constraint is re-added. In
your isolation test example, if c_swap creates a new domain instead
(CREATE DOMAIN d AS ct), the type check passes, the ALTER succeeds, and
d_check silently ends up on the new domain, while the original (now
d_old) loses it.
Note that this isn't new with the patch. What 0003 changes is that
without the ownership check, it also works when the domain belongs to
someone other than the user running the ALTER.
We may try to capture the domain oid above AlterDomainAddConstraint,
while the old constraint still exists and re-add the constraint to that
OID instead of resolving the name again. But I think that it will
require more code to write which would make it harder for back patching.
Looking for thoughts here.
> + if (!con->skip_validation)
> + tab->domain_constraints =
> + lappend_oid(tab->domain_constraints,
> + constrAddr.objectId);
>
> This can be uninitialized, AlterDomainAddConstraint doesn't guarantee
> a write. I think this could use both a Assert(con->contype ==
> CONSTR_CHECK); and initalizating constrAddr to InvalidObjectAddress.
Fixed.
--
Matheus Alcantara
EDB: https://www.enterprisedb.com
