From 2611fb874a476c1a8d665f97d973193beb70292a Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Fri, 29 Mar 2024 15:43:26 +0000
Subject: [PATCH v8] Avoid orphaned objects dependencies

It's currently possible to create orphaned objects dependencies, for example:

Scenario 1:

session 1: begin; drop schema schem;
session 2: create a function in the schema schem
session 1: commit;

With the above, the function created in session 2 would be linked to a non
existing schema.

Scenario 2:

session 1: begin; create a function in the schema schem
session 2: drop schema schem;
session 1: commit;

With the above, the function created in session 1 would be linked to a non
existing schema.

To avoid those scenarios, a new lock (that conflicts with a lock taken by DROP)
has been put in place when the dependencies are being recorded. With this in
place, the drop schema in scenario 2 would be locked.

Also, after the new lock attempt, the patch checks that the object still exists:
with this in place session 2 in scenario 1 would be locked and would report an
error once session 1 committs (that would not be the case should session 1 abort
the transaction).

If the object is dropped before the new lock attempt is triggered then the patch
would also report an error (but with less details).

The patch adds a few tests for some dependency cases (that would currently produce
orphaned objects):

- schema and function (as the above scenarios)
- alter a dependency (function and schema)
- function and arg type
- function and return type
- function and function
- domain and domain
- table and type
- server and foreign data wrapper
---
 src/backend/catalog/dependency.c              |  59 ++++++++
 src/backend/catalog/objectaddress.c           |  66 +++++++++
 src/backend/catalog/pg_depend.c               |  12 ++
 src/backend/utils/errcodes.txt                |   1 +
 src/include/catalog/dependency.h              |   1 +
 src/include/catalog/objectaddress.h           |   1 +
 .../expected/test_dependencies_locks.out      | 129 ++++++++++++++++++
 src/test/isolation/isolation_schedule         |   1 +
 .../specs/test_dependencies_locks.spec        |  89 ++++++++++++
 9 files changed, 359 insertions(+)
  24.3% src/backend/catalog/
  45.3% src/test/isolation/expected/
  28.2% src/test/isolation/specs/

diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index d4b5b2ade1..4271ed7c5b 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -1517,6 +1517,65 @@ AcquireDeletionLock(const ObjectAddress *object, int flags)
 	}
 }
 
+/*
+ * depLockAndCheckObject
+ *
+ * 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 the object has been dropped, this function
+ * does not return!
+ */
+void
+depLockAndCheckObject(const ObjectAddress *object)
+{
+	char	   *object_description;
+
+	/*
+	 * Those don't rely on LockDatabaseObject() when being dropped (see
+	 * AcquireDeletionLock()). Also it looks like they can not produce
+	 * orphaned dependent objects when being dropped.
+	 */
+	if (object->classId == RelationRelationId || object->classId == AuthMemRelationId)
+		return;
+
+	object_description = getObjectDescription(object, true);
+
+	/* assume we should lock the whole object not a sub-object */
+	LockDatabaseObject(object->classId, object->objectId, 0, AccessShareLock);
+
+	/* check if object still exists */
+	if (!ObjectByIdExist(object, false))
+	{
+		/*
+		 * It might be possible that we are creating it (for example creating
+		 * a composite type while creating a relation), so bypass the syscache
+		 * lookup and use a SnapshotSelf snapshot instead to cover this
+		 * scenario.
+		 */
+		if (!ObjectByIdExist(object, true))
+		{
+			/*
+			 * If the object has been dropped before we get a chance to get
+			 * its description, then emit a generic error message. That looks
+			 * like a good compromise over extra complexity.
+			 */
+			if (object_description)
+				ereport(ERROR,
+						(errcode(ERRCODE_DEPENDENT_OBJECTS_DOES_NOT_EXIST),
+						 errmsg("%s does not exist", object_description)));
+			else
+				ereport(ERROR,
+						(errcode(ERRCODE_DEPENDENT_OBJECTS_DOES_NOT_EXIST),
+						 errmsg("a dependent object does not exist")));
+		}
+	}
+
+	if (object_description)
+		pfree(object_description);
+
+	return;
+}
+
 /*
  * ReleaseDeletionLock - release an object deletion lock
  *
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index 7b536ac6fd..115cb1572e 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -2590,6 +2590,72 @@ get_object_namespace(const ObjectAddress *address)
 	return oid;
 }
 
+/*
+ * ObjectByIdExist
+ *
+ * Return whether the given object exists.
+ *
+ * Works for most catalogs, if no special processing is needed.
+ */
+bool
+ObjectByIdExist(const ObjectAddress *address, bool use_snapshot_self)
+{
+	HeapTuple	tuple;
+	int			cache = -1;
+	const ObjectPropertyType *property;
+
+	if (!use_snapshot_self)
+	{
+		property = get_object_property_data(address->classId);
+
+		cache = property->oid_catcache_id;
+	}
+
+	if (cache >= 0)
+	{
+		/* Fetch tuple from syscache. */
+		tuple = SearchSysCache1(cache, ObjectIdGetDatum(address->objectId));
+
+		if (!HeapTupleIsValid(tuple))
+		{
+			return false;
+		}
+
+		ReleaseSysCache(tuple);
+
+		return true;
+	}
+	else
+	{
+		Relation	rel;
+		ScanKeyData skey[1];
+		SysScanDesc scan;
+		Snapshot	snapshot;
+
+		if (use_snapshot_self)
+			snapshot = SnapshotSelf;
+		else
+			snapshot = NULL;
+
+		rel = table_open(address->classId, AccessShareLock);
+
+		ScanKeyInit(&skey[0],
+					get_object_attnum_oid(address->classId),
+					BTEqualStrategyNumber, F_OIDEQ,
+					ObjectIdGetDatum(address->objectId));
+
+		scan = systable_beginscan(rel, get_object_oid_index(address->classId), true,
+								  snapshot, 1, skey);
+
+		/* we expect exactly one match */
+		tuple = systable_getnext(scan);
+		systable_endscan(scan);
+		table_close(rel, AccessShareLock);
+
+		return (HeapTupleIsValid(tuple));
+	}
+}
+
 /*
  * Return ObjectType for the given object type as given by
  * getObjectTypeDescription; if no valid ObjectType code exists, but it's a
diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c
index 5366f7820c..f16af28429 100644
--- a/src/backend/catalog/pg_depend.c
+++ b/src/backend/catalog/pg_depend.c
@@ -108,6 +108,12 @@ recordMultipleDependencies(const ObjectAddress *depender,
 		if (isObjectPinned(referenced))
 			continue;
 
+		/*
+		 * Acquire a lock and check object still exists while recording the
+		 * dependency. XXX - Should we do so only for DEPENDENCY_NORMAL?
+		 */
+		depLockAndCheckObject(referenced);
+
 		if (slot_init_count < max_slots)
 		{
 			slot[slot_stored_count] = MakeSingleTupleTableSlot(RelationGetDescr(dependDesc),
@@ -506,6 +512,12 @@ changeDependencyFor(Oid classId, Oid objectId,
 		return 1;
 	}
 
+	/*
+	 * Acquire a lock and check object still exists while changing the
+	 * dependency.
+	 */
+	depLockAndCheckObject(&objAddr);
+
 	depRel = table_open(DependRelationId, RowExclusiveLock);
 
 	/* There should be existing dependency record(s), so search. */
diff --git a/src/backend/utils/errcodes.txt b/src/backend/utils/errcodes.txt
index 3250d539e1..60e8539fe3 100644
--- a/src/backend/utils/errcodes.txt
+++ b/src/backend/utils/errcodes.txt
@@ -271,6 +271,7 @@ Section: Class 28 - Invalid Authorization Specification
 Section: Class 2B - Dependent Privilege Descriptors Still Exist
 
 2B000    E    ERRCODE_DEPENDENT_PRIVILEGE_DESCRIPTORS_STILL_EXIST            dependent_privilege_descriptors_still_exist
+2BP02    E    ERRCODE_DEPENDENT_OBJECTS_DOES_NOT_EXIST                       dependent_objects_does_not_exist
 2BP01    E    ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST                          dependent_objects_still_exist
 
 Section: Class 2D - Invalid Transaction Termination
diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h
index 7eee66f810..5619b6f55e 100644
--- a/src/include/catalog/dependency.h
+++ b/src/include/catalog/dependency.h
@@ -101,6 +101,7 @@ typedef struct ObjectAddresses ObjectAddresses;
 /* in dependency.c */
 
 extern void AcquireDeletionLock(const ObjectAddress *object, int flags);
+extern void depLockAndCheckObject(const ObjectAddress *object);
 
 extern void ReleaseDeletionLock(const ObjectAddress *object);
 
diff --git a/src/include/catalog/objectaddress.h b/src/include/catalog/objectaddress.h
index 3a70d80e32..53ee9ad9e6 100644
--- a/src/include/catalog/objectaddress.h
+++ b/src/include/catalog/objectaddress.h
@@ -53,6 +53,7 @@ extern void check_object_ownership(Oid roleid,
 								   Node *object, Relation relation);
 
 extern Oid	get_object_namespace(const ObjectAddress *address);
+extern bool ObjectByIdExist(const ObjectAddress *address, bool use_snapshot_self);
 
 extern bool is_objectclass_supported(Oid class_id);
 extern const char *get_object_class_descr(Oid class_id);
diff --git a/src/test/isolation/expected/test_dependencies_locks.out b/src/test/isolation/expected/test_dependencies_locks.out
new file mode 100644
index 0000000000..9b645d7aa5
--- /dev/null
+++ b/src/test/isolation/expected/test_dependencies_locks.out
@@ -0,0 +1,129 @@
+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; <waiting ...>
+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; <waiting ...>
+step s2_commit: COMMIT;
+step s1_create_function_in_schema: <... completed>
+ERROR:  schema testschema does not exist
+
+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; <waiting ...>
+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; <waiting ...>
+step s2_commit: COMMIT;
+step s1_alter_function_schema: <... completed>
+ERROR:  schema alterschema does not exist
+
+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; <waiting ...>
+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; <waiting ...>
+step s2_commit: COMMIT;
+step s1_create_function_with_argtype: <... completed>
+ERROR:  type foo does not exist
+
+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; <waiting ...>
+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; <waiting ...>
+step s2_commit: COMMIT;
+step s1_create_function_with_rettype: <... completed>
+ERROR:  type id does not exist
+
+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(); <waiting ...>
+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; <waiting ...>
+step s2_commit: COMMIT;
+step s1_create_function_with_function: <... completed>
+ERROR:  function f() does not exist
+
+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; <waiting ...>
+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; <waiting ...>
+step s2_commit: COMMIT;
+step s1_create_domain_with_domain: <... completed>
+ERROR:  type id does not exist
+
+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; <waiting ...>
+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); <waiting ...>
+step s2_commit: COMMIT;
+step s1_create_table_with_type: <... completed>
+ERROR:  type footab does not exist
+
+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; <waiting ...>
+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; <waiting ...>
+step s2_commit: COMMIT;
+step s1_create_server_with_fdw_wrapper: <... completed>
+ERROR:  foreign-data wrapper fdw_wrapper does not exist
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 0342eb39e4..1b67f0bffe 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -114,3 +114,4 @@ test: serializable-parallel-2
 test: serializable-parallel-3
 test: matview-write-skew
 test: lock-nowait
+test: test_dependencies_locks
diff --git a/src/test/isolation/specs/test_dependencies_locks.spec b/src/test/isolation/specs/test_dependencies_locks.spec
new file mode 100644
index 0000000000..5d04dfe9dc
--- /dev/null
+++ b/src/test/isolation/specs/test_dependencies_locks.spec
@@ -0,0 +1,89 @@
+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;
+}
+
+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;
+}
+
+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_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_commit" { COMMIT; }
+
+# function - 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 - 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"
+
+# function - 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"
+
+# function - 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"
+
+# function - function
+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"
+
+# domain - 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"
+
+# table - type
+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"
+
+# server - 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"
-- 
2.34.1

