From: Fujii Masao Date: Thu, 18 Jun 2026 01:19:15 +0000 (+0900) Subject: Fix ALTER DOMAIN VALIDATE CONSTRAINT locking X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=64797ad97d6e0a476f809979df99e0013c1933b1;p=thirdparty%2Fpostgresql.git Fix ALTER DOMAIN VALIDATE CONSTRAINT locking Commit 16a0039dc0d1 reduced the lock level for ALTER DOMAIN ... VALIDATE CONSTRAINT from ShareLock to ShareUpdateExclusiveLock. However, that change was unsafe. If DML on tables using the domain had already started and initialized domain constraint checks before a NOT VALID constraint was added, it could still insert or update rows that violated the new constraint. This commit reverts commit 16a0039dc0d1 so that ALTER DOMAIN ... VALIDATE CONSTRAINT once again acquires ShareLock on relations using the domain. Also add an isolation test covering this case. Author: Chao Li Reviewed-by: Fujii Masao Reviewed-by: Álvaro Herrera Discussion: https://postgr.es/m/463C0E1A-4A40-4BCA-839C-9236B80D65EE@gmail.com --- 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