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 <lic@highgo.com>
Reviewed-by: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Álvaro Herrera <alvherre@kurilemu.de>
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 c4c3cdb5461a1a71ef5b5ee444b275cae6d02bb6..e9c3215ccecb22798e6b2b27f271116271e74095 100644 (file)
--- 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 (file)
index 0000000..483e65c
--- /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; <waiting ...>
+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; <waiting ...>
+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 15c33fad4c5ed9e362dac455755e45f77bd80c82..b8ebe92553c54d56bc6faf72bba96b4aa3eceb4b 100644 (file)
--- 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 (file)
index 0000000..daf9d83
--- /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