From 2cfd1767caf396cd6c2a64732447406aa04f58c2 Mon Sep 17 00:00:00 2001 From: "Chao Li (Evan)" Date: Mon, 8 Jun 2026 11:07:15 +0800 Subject: [PATCH v3] Fix ALTER DOMAIN VALIDATE CONSTRAINT locking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ALTER DOMAIN ... VALIDATE CONSTRAINT must wait for already-running DML commands on tables using the domain. Those commands may have initialized domain constraint checks before a NOT VALID constraint was added, so they can still insert or update rows that violate the new constraint. Commit 16a0039dc0d1 reduced the related-relation lock level to ShareUpdateExclusiveLock and refactored validateDomainCheckConstraint() to accept a lock mode. That lock-level reduction is unsafe for DML that was already running before the NOT VALID constraint was added. Manually revert 16a0039dc0d1 so validateDomainCheckConstraint() again always uses ShareLock for relations using the domain. Keep the later a99c6b56f behavior that validating an already-validated constraint is a no-op. Add an isolation test for the race, including a check that convalidated remains false when validation fails after waiting for the already-running DML. Author: Chao Li Reviewed-by: Fujii Masao Reviewed-by: Álvaro Herrera Discussion: https://postgr.es/m/463C0E1A-4A40-4BCA-839C-9236B80D65EE@gmail.com --- src/backend/commands/typecmds.c | 24 +++++---------- .../expected/alter-domain-validate.out | 23 ++++++++++++++ src/test/isolation/isolation_schedule | 1 + .../specs/alter-domain-validate.spec | 30 +++++++++++++++++++ 4 files changed, 61 insertions(+), 17 deletions(-) create mode 100644 src/test/isolation/expected/alter-domain-validate.out create mode 100644 src/test/isolation/specs/alter-domain-validate.spec diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c index c4c3cdb5461..e9c3215ccec 100644 --- a/src/backend/commands/typecmds.c +++ b/src/backend/commands/typecmds.c @@ -128,7 +128,7 @@ static Oid findTypeSubscriptingFunction(List *procname, Oid typeOid); static Oid findRangeSubOpclass(List *opcname, Oid subtype); static Oid findRangeCanonicalFunction(List *procname, Oid typeOid); static Oid findRangeSubtypeDiffFunction(List *procname, Oid subtype); -static void validateDomainCheckConstraint(Oid domainoid, const char *ccbin, LOCKMODE lockmode); +static void validateDomainCheckConstraint(Oid domainoid, const char *ccbin); static void validateDomainNotNullConstraint(Oid domainoid); static List *get_rels_with_domain(Oid domainOid, LOCKMODE lockmode); static void checkEnumOwner(HeapTuple tup); @@ -3018,7 +3018,7 @@ AlterDomainAddConstraint(List *names, Node *newConstraint, * to. */ if (!constr->skip_validation) - validateDomainCheckConstraint(domainoid, ccbin, ShareLock); + validateDomainCheckConstraint(domainoid, ccbin); /* * We must send out an sinval message for the domain, to ensure that @@ -3135,12 +3135,7 @@ AlterDomainValidateConstraint(List *names, const char *constrName) val = SysCacheGetAttrNotNull(CONSTROID, tuple, Anum_pg_constraint_conbin); conbin = TextDatumGetCString(val); - /* - * Locking related relations with ShareUpdateExclusiveLock is ok - * because not-yet-valid constraints are still enforced against - * concurrent inserts or updates. - */ - validateDomainCheckConstraint(domainoid, conbin, ShareUpdateExclusiveLock); + validateDomainCheckConstraint(domainoid, conbin); /* * Now update the catalog, while we have the door open. @@ -3235,16 +3230,9 @@ validateDomainNotNullConstraint(Oid domainoid) /* * Verify that all columns currently using the domain satisfy the given check * constraint expression. - * - * It is used to validate existing constraints and to add newly created check - * constraints to a domain. - * - * The lockmode is used for relations using the domain. It should be - * ShareLock when adding a new constraint to domain. It can be - * ShareUpdateExclusiveLock when validating an existing constraint. */ static void -validateDomainCheckConstraint(Oid domainoid, const char *ccbin, LOCKMODE lockmode) +validateDomainCheckConstraint(Oid domainoid, const char *ccbin) { Expr *expr = (Expr *) stringToNode(ccbin); List *rels; @@ -3261,7 +3249,9 @@ validateDomainCheckConstraint(Oid domainoid, const char *ccbin, LOCKMODE lockmod exprstate = ExecPrepareExpr(expr, estate); /* Fetch relation list with attributes based on this domain */ - rels = get_rels_with_domain(domainoid, lockmode); + /* ShareLock is sufficient to prevent concurrent data changes */ + + rels = get_rels_with_domain(domainoid, ShareLock); foreach(rt, rels) { diff --git a/src/test/isolation/expected/alter-domain-validate.out b/src/test/isolation/expected/alter-domain-validate.out new file mode 100644 index 00000000000..483e65c4a81 --- /dev/null +++ b/src/test/isolation/expected/alter-domain-validate.out @@ -0,0 +1,23 @@ +Parsed test spec with 3 sessions + +starting permutation: s1_lock s2_insert s3_add s3_validate s1_unlock s3_validated s3_check +step s1_lock: DO $$ BEGIN PERFORM pg_advisory_lock(8888); END $$; +step s2_insert: WITH wait AS MATERIALIZED (SELECT pg_advisory_lock(8888)) INSERT INTO alter_domain_validate_t SELECT (-1)::alter_domain_validate_d FROM wait; +step s3_add: ALTER DOMAIN alter_domain_validate_d ADD CONSTRAINT alter_domain_validate_d_pos CHECK (VALUE > 0) NOT VALID; +step s3_validate: ALTER DOMAIN alter_domain_validate_d VALIDATE CONSTRAINT alter_domain_validate_d_pos; +step s1_unlock: DO $$ BEGIN PERFORM pg_advisory_unlock(8888); END $$; +step s2_insert: <... completed> +step s3_validate: <... completed> +ERROR: column "a" of table "alter_domain_validate_t" contains values that violate the new constraint +step s3_validated: SELECT convalidated FROM pg_constraint WHERE conname = 'alter_domain_validate_d_pos'; +convalidated +------------ +f +(1 row) + +step s3_check: SELECT count(*) FROM alter_domain_validate_t; +count +----- + 1 +(1 row) + diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index 15c33fad4c5..b8ebe92553c 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -90,6 +90,7 @@ test: skip-locked-3 test: skip-locked-4 test: drop-index-concurrently-1 test: multiple-cic +test: alter-domain-validate test: alter-table-1 test: alter-table-2 test: alter-table-3 diff --git a/src/test/isolation/specs/alter-domain-validate.spec b/src/test/isolation/specs/alter-domain-validate.spec new file mode 100644 index 00000000000..daf9d83bfb6 --- /dev/null +++ b/src/test/isolation/specs/alter-domain-validate.spec @@ -0,0 +1,30 @@ +# Test ALTER DOMAIN VALIDATE CONSTRAINT waits for already-running DML. + +setup +{ + CREATE DOMAIN alter_domain_validate_d AS int; + CREATE TABLE alter_domain_validate_t (a alter_domain_validate_d); +} + +teardown +{ + DROP TABLE alter_domain_validate_t; + DROP DOMAIN alter_domain_validate_d; +} + +session s1 +step s1_lock { DO $$ BEGIN PERFORM pg_advisory_lock(8888); END $$; } +step s1_unlock { DO $$ BEGIN PERFORM pg_advisory_unlock(8888); END $$; } + +session s2 +# CoerceToDomain initializes the domain constraint list during executor +# startup, before this CTE waits on the advisory lock. +step s2_insert { WITH wait AS MATERIALIZED (SELECT pg_advisory_lock(8888)) INSERT INTO alter_domain_validate_t SELECT (-1)::alter_domain_validate_d FROM wait; } + +session s3 +step s3_add { ALTER DOMAIN alter_domain_validate_d ADD CONSTRAINT alter_domain_validate_d_pos CHECK (VALUE > 0) NOT VALID; } +step s3_validate { ALTER DOMAIN alter_domain_validate_d VALIDATE CONSTRAINT alter_domain_validate_d_pos; } +step s3_validated { SELECT convalidated FROM pg_constraint WHERE conname = 'alter_domain_validate_d_pos'; } +step s3_check { SELECT count(*) FROM alter_domain_validate_t; } + +permutation s1_lock s2_insert s3_add s3_validate s1_unlock s3_validated s3_check -- 2.50.1 (Apple Git-155)