diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c index 4f1a44ee901..91dbf8c9066 100644 --- a/src/backend/catalog/pg_depend.c +++ b/src/backend/catalog/pg_depend.c @@ -27,13 +27,17 @@ #include "catalog/partition.h" #include "commands/extension.h" #include "miscadmin.h" +#include "storage/lmgr.h" +#include "storage/lock.h" #include "utils/fmgroids.h" #include "utils/lsyscache.h" -#include "utils/syscache.h" #include "utils/rel.h" +#include "utils/snapmgr.h" +#include "utils/syscache.h" static bool isObjectPinned(const ObjectAddress *object); +static void dependencyLockAndCheckObject(Oid classId, Oid objectId); /* @@ -109,6 +113,13 @@ recordMultipleDependencies(const ObjectAddress *depender, if (isObjectPinned(referenced)) continue; + /* + * Make sure the new referenced object doesn't go away while we record + * the dependency. DROP routines should lock the object exclusively + * before they check dependencies. + */ + dependencyLockAndCheckObject(referenced->classId, referenced->objectId); + if (slot_init_count < max_slots) { slot[slot_stored_count] = MakeSingleTupleTableSlot(RelationGetDescr(dependDesc), @@ -507,6 +518,13 @@ changeDependencyFor(Oid classId, Oid objectId, return 1; } + /* + * Make sure the new referenced object doesn't go away while we record the + * dependency. + */ + if (!newIsPinned) + dependencyLockAndCheckObject(refClassId, newRefObjectId); + depRel = table_open(DependRelationId, RowExclusiveLock); /* There should be existing dependency record(s), so search. */ @@ -714,6 +732,119 @@ isObjectPinned(const ObjectAddress *object) } +/* + * dependencyLockAndCheckObject + * + * Lock the object that we are about to record a dependency on. After it's + * locked, verify that it hasn't been dropped while we weren't looking. If it + * has been dropped, throw an an error. + * + * If the caller already holds a lock that conflicts with DROP + * (AccessShareLock or stronger), this does nothing. Callers should acquire + * locks already when they look up the dependent objects, but many callers + * currently do not. This is a backstop to make sure that we don't record a + * bogus reference permanently in the catalogs in that case. In the future, + * after we have tightened up all the callers to acquire locks earlier, this + * could just verify that the object is already locked and throw an error if + * not. + */ +static void +dependencyLockAndCheckObject(Oid classId, Oid objectId) +{ + /* + * Pinned objects cannot be dropped concurrently, and callers checked this + * already. + */ + Assert(!IsPinnedObject(classId, objectId)); + + if (classId != RelationRelationId) + { + LOCKTAG tag; + int cache; + Relation rel; + SysScanDesc scan; + ScanKeyData skey; + HeapTuple tuple; + + SET_LOCKTAG_OBJECT(tag, + MyDatabaseId, + classId, + objectId, + 0); + + if (LockHeldByMe(&tag, AccessShareLock, true)) + return; + + /* Assume we should lock the whole object not a sub-object */ + LockDatabaseObject(classId, objectId, 0, AccessShareLock); + + /* + * Check that the object still exists. If the catalog has a suitable + * syscache, check that first. + */ + cache = get_object_catcache_oid(classId); + if (cache != -1) + { + if (SearchSysCacheExists1(cache, ObjectIdGetDatum(objectId))) + return; + } + + /* + * If it's not found in the syscache, or there's no suitable syscache + * we can use, scan the catalog table using SnapshotSelf. This + * handles the case that it's an object we just created (for example, + * if it's a composite type created as part of creating a table). + */ + rel = table_open(classId, AccessShareLock); + + ScanKeyInit(&skey, + get_object_attnum_oid(classId), + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(objectId)); + + scan = systable_beginscan(rel, get_object_oid_index(classId), + true, SnapshotSelf, 1, &skey); + + tuple = systable_getnext(scan); + if (!HeapTupleIsValid(tuple)) + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("dependent %s was concurrently dropped", + get_object_class_descr(classId)))); + + systable_endscan(scan); + table_close(rel, AccessShareLock); + } + else + { + /* + * Same logic for pg_class entries, but locking relations is handled + * by different functions. + * + * Callers are more careful with locking relations than other objects, + * so we should already have a lock on the relation, or on another + * object that indirectly prevents the relation from being dropped. + * For example, we might have a strong lock on a table while adding + * dependency to its index. However, we cannot detect the indirectly + * protected case here easily. To err on the safe side, acquire a + * lock directly on the relation if we're not holding one already. + */ + + /* all shared relations are pinned */ + Assert(!IsSharedRelation(objectId)); + + if (CheckRelationOidLockedByMe(objectId, AccessShareLock, true)) + return; + LockRelationOid(objectId, AccessShareLock); + + if (SearchSysCacheExists1(RELOID, ObjectIdGetDatum(objectId))) + return; + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_OBJECT), + errmsg("dependent relation was concurrently dropped"))); + } +} + /* * Various special-purpose lookups and manipulations of pg_depend. */ diff --git a/src/test/isolation/expected/ddl-dependency-locking.out b/src/test/isolation/expected/ddl-dependency-locking.out new file mode 100644 index 00000000000..3bf9e435725 --- /dev/null +++ b/src/test/isolation/expected/ddl-dependency-locking.out @@ -0,0 +1,137 @@ +Parsed test spec with 2 sessions + +starting permutation: s1_begin s1_create_function_in_schema s2_drop_schema s1_commit +step s1_begin: BEGIN; +step s1_create_function_in_schema: CREATE FUNCTION testschema.foo() RETURNS int AS 'select 1' LANGUAGE sql; +step s2_drop_schema: DROP SCHEMA testschema; +step s1_commit: COMMIT; +step s2_drop_schema: <... completed> +ERROR: cannot drop schema testschema because other objects depend on it + +starting permutation: s2_begin s2_drop_schema s1_create_function_in_schema s2_commit +step s2_begin: BEGIN; +step s2_drop_schema: DROP SCHEMA testschema; +step s1_create_function_in_schema: CREATE FUNCTION testschema.foo() RETURNS int AS 'select 1' LANGUAGE sql; +step s2_commit: COMMIT; +step s1_create_function_in_schema: <... completed> +ERROR: dependent schema was concurrently dropped + +starting permutation: s1_begin s1_alter_function_schema s2_drop_alterschema s1_commit +step s1_begin: BEGIN; +step s1_alter_function_schema: ALTER FUNCTION public.falter() SET SCHEMA alterschema; +step s2_drop_alterschema: DROP SCHEMA alterschema; +step s1_commit: COMMIT; +step s2_drop_alterschema: <... completed> +ERROR: cannot drop schema alterschema because other objects depend on it + +starting permutation: s2_begin s2_drop_alterschema s1_alter_function_schema s2_commit +step s2_begin: BEGIN; +step s2_drop_alterschema: DROP SCHEMA alterschema; +step s1_alter_function_schema: ALTER FUNCTION public.falter() SET SCHEMA alterschema; +step s2_commit: COMMIT; +step s1_alter_function_schema: <... completed> +ERROR: dependent schema was concurrently dropped + +starting permutation: s1_begin s1_create_function_with_argtype s2_drop_foo_type s1_commit +step s1_begin: BEGIN; +step s1_create_function_with_argtype: CREATE FUNCTION fooargtype(num foo) RETURNS int AS 'select 1' LANGUAGE sql; +step s2_drop_foo_type: DROP TYPE public.foo; +step s1_commit: COMMIT; +step s2_drop_foo_type: <... completed> +ERROR: cannot drop type foo because other objects depend on it + +starting permutation: s2_begin s2_drop_foo_type s1_create_function_with_argtype s2_commit +step s2_begin: BEGIN; +step s2_drop_foo_type: DROP TYPE public.foo; +step s1_create_function_with_argtype: CREATE FUNCTION fooargtype(num foo) RETURNS int AS 'select 1' LANGUAGE sql; +step s2_commit: COMMIT; +step s1_create_function_with_argtype: <... completed> +ERROR: dependent type was concurrently dropped + +starting permutation: s1_begin s1_create_function_with_rettype s2_drop_foo_rettype s1_commit +step s1_begin: BEGIN; +step s1_create_function_with_rettype: CREATE FUNCTION footrettype() RETURNS id LANGUAGE sql RETURN 1; +step s2_drop_foo_rettype: DROP DOMAIN id; +step s1_commit: COMMIT; +step s2_drop_foo_rettype: <... completed> +ERROR: cannot drop type id because other objects depend on it + +starting permutation: s2_begin s2_drop_foo_rettype s1_create_function_with_rettype s2_commit +step s2_begin: BEGIN; +step s2_drop_foo_rettype: DROP DOMAIN id; +step s1_create_function_with_rettype: CREATE FUNCTION footrettype() RETURNS id LANGUAGE sql RETURN 1; +step s2_commit: COMMIT; +step s1_create_function_with_rettype: <... completed> +ERROR: dependent type was concurrently dropped + +starting permutation: s1_begin s1_create_function_with_function s2_drop_function_f s1_commit +step s1_begin: BEGIN; +step s1_create_function_with_function: CREATE FUNCTION foofunc() RETURNS int LANGUAGE SQL RETURN f() + 1; +step s2_drop_function_f: DROP FUNCTION f(); +step s1_commit: COMMIT; +step s2_drop_function_f: <... completed> +ERROR: cannot drop function f() because other objects depend on it + +starting permutation: s2_begin s2_drop_function_f s1_create_function_with_function s2_commit +step s2_begin: BEGIN; +step s2_drop_function_f: DROP FUNCTION f(); +step s1_create_function_with_function: CREATE FUNCTION foofunc() RETURNS int LANGUAGE SQL RETURN f() + 1; +step s2_commit: COMMIT; +step s1_create_function_with_function: <... completed> +ERROR: dependent function was concurrently dropped + +starting permutation: s1_begin s1_create_domain_with_domain s2_drop_domain_id s1_commit +step s1_begin: BEGIN; +step s1_create_domain_with_domain: CREATE DOMAIN idid as id; +step s2_drop_domain_id: DROP DOMAIN id; +step s1_commit: COMMIT; +step s2_drop_domain_id: <... completed> +ERROR: cannot drop type id because other objects depend on it + +starting permutation: s2_begin s2_drop_domain_id s1_create_domain_with_domain s2_commit +step s2_begin: BEGIN; +step s2_drop_domain_id: DROP DOMAIN id; +step s1_create_domain_with_domain: CREATE DOMAIN idid as id; +step s2_commit: COMMIT; +step s1_create_domain_with_domain: <... completed> +ERROR: dependent type was concurrently dropped + +starting permutation: s1_begin s1_create_table_with_type s2_drop_footab_type s1_commit +step s1_begin: BEGIN; +step s1_create_table_with_type: CREATE TABLE tabtype(a footab); +step s2_drop_footab_type: DROP TYPE public.footab; +step s1_commit: COMMIT; +step s2_drop_footab_type: <... completed> +ERROR: cannot drop type footab because other objects depend on it + +starting permutation: s2_begin s2_drop_footab_type s1_create_table_with_type s2_commit +step s2_begin: BEGIN; +step s2_drop_footab_type: DROP TYPE public.footab; +step s1_create_table_with_type: CREATE TABLE tabtype(a footab); +step s2_commit: COMMIT; +step s1_create_table_with_type: <... completed> +ERROR: dependent type was concurrently dropped + +starting permutation: s1_begin s1_create_server_with_fdw_wrapper s2_drop_fdw_wrapper s1_commit +step s1_begin: BEGIN; +step s1_create_server_with_fdw_wrapper: CREATE SERVER srv_fdw_wrapper FOREIGN DATA WRAPPER fdw_wrapper; +step s2_drop_fdw_wrapper: DROP FOREIGN DATA WRAPPER fdw_wrapper RESTRICT; +step s1_commit: COMMIT; +step s2_drop_fdw_wrapper: <... completed> +ERROR: cannot drop foreign-data wrapper fdw_wrapper because other objects depend on it + +starting permutation: s2_begin s2_drop_fdw_wrapper s1_create_server_with_fdw_wrapper s2_commit +step s2_begin: BEGIN; +step s2_drop_fdw_wrapper: DROP FOREIGN DATA WRAPPER fdw_wrapper RESTRICT; +step s1_create_server_with_fdw_wrapper: CREATE SERVER srv_fdw_wrapper FOREIGN DATA WRAPPER fdw_wrapper; +step s2_commit: COMMIT; +step s1_create_server_with_fdw_wrapper: <... completed> +ERROR: dependent foreign-data wrapper was concurrently dropped + +starting permutation: s1_begin s1_alter_function_owner s2_drop_role s1_commit +step s1_begin: BEGIN; +step s1_alter_function_owner: ALTER FUNCTION public.falter() OWNER TO regress_dependency; +step s2_drop_role: DROP ROLE regress_dependency; +step s1_commit: COMMIT; +step s2_drop_role: <... completed> +ERROR: role "regress_dependency" cannot be dropped because some objects depend on it diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index f476aa9b7ed..7a4ab2ce805 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: ddl-dependency-locking diff --git a/src/test/isolation/specs/ddl-dependency-locking.spec b/src/test/isolation/specs/ddl-dependency-locking.spec new file mode 100644 index 00000000000..de5bd88d35e --- /dev/null +++ b/src/test/isolation/specs/ddl-dependency-locking.spec @@ -0,0 +1,104 @@ +# Test that concurrent DROP and CREATE commands do not leave behind +# references to non-existent objects. + +setup +{ + CREATE SCHEMA testschema; + CREATE SCHEMA alterschema; + CREATE TYPE public.foo as enum ('one', 'two'); + CREATE TYPE public.footab as enum ('three', 'four'); + CREATE DOMAIN id AS int; + CREATE FUNCTION f() RETURNS int LANGUAGE SQL RETURN 1; + CREATE FUNCTION public.falter() RETURNS int LANGUAGE SQL RETURN 1; + CREATE FOREIGN DATA WRAPPER fdw_wrapper; + CREATE ROLE regress_dependency; +} + +teardown +{ + DROP FUNCTION IF EXISTS testschema.foo(); + DROP FUNCTION IF EXISTS fooargtype(num foo); + DROP FUNCTION IF EXISTS footrettype(); + DROP FUNCTION IF EXISTS foofunc(); + DROP FUNCTION IF EXISTS public.falter(); + DROP FUNCTION IF EXISTS alterschema.falter(); + DROP DOMAIN IF EXISTS idid; + DROP SERVER IF EXISTS srv_fdw_wrapper; + DROP TABLE IF EXISTS tabtype; + DROP SCHEMA IF EXISTS testschema; + DROP SCHEMA IF EXISTS alterschema; + DROP TYPE IF EXISTS public.foo; + DROP TYPE IF EXISTS public.footab; + DROP DOMAIN IF EXISTS id; + DROP FUNCTION IF EXISTS f(); + DROP FOREIGN DATA WRAPPER IF EXISTS fdw_wrapper; + DROP ROLE regress_dependency; +} + +session "s1" + +step "s1_begin" { BEGIN; } +step "s1_create_function_in_schema" { CREATE FUNCTION testschema.foo() RETURNS int AS 'select 1' LANGUAGE sql; } +step "s1_create_function_with_argtype" { CREATE FUNCTION fooargtype(num foo) RETURNS int AS 'select 1' LANGUAGE sql; } +step "s1_create_function_with_rettype" { CREATE FUNCTION footrettype() RETURNS id LANGUAGE sql RETURN 1; } +step "s1_create_function_with_function" { CREATE FUNCTION foofunc() RETURNS int LANGUAGE SQL RETURN f() + 1; } +step "s1_alter_function_owner" { ALTER FUNCTION public.falter() OWNER TO regress_dependency; } +step "s1_alter_function_schema" { ALTER FUNCTION public.falter() SET SCHEMA alterschema; } +step "s1_create_domain_with_domain" { CREATE DOMAIN idid as id; } +step "s1_create_table_with_type" { CREATE TABLE tabtype(a footab); } +step "s1_create_server_with_fdw_wrapper" { CREATE SERVER srv_fdw_wrapper FOREIGN DATA WRAPPER fdw_wrapper; } +step "s1_commit" { COMMIT; } + +session "s2" + +step "s2_begin" { BEGIN; } +step "s2_drop_schema" { DROP SCHEMA testschema; } +step "s2_drop_alterschema" { DROP SCHEMA alterschema; } +step "s2_drop_foo_type" { DROP TYPE public.foo; } +step "s2_drop_foo_rettype" { DROP DOMAIN id; } +step "s2_drop_footab_type" { DROP TYPE public.footab; } +step "s2_drop_function_f" { DROP FUNCTION f(); } +step "s2_drop_domain_id" { DROP DOMAIN id; } +step "s2_drop_fdw_wrapper" { DROP FOREIGN DATA WRAPPER fdw_wrapper RESTRICT; } +step "s2_drop_role" { DROP ROLE regress_dependency; } +step "s2_commit" { COMMIT; } + +# create function - drop schema +permutation "s1_begin" "s1_create_function_in_schema" "s2_drop_schema" "s1_commit" +permutation "s2_begin" "s2_drop_schema" "s1_create_function_in_schema" "s2_commit" + +# alter function - drop schema +permutation "s1_begin" "s1_alter_function_schema" "s2_drop_alterschema" "s1_commit" +permutation "s2_begin" "s2_drop_alterschema" "s1_alter_function_schema" "s2_commit" + +# create function - drop argtype +permutation "s1_begin" "s1_create_function_with_argtype" "s2_drop_foo_type" "s1_commit" +permutation "s2_begin" "s2_drop_foo_type" "s1_create_function_with_argtype" "s2_commit" + +# create function - drop rettype +permutation "s1_begin" "s1_create_function_with_rettype" "s2_drop_foo_rettype" "s1_commit" +permutation "s2_begin" "s2_drop_foo_rettype" "s1_create_function_with_rettype" "s2_commit" + +# create function - drop function used in its body +permutation "s1_begin" "s1_create_function_with_function" "s2_drop_function_f" "s1_commit" +permutation "s2_begin" "s2_drop_function_f" "s1_create_function_with_function" "s2_commit" + +# create domain over domain - drop the base domain +permutation "s1_begin" "s1_create_domain_with_domain" "s2_drop_domain_id" "s1_commit" +permutation "s2_begin" "s2_drop_domain_id" "s1_create_domain_with_domain" "s2_commit" + +# create table - drop type used in column +permutation "s1_begin" "s1_create_table_with_type" "s2_drop_footab_type" "s1_commit" +permutation "s2_begin" "s2_drop_footab_type" "s1_create_table_with_type" "s2_commit" + +# create server - drop foreign data wrapper +permutation "s1_begin" "s1_create_server_with_fdw_wrapper" "s2_drop_fdw_wrapper" "s1_commit" +permutation "s2_begin" "s2_drop_fdw_wrapper" "s1_create_server_with_fdw_wrapper" "s2_commit" + +# create function - drop owner role +permutation "s1_begin" "s1_alter_function_owner" "s2_drop_role" "s1_commit" + +# XXX: This permutation is disabled because the error message, "role +# was concurrently dropped", contains an OID that is not stable. +# +# permutation "s2_begin" "s2_drop_role" "s1_alter_function_owner" "s2_commit" diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index 632fbfdba84..4f5bd1e195f 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -2866,11 +2866,12 @@ begin; alter table alterlock2 add constraint alterlock2nv foreign key (f1) references alterlock (f1) NOT VALID; select * from my_locks order by 1; - relname | max_lockmode -------------+----------------------- - alterlock | ShareRowExclusiveLock - alterlock2 | ShareRowExclusiveLock -(2 rows) + relname | max_lockmode +----------------+----------------------- + alterlock | ShareRowExclusiveLock + alterlock2 | ShareRowExclusiveLock + alterlock_pkey | AccessShareLock +(3 rows) commit; begin;