On Wed, Sep 05, 2018 at 09:23:37AM -0700, Andres Freund wrote: > Well, we kinda do, during some of their own DDL. CF > AcquireDeletionLock(), RangeVarGetAndCheckCreationNamespace(), and other > LockDatabaseObject() callers. The > RangeVarGetAndCheckCreationNamespace() even locks the schema an object > is created in , which is pretty much what we're discussing here. > > I think the problem with the current logic is more that the > findDependentObjects() scan doesn't use a "dirty" scan, so it doesn't > ever get to seeing conflicting operations.
Well, it seems to me that we don't necessarily want to go down to that for sure on back-branches. What's actually striking me as a very bad thing is the inconsistent behavior when we need to get a namespace OID after calling QualifiedNameGetCreationNamespace using a list of defnames which does not lock the schema the DDL is working on. And this happens for basically all the object types Jimmy has mentioned. For example, when creating a composite type, the namespace lock is taken through RangeVarGetAndCheckCreationNamespace. If a user tries to create a root type, then we simply don't lock it, which leads to an inconsistency of behavior between composite types and root types. In my opinion the inconsistency of behavior for the same DDL is not logic. So I have been looking at that, and finished with the attached, which actually fixes the set of problems reported. Thoughts? -- Michael
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index 5d13e6a3d7..49bd61aff6 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -3011,6 +3011,14 @@ QualifiedNameGetCreationNamespace(List *names, char **objname_p)
errmsg("no schema has been selected to create in")));
}
+ Assert(OidIsValid(namespaceId));
+
+ /*
+ * Lock the creation namespace to protect against concurrent namespace
+ * drop.
+ */
+ LockDatabaseObject(NamespaceRelationId, namespaceId, 0, AccessShareLock);
+
return namespaceId;
}
diff --git a/src/test/isolation/expected/schema-drop.out b/src/test/isolation/expected/schema-drop.out
new file mode 100644
index 0000000000..5ea14abb0b
--- /dev/null
+++ b/src/test/isolation/expected/schema-drop.out
@@ -0,0 +1,43 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1_begin s1_create_type s2_drop_schema s1_commit
+step s1_begin: BEGIN;
+step s1_create_type: CREATE TYPE testschema.testtype;
+step s2_drop_schema: DROP SCHEMA testschema CASCADE; <waiting ...>
+step s1_commit: COMMIT;
+step s2_drop_schema: <... completed>
+
+starting permutation: s1_begin s1_create_agg s2_drop_schema s1_commit
+step s1_begin: BEGIN;
+step s1_create_agg: CREATE AGGREGATE testschema.a1(int4) (SFUNC=int4pl, STYPE=int4);
+step s2_drop_schema: DROP SCHEMA testschema CASCADE; <waiting ...>
+step s1_commit: COMMIT;
+step s2_drop_schema: <... completed>
+
+starting permutation: s1_begin s1_create_func s2_drop_schema s1_commit
+step s1_begin: BEGIN;
+step s1_create_func: CREATE FUNCTION testschema.f1() RETURNS bool LANGUAGE SQL AS 'SELECT true';
+step s2_drop_schema: DROP SCHEMA testschema CASCADE; <waiting ...>
+step s1_commit: COMMIT;
+step s2_drop_schema: <... completed>
+
+starting permutation: s1_begin s1_create_op s2_drop_schema s1_commit
+step s1_begin: BEGIN;
+step s1_create_op: CREATE OPERATOR testschema.@+@ (LEFTARG=int4, RIGHTARG=int4, PROCEDURE=int4pl);
+step s2_drop_schema: DROP SCHEMA testschema CASCADE; <waiting ...>
+step s1_commit: COMMIT;
+step s2_drop_schema: <... completed>
+
+starting permutation: s1_begin s1_create_opfamily s2_drop_schema s1_commit
+step s1_begin: BEGIN;
+step s1_create_opfamily: CREATE OPERATOR FAMILY testschema.opfam1 USING btree;
+step s2_drop_schema: DROP SCHEMA testschema CASCADE; <waiting ...>
+step s1_commit: COMMIT;
+step s2_drop_schema: <... completed>
+
+starting permutation: s1_begin s1_create_opclass s2_drop_schema s1_commit
+step s1_begin: BEGIN;
+step s1_create_opclass: CREATE OPERATOR CLASS testschema.opclass1 FOR TYPE uuid USING hash AS STORAGE uuid;
+step s2_drop_schema: DROP SCHEMA testschema CASCADE; <waiting ...>
+step s1_commit: COMMIT;
+step s2_drop_schema: <... completed>
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index c23b401225..12b7a96d7e 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -78,3 +78,4 @@ test: partition-key-update-3
test: partition-key-update-4
test: plpgsql-toast
test: truncate-conflict
+test: schema-drop
diff --git a/src/test/isolation/specs/schema-drop.spec b/src/test/isolation/specs/schema-drop.spec
new file mode 100644
index 0000000000..d40703d3a6
--- /dev/null
+++ b/src/test/isolation/specs/schema-drop.spec
@@ -0,0 +1,32 @@
+# Tests for schema drop with concurrently-created objects
+#
+# When an empty namespace is being initially populated with the below
+# objects, it is possible to DROP SCHEMA without a CASCADE before the
+# objects are committed. DROP SCHEMA should wait for the transaction
+# creating the given objects to commit before being able to perform the
+# schema deletion, and should drop all objects associated with it.
+
+setup
+{
+ CREATE SCHEMA testschema;
+}
+
+session "s1"
+step "s1_begin" { BEGIN; }
+step "s1_create_type" { CREATE TYPE testschema.testtype; }
+step "s1_create_agg" { CREATE AGGREGATE testschema.a1(int4) (SFUNC=int4pl, STYPE=int4); }
+step "s1_create_func" { CREATE FUNCTION testschema.f1() RETURNS bool LANGUAGE SQL AS 'SELECT true'; }
+step "s1_create_op" { CREATE OPERATOR testschema.@+@ (LEFTARG=int4, RIGHTARG=int4, PROCEDURE=int4pl);}
+step "s1_create_opfamily" { CREATE OPERATOR FAMILY testschema.opfam1 USING btree; }
+step "s1_create_opclass" { CREATE OPERATOR CLASS testschema.opclass1 FOR TYPE uuid USING hash AS STORAGE uuid; }
+step "s1_commit" { COMMIT; }
+
+session "s2"
+step "s2_drop_schema" { DROP SCHEMA testschema CASCADE; }
+
+permutation "s1_begin" "s1_create_type" "s2_drop_schema" "s1_commit"
+permutation "s1_begin" "s1_create_agg" "s2_drop_schema" "s1_commit"
+permutation "s1_begin" "s1_create_func" "s2_drop_schema" "s1_commit"
+permutation "s1_begin" "s1_create_op" "s2_drop_schema" "s1_commit"
+permutation "s1_begin" "s1_create_opfamily" "s2_drop_schema" "s1_commit"
+permutation "s1_begin" "s1_create_opclass" "s2_drop_schema" "s1_commit"
signature.asc
Description: PGP signature
