hi similar to thread "Prevent internal error at concurrent CREATE OR REPLACE FUNCTION" [1].
We should prevent concurrent modifications to a domain's definition. Currently, it is possible for one session to drop a domain while another session simultaneously adds a constraint to it. It may result in errors such as "tuple concurrently updated." also dropping a domain should not be allowed if another session is modifying it, IMHO. The attached patch is very similar to the "CREATE OR REPLACE FUNCTION" thread [1], by acquiring a AccessExclusiveLock on the changed domain oid. Other sessions must wait for the current transactions to finish modifying the domain definition before making changes on it. [1] https://postgr.es/m/20250331200057.00a62760966a821d484ea...@sraoss.co.jp
From a1f928637d9bf796e7a62e260d844c127ddfa720 Mon Sep 17 00:00:00 2001 From: jian he <jian.universal...@gmail.com> Date: Fri, 23 May 2025 16:15:11 +0800 Subject: [PATCH v1 1/1] fix concurrent issue in ALTER DOMAIN we should ensure that two backends can not ALTER DOMAIN at the same time discussion: https://postgr.es/m/ --- src/backend/commands/typecmds.c | 35 +++++++++++++++ src/test/isolation/expected/domains.out | 58 +++++++++++++++++++++++++ src/test/isolation/isolation_schedule | 1 + src/test/isolation/specs/domains.spec | 32 ++++++++++++++ 4 files changed, 126 insertions(+) create mode 100644 src/test/isolation/expected/domains.out create mode 100644 src/test/isolation/specs/domains.spec diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c index 45ae7472ab5..e2b52367d87 100644 --- a/src/backend/commands/typecmds.c +++ b/src/backend/commands/typecmds.c @@ -2623,6 +2623,13 @@ AlterDomainDefault(List *names, Node *defaultRaw) typename = makeTypeNameFromNameList(names); domainoid = typenameTypeId(NULL, typename); + /* + * Acquire a lock on the domain type, which we won't release until commit. + * This ensures that two backends aren't concurrently modifying the same + * domain type. + */ + LockDatabaseObject(TypeRelationId, domainoid, 0, AccessExclusiveLock); + /* Look up the domain in the type table */ rel = table_open(TypeRelationId, RowExclusiveLock); @@ -2745,6 +2752,13 @@ AlterDomainNotNull(List *names, bool notNull) typename = makeTypeNameFromNameList(names); domainoid = typenameTypeId(NULL, typename); + /* + * Acquire a lock on the domain type, which we won't release until commit. + * This ensures that two backends aren't concurrently modifying the same + * domain type. + */ + LockDatabaseObject(TypeRelationId, domainoid, 0, AccessExclusiveLock); + /* Look up the domain in the type table */ typrel = table_open(TypeRelationId, RowExclusiveLock); @@ -2836,6 +2850,13 @@ AlterDomainDropConstraint(List *names, const char *constrName, typename = makeTypeNameFromNameList(names); domainoid = typenameTypeId(NULL, typename); + /* + * Acquire a lock on the domain type, which we won't release until commit. + * This ensures that two backends aren't concurrently modifying the same + * domain type. + */ + LockDatabaseObject(TypeRelationId, domainoid, 0, AccessExclusiveLock); + /* Look up the domain in the type table */ rel = table_open(TypeRelationId, RowExclusiveLock); @@ -2940,6 +2961,13 @@ AlterDomainAddConstraint(List *names, Node *newConstraint, typename = makeTypeNameFromNameList(names); domainoid = typenameTypeId(NULL, typename); + /* + * Acquire a lock on the domain type, which we won't release until commit. + * This ensures that two backends aren't concurrently modifying the same + * domain type. + */ + LockDatabaseObject(TypeRelationId, domainoid, 0, AccessExclusiveLock); + /* Look up the domain in the type table */ typrel = table_open(TypeRelationId, RowExclusiveLock); @@ -3042,6 +3070,13 @@ AlterDomainValidateConstraint(List *names, const char *constrName) typename = makeTypeNameFromNameList(names); domainoid = typenameTypeId(NULL, typename); + /* + * Acquire a lock on the domain type, which we won't release until commit. + * This ensures that two backends aren't concurrently modifying the same + * domain type. + */ + LockDatabaseObject(TypeRelationId, domainoid, 0, AccessExclusiveLock); + /* Look up the domain in the type table */ typrel = table_open(TypeRelationId, AccessShareLock); diff --git a/src/test/isolation/expected/domains.out b/src/test/isolation/expected/domains.out new file mode 100644 index 00000000000..779e0918c54 --- /dev/null +++ b/src/test/isolation/expected/domains.out @@ -0,0 +1,58 @@ +Parsed test spec with 2 sessions + +starting permutation: b1 b2 s_add s2_drop c1 c2 +step b1: BEGIN; +step b2: BEGIN; +step s_add: ALTER DOMAIN dd ADD CONSTRAINT dd_check1 CHECK(VALUE > 0); +step s2_drop: DROP DOMAIN dd; <waiting ...> +step c1: COMMIT; +step s2_drop: <... completed> +step c2: COMMIT; + +starting permutation: b1 b2 s_add s2_set c1 c2 +step b1: BEGIN; +step b2: BEGIN; +step s_add: ALTER DOMAIN dd ADD CONSTRAINT dd_check1 CHECK(VALUE > 0); +step s2_set: ALTER DOMAIN dd SET DEFAULT 3; <waiting ...> +step c1: COMMIT; +step s2_set: <... completed> +step c2: COMMIT; + +starting permutation: b1 b2 s1_set s1_drop s2_set c1 c2 +step b1: BEGIN; +step b2: BEGIN; +step s1_set: ALTER DOMAIN dd SET NOT NULL; +step s1_drop: ALTER DOMAIN dd DROP DEFAULT; +step s2_set: ALTER DOMAIN dd SET DEFAULT 3; <waiting ...> +step c1: COMMIT; +step s2_set: <... completed> +step c2: COMMIT; + +starting permutation: b1 b2 s1_drop s2_set c1 c2 +step b1: BEGIN; +step b2: BEGIN; +step s1_drop: ALTER DOMAIN dd DROP DEFAULT; +step s2_set: ALTER DOMAIN dd SET DEFAULT 3; <waiting ...> +step c1: COMMIT; +step s2_set: <... completed> +step c2: COMMIT; + +starting permutation: b1 b2 s1_set s1_drop2 v2 c1 c2 +step b1: BEGIN; +step b2: BEGIN; +step s1_set: ALTER DOMAIN dd SET NOT NULL; +step s1_drop2: ALTER DOMAIN dd DROP CONSTRAINT cc; +step v2: ALTER DOMAIN dd VALIDATE CONSTRAINT cc; <waiting ...> +step c1: COMMIT; +step v2: <... completed> +ERROR: constraint "cc" of domain "dd" does not exist +step c2: COMMIT; + +starting permutation: b1 b2 v2 s1_drop2 c2 c1 +step b1: BEGIN; +step b2: BEGIN; +step v2: ALTER DOMAIN dd VALIDATE CONSTRAINT cc; +step s1_drop2: ALTER DOMAIN dd DROP CONSTRAINT cc; <waiting ...> +step c2: COMMIT; +step s1_drop2: <... completed> +step c1: COMMIT; diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index e3c669a29c7..b57aa03155a 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -116,3 +116,4 @@ test: serializable-parallel-2 test: serializable-parallel-3 test: matview-write-skew test: lock-nowait +test: domains diff --git a/src/test/isolation/specs/domains.spec b/src/test/isolation/specs/domains.spec new file mode 100644 index 00000000000..67e6e38d031 --- /dev/null +++ b/src/test/isolation/specs/domains.spec @@ -0,0 +1,32 @@ +setup +{ + CREATE DOMAIN dd AS INT; + ALTER DOMAIN dd ADD CONSTRAINT cc CHECK(VALUE > 1) NOT VALID; +} + +teardown +{ + DROP DOMAIN IF EXISTS dd CASCADE; +} + +session s1 +step b1 { BEGIN; } +step s_add { ALTER DOMAIN dd ADD CONSTRAINT dd_check1 CHECK(VALUE > 0); } +step s1_drop { ALTER DOMAIN dd DROP DEFAULT; } +step s1_set { ALTER DOMAIN dd SET NOT NULL; } +step s1_drop2 { ALTER DOMAIN dd DROP CONSTRAINT cc;} +step c1 { COMMIT; } + +session s2 +step b2 { BEGIN; } +step s2_set { ALTER DOMAIN dd SET DEFAULT 3; } +step s2_drop { DROP DOMAIN dd; } +step v2 { ALTER DOMAIN dd VALIDATE CONSTRAINT cc; } +step c2 { COMMIT; } + +permutation b1 b2 s_add s2_drop c1 c2 +permutation b1 b2 s_add s2_set c1 c2 +permutation b1 b2 s1_set s1_drop s2_set c1 c2 +permutation b1 b2 s1_drop s2_set c1 c2 +permutation b1 b2 s1_set s1_drop2 v2 c1 c2 +permutation b1 b2 v2 s1_drop2 c2 c1 -- 2.34.1
