Thread
Commits
-
Fix ALTER DOMAIN VALIDATE CONSTRAINT locking
- 64797ad97d6e 19 (unreleased) landed
-
Fix ALTER DOMAIN VALIDATE CONSTRAINT locking
Chao Li <li.evan.chao@gmail.com> — 2026-06-08T03:53:29Z
Hi, While testing "[16a0039dc] Reduce lock level for ALTER DOMAIN ... VALIDATE CONSTRAINT", I found that it doesn't handle concurrent DML properly. Here is a repro: 1. In session 1, set up and use an advisory lock to block a concurrent insert from session 2: ``` evantest=# create domain d as int; CREATE DOMAIN evantest=# create table t (a d); CREATE TABLE evantest=# select pg_advisory_lock(8888); pg_advisory_lock ------------------ (1 row) ``` 2. In session 2, run a concurrent insert ``` evantest=# with wait as materialized (select pg_advisory_lock(8888)) insert into t select (-1)::d from wait; -- block here ``` 3. In session 3, alter the domain to add and validate a check constraint ``` evantest=# alter domain d add constraint d_c check (value > 0) not valid; ALTER DOMAIN evantest=# alter domain d validate constraint d_c; ALTER DOMAIN evantest=# select convalidated from pg_constraint where conname = 'd_c'; convalidated -------------- t (1 row) ``` 4. In session 1, unlock to let session’s insert continue ``` evantest=# select pg_advisory_unlock(8888); pg_advisory_unlock -------------------- t (1 row) ``` 5. Check the result in any session ``` evantest=# select convalidated from pg_constraint where conname = 'd_c'; convalidated -------------- t (1 row) evantest=# select * from t; a ---- -1 (1 row) ``` As we can see, the constraint is validated, but “-1", which violates the constraint, has been inserted. The commit message for 16a0039dc says table constraints have the same behavior: ``` Now we should still be able to perform DML operations on table t while the domain constraint is being validated. The equivalent works already on table constraints. ``` I don't think that is true for already-running DML. ALTER TABLE ADD CONSTRAINT acquires AccessExclusiveLock, so it waits for a concurrent INSERT on the target table. Here is a similar test with ALTER TABLE: 1. In session 1: ``` evantest=# create table t1 (a int); CREATE TABLE evantest=# select pg_advisory_lock(6666); pg_advisory_lock ------------------ (1 row) ``` 2. In session 2 ``` evantest=# with wait as materialized (select pg_advisory_lock(6666)) insert into t1 select -1 from wait; -- block here ``` 3. In session 3 ``` evantest=# alter table t1 add constraint t_c check (a>0) not valid; -- block here ``` 4. In session 4, unlock ``` evantest=# select pg_advisory_unlock(6666); pg_advisory_unlock -------------------- t (1 row) ``` 5. ADD constraint succeeded in session 3: ``` ALTER TABLE ``` 6. INSERT also succeeded in session 2: ``` INSERT 0 1 ``` 7. Then, VALIDATE CONSTRAINT failed, and the constraint’s convalidated remains false: ``` evantest=# alter table t1 validate constraint t_c; ERROR: check constraint "t_c" of relation "t1" is violated by some row evantest=# select convalidated from pg_constraint where conname = 't_c'; convalidated -------------- f (1 row) ``` So the key difference is that ALTER TABLE ADD CONSTRAINT waits for already-running DML on the target table before adding the NOT VALID constraint, but ALTER DOMAIN ADD CONSTRAINT does not wait for already-running DML on dependent tables. The only fix I can see is to use ShareLock again while validating the domain constraint. There is a follow up commit, a99c6b56ffa, and that change looks sound, so maybe we should update the current code instead of reverting 16a0039dc. With ShareLock restored, rerunning the repro makes session 2's INSERT succeed, but session 3's ALTER DOMAIN VALIDATE CONSTRAINT fails and convalidated remains false. Now the behavior matches the ALTER TABLE case. See the attached patch for details. I also added an isolation test that follows the repro above. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ -
Re: Fix ALTER DOMAIN VALIDATE CONSTRAINT locking
Fujii Masao <masao.fujii@gmail.com> — 2026-06-12T11:47:28Z
On Mon, Jun 8, 2026 at 12:54 PM Chao Li <li.evan.chao@gmail.com> wrote: > See the attached patch for details. I also added an isolation test that follows the repro above. Thanks for the patch! It looks good to me. I just have a few minor review comments. * 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. This comment in validateDomainCheckConstraint() still references ShareUpdateExclusiveLock, so it seems to need updating. Regarding the test, I think it would be safer to also check whether pg_constraint.convalidated is set correctly. For example: ----------------------- 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_check +permutation s1_lock s2_insert s3_add s3_validate s1_unlock s3_validated s3_chec ----------------------- After the fix is committed, we should probably also ask Bruce to update the following v19 release note item?: Reduce lock level of ALTER DOMAIN ... VALIDATE CONSTRAINT to match ALTER TABLE ... VALIDATE CONSTRAINT (Jian He) § Regards, -- Fujii Masao -
Re: Fix ALTER DOMAIN VALIDATE CONSTRAINT locking
Chao Li <li.evan.chao@gmail.com> — 2026-06-12T12:45:00Z
> On Jun 12, 2026, at 19:47, Fujii Masao <masao.fujii@gmail.com> wrote: > > On Mon, Jun 8, 2026 at 12:54 PM Chao Li <li.evan.chao@gmail.com> wrote: >> See the attached patch for details. I also added an isolation test that follows the repro above. > > Thanks for the patch! It looks good to me. > > I just have a few minor review comments. > > > * 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. > > This comment in validateDomainCheckConstraint() still references > ShareUpdateExclusiveLock, so it seems to need updating. > > > Regarding the test, I think it would be safer to also check whether > pg_constraint.convalidated is set correctly. For example: > > ----------------------- > 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_check > +permutation s1_lock s2_insert s3_add s3_validate s1_unlock s3_validated s3_chec > ----------------------- > > > After the fix is committed, we should probably also ask Bruce to update > the following v19 release note item?: > > Reduce lock level of ALTER DOMAIN ... VALIDATE CONSTRAINT to match > ALTER TABLE ... VALIDATE CONSTRAINT (Jian He) § > > Regards, > > -- > Fujii Masao Hi Fujii-san, Thanks for picking this up. PFA v2 that addressed your comments. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ -
Re: Fix ALTER DOMAIN VALIDATE CONSTRAINT locking
Álvaro Herrera <alvherre@kurilemu.de> — 2026-06-12T13:02:37Z
Hello, On 2026-Jun-12, Fujii Masao wrote: > I just have a few minor review comments. > > * 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. > > This comment in validateDomainCheckConstraint() still references > ShareUpdateExclusiveLock, so it seems to need updating. I may be missing something, but doesn't changing the ShareUpdateExclusive lock to ShareLock means essentially reverting 16a0039dc0d1? I mean, the code was previously using ShareLock for both uses of validateDomainCheckConstraint(); and what that commit did was change AlterDomainValidateConstraint() to use ShareUpdateExclusiveLock, while AlterDomainAddConstraint() retained the stronger ShareLock level. If we now change AlterDomainValidateConstraint() back to ShareLock, then the aforementioned commit has no effect whatsoever. > After the fix is committed, we should probably also ask Bruce to update > the following v19 release note item?: > > Reduce lock level of ALTER DOMAIN ... VALIDATE CONSTRAINT to match > ALTER TABLE ... VALIDATE CONSTRAINT (Jian He) § If we make this change, then the release note item should be removed entirely, ISTM. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Hay quien adquiere la mala costumbre de ser infeliz" (M. A. Evans)
-
Re: Fix ALTER DOMAIN VALIDATE CONSTRAINT locking
Chao Li <li.evan.chao@gmail.com> — 2026-06-12T14:01:44Z
> On Jun 12, 2026, at 21:02, Álvaro Herrera <alvherre@kurilemu.de> wrote: > > Hello, > > On 2026-Jun-12, Fujii Masao wrote: > >> I just have a few minor review comments. >> >> * 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. >> >> This comment in validateDomainCheckConstraint() still references >> ShareUpdateExclusiveLock, so it seems to need updating. > > I may be missing something, but doesn't changing the > ShareUpdateExclusive lock to ShareLock means essentially reverting > 16a0039dc0d1? I mean, the code was previously using ShareLock for both > uses of validateDomainCheckConstraint(); and what that commit did was > change AlterDomainValidateConstraint() to use ShareUpdateExclusiveLock, > while AlterDomainAddConstraint() retained the stronger ShareLock level. > If we now change AlterDomainValidateConstraint() back to ShareLock, then > the aforementioned commit has no effect whatsoever. From a runtime behavior perspective, yes, this patch reverts the behavior change made by 16a0039dc0d1. However: * 16a0039dc0d1 also refactored validateDomainCheckConstraint() to allow passing in the lock mode, and I think that refactoring is still useful and maybe worth keeping. * A follow-up commit, a99c6b56f, made validating an already-validated constraint a no-op. A direct revert of 16a0039dc0d1 would conflict with later changes around this code. * This patch also adds a test to prevent future changes from making the same mistake. > >> After the fix is committed, we should probably also ask Bruce to update >> the following v19 release note item?: >> >> Reduce lock level of ALTER DOMAIN ... VALIDATE CONSTRAINT to match >> ALTER TABLE ... VALIDATE CONSTRAINT (Jian He) § > > If we make this change, then the release note item should be removed > entirely, ISTM. > True. Once this patch is pushed, this item should be removed from the release note. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
-
Re: Fix ALTER DOMAIN VALIDATE CONSTRAINT locking
Fujii Masao <masao.fujii@gmail.com> — 2026-06-14T15:52:44Z
On Fri, Jun 12, 2026 at 11:02 PM Chao Li <li.evan.chao@gmail.com> wrote: > From a runtime behavior perspective, yes, this patch reverts the behavior change made by 16a0039dc0d1. Yes. > However: > > * 16a0039dc0d1 also refactored validateDomainCheckConstraint() to allow passing in the lock mode, and I think that refactoring is still useful and maybe worth keeping. I'd prefer to remove that refactoring code, since after the fix there is no longer any user of the lockmode argument in validateDomainCheckConstraint(). > * A follow-up commit, a99c6b56f, made validating an already-validated constraint a no-op. A direct revert of 16a0039dc0d1 would conflict with later changes around this code. Yes, but fixing that conflict does not seem very difficult, no? > * This patch also adds a test to prevent future changes from making the same mistake. +1 for adding the test! > > If we make this change, then the release note item should be removed > > entirely, ISTM. > > > > True. Once this patch is pushed, this item should be removed from the release note. Agreed. Regards, -- Fujii Masao
-
Re: Fix ALTER DOMAIN VALIDATE CONSTRAINT locking
Chao Li <li.evan.chao@gmail.com> — 2026-06-15T01:11:03Z
> On Jun 14, 2026, at 23:52, Fujii Masao <masao.fujii@gmail.com> wrote: > > On Fri, Jun 12, 2026 at 11:02 PM Chao Li <li.evan.chao@gmail.com> wrote: >> From a runtime behavior perspective, yes, this patch reverts the behavior change made by 16a0039dc0d1. > > Yes. > > >> However: >> >> * 16a0039dc0d1 also refactored validateDomainCheckConstraint() to allow passing in the lock mode, and I think that refactoring is still useful and maybe worth keeping. > > I'd prefer to remove that refactoring code, since after the fix there > is no longer any user of the lockmode argument in > validateDomainCheckConstraint(). > > >> * A follow-up commit, a99c6b56f, made validating an already-validated constraint a no-op. A direct revert of 16a0039dc0d1 would conflict with later changes around this code. > > Yes, but fixing that conflict does not seem very difficult, no? > > >> * This patch also adds a test to prevent future changes from making the same mistake. > > +1 for adding the test! > > >>> If we make this change, then the release note item should be removed >>> entirely, ISTM. >>> >> >> True. Once this patch is pushed, this item should be removed from the release note. > > Agreed. > > Regards, > > -- > Fujii Masao Okay, I manually reverted 16a0039dc0d1 with retaining a99c6b56f. I also updated the commit message accordingly. See the attached v3. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
-
Re: Fix ALTER DOMAIN VALIDATE CONSTRAINT locking
Fujii Masao <masao.fujii@gmail.com> — 2026-06-17T00:12:33Z
On Mon, Jun 15, 2026 at 10:11 AM Chao Li <li.evan.chao@gmail.com> wrote: > Okay, I manually reverted 16a0039dc0d1 with retaining a99c6b56f. I also updated the commit message accordingly. See the attached v3. Thanks for updating the patch! Barring any objections, I will commit it. Regards, -- Fujii Masao
-
Re: Fix ALTER DOMAIN VALIDATE CONSTRAINT locking
Fujii Masao <masao.fujii@gmail.com> — 2026-06-18T01:23:18Z
On Wed, Jun 17, 2026 at 9:12 AM Fujii Masao <masao.fujii@gmail.com> wrote: > > On Mon, Jun 15, 2026 at 10:11 AM Chao Li <li.evan.chao@gmail.com> wrote: > > Okay, I manually reverted 16a0039dc0d1 with retaining a99c6b56f. I also updated the commit message accordingly. See the attached v3. > > Thanks for updating the patch! Barring any objections, I will commit it. I've pushed the patch. Thanks! Regards, -- Fujii Masao