Subject: Re: Avoid orphaned objects dependencies, take 3

From: Bertrand Drouvot <[email-masked]>
Date: Thu, 25 Apr 2024 07:20:05 +0000

Hi,

On Thu, Apr 25, 2024 at 09:00:00AM +0300, Alexander Lakhin wrote:
> Hi,
> 
> 25.04.2024 08:00, Bertrand Drouvot wrote:
> > 
> > > though concurrent create/drop operations can still produce
> > > the "cache lookup failed" error, which is probably okay, except that it is
> > > an INTERNAL_ERROR, which assumed to be not easily triggered by users.
> > I did not see any of those "cache lookup failed" during my testing with/without
> > your script. During which test(s) did you see those with v3 applied?
> 
> You can try, for example, table-trigger, or other tests that check for
> "cache lookup failed" psql output only (maybe you'll need to increase the
> iteration count). For instance, I've got (with v4 applied):
> 2024-04-25 05:48:08.102 UTC [3638763] ERROR:  cache lookup failed for function 20893
> 2024-04-25 05:48:08.102 UTC [3638763] STATEMENT:  CREATE TRIGGER modified_c1 BEFORE UPDATE OF c ON t
>         FOR EACH ROW WHEN (OLD.c <> NEW.c) EXECUTE PROCEDURE trigger_func('modified_c');
> 
> Or with function-function:
> 2024-04-25 05:52:31.390 UTC [3711897] ERROR:  cache lookup failed for function 32190 at character 54
> 2024-04-25 05:52:31.390 UTC [3711897] STATEMENT:  CREATE FUNCTION f1() RETURNS int LANGUAGE SQL RETURN f() + 1;
> --
> 2024-04-25 05:52:37.639 UTC [3720011] ERROR:  cache lookup failed for function 34465 at character 54
> 2024-04-25 05:52:37.639 UTC [3720011] STATEMENT:  CREATE FUNCTION f1() RETURNS int LANGUAGE SQL RETURN f() + 1;

I see, so this is during object creation.

It's easy to reproduce this kind of issue with gdb. For example set a breakpoint
on SearchSysCache1() and during the create function f1() once it breaks on:

#0  SearchSysCache1 (cacheId=45, key1=16400) at syscache.c:221
#1  0x00005ad305beacd6 in func_get_detail (funcname=0x5ad308204d50, fargs=0x0, fargnames=0x0, nargs=0, argtypes=0x7ffff2ff9cc0, expand_variadic=true, expand_defaults=true, include_out_arguments=false, funcid=0x7ffff2ff9ba0, rettype=0x7ffff2ff9b9c, retset=0x7ffff2ff9b94, nvargs=0x7ffff2ff9ba4,
    vatype=0x7ffff2ff9ba8, true_typeids=0x7ffff2ff9bd8, argdefaults=0x7ffff2ff9be0) at parse_func.c:1622
#2  0x00005ad305be7dd0 in ParseFuncOrColumn (pstate=0x5ad30823be98, funcname=0x5ad308204d50, fargs=0x0, last_srf=0x0, fn=0x5ad308204da0, proc_call=false, location=55) at parse_func.c:266
#3  0x00005ad305bdffb0 in transformFuncCall (pstate=0x5ad30823be98, fn=0x5ad308204da0) at parse_expr.c:1474
#4  0x00005ad305bdd2ee in transformExprRecurse (pstate=0x5ad30823be98, expr=0x5ad308204da0) at parse_expr.c:230
#5  0x00005ad305bdec34 in transformAExprOp (pstate=0x5ad30823be98, a=0x5ad308204e20) at parse_expr.c:990
#6  0x00005ad305bdd1a0 in transformExprRecurse (pstate=0x5ad30823be98, expr=0x5ad308204e20) at parse_expr.c:187
#7  0x00005ad305bdd00b in transformExpr (pstate=0x5ad30823be98, expr=0x5ad308204e20, exprKind=EXPR_KIND_SELECT_TARGET) at parse_expr.c:131
#8  0x00005ad305b96b7e in transformReturnStmt (pstate=0x5ad30823be98, stmt=0x5ad308204ee0) at analyze.c:2395
#9  0x00005ad305b92213 in transformStmt (pstate=0x5ad30823be98, parseTree=0x5ad308204ee0) at analyze.c:375
#10 0x00005ad305c6321a in interpret_AS_clause (languageOid=14, languageName=0x5ad308204c40 "sql", funcname=0x5ad308204ad8 "f100", as=0x0, sql_body_in=0x5ad308204ee0, parameterTypes=0x0, inParameterNames=0x0, prosrc_str_p=0x7ffff2ffa208, probin_str_p=0x7ffff2ffa200, sql_body_out=0x7ffff2ffa210,
    queryString=0x5ad3082040b0 "CREATE FUNCTION f100() RETURNS int LANGUAGE SQL RETURN f() + 1;") at functioncmds.c:953
#11 0x00005ad305c63c93 in CreateFunction (pstate=0x5ad308186310, stmt=0x5ad308204f00) at functioncmds.c:1221

then drop function f() in another session. Then the create function f1() would:

postgres=# CREATE FUNCTION f() RETURNS int LANGUAGE SQL RETURN f() + 1;
ERROR:  cache lookup failed for function 16400

This stuff does appear before we get a chance to call the new depLockAndCheckObject()
function.

I think this is what Tom was referring to in [1]:

"
So the only real fix for this would be to make every object lookup in the entire
system do the sort of dance that's done in RangeVarGetRelidExtended.
"

The fact that those kind of errors appear also somehow ensure that no orphaned
dependencies can be created.

The patch does ensure that no orphaned depencies can occur after those "initial"
look up are done (should the dependent object be dropped).

I'm tempted to not add extra complexity to avoid those kind of errors and keep the
patch as it is. All of those servicing the same goal: no orphaned depencies are
created.

[1]: https://www.postgresql.org/message-id/2872252.1630851337%40sss.pgh.pa.us

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: Alexander Lakhin <[email-masked]>
Date: Tue, 30 Apr 2024 20:00:00 +0300

Hi Bertrand,

> dependencies can be created.

I agree; the only thing that I'd change here, is the error code.

But I've discovered yet another possibility to get a broken dependency.
Please try this script:
res=0
numclients=20
for ((i=1;i<=100;i++)); do
for ((c=1;c<=numclients;c++)); do
   echo "
CREATE SCHEMA s_$c;
CREATE CONVERSION myconv_$c FOR 'LATIN1' TO 'UTF8' FROM iso8859_1_to_utf8;
ALTER CONVERSION myconv_$c SET SCHEMA s_$c;
   " | psql >psql1-$c.log 2>&1 &
   echo "DROP SCHEMA s_$c RESTRICT;" | psql >psql2-$c.log 2>&1 &
done
wait
pg_dump -f db.dump || { echo "on iteration $i"; res=1; break; }
for ((c=1;c<=numclients;c++)); do
   echo "DROP SCHEMA s_$c CASCADE;" | psql >psql3-$c.log 2>&1
done
done
psql -c "SELECT * FROM pg_conversion WHERE connamespace NOT IN (SELECT oid FROM pg_namespace);"

It fails for me (with the v4 patch applied) as follows:
pg_dump: error: schema with OID 16392 does not exist
on iteration 1
   oid  | conname  | connamespace | conowner | conforencoding | contoencoding |      conproc      | condefault
-------+----------+--------------+----------+----------------+---------------+-------------------+------------
  16396 | myconv_6 |        16392 |       10 |              8 |             6 | iso8859_1_to_utf8 | f

Best regards,
Alexander

---

From: Bertrand Drouvot <[email-masked]>
Date: Thu, 9 May 2024 12:20:51 +0000

Hi,

> But I've discovered yet another possibility to get a broken dependency.

Thanks for the testing and the report!

>   oid  | conname  | connamespace | conowner | conforencoding | contoencoding |      conproc      | condefault

Thanks for sharing the test, I'm able to reproduce the issue with v4.

Oh I see, your test updates an existing dependency. v4 took care about brand new 
dependencies creation (recordMultipleDependencies()) but forgot to take care
about changing an existing dependency (which is done in another code path:
changeDependencyFor()).

Please find attached v5 that adds:

- a call to the new depLockAndCheckObject() function in changeDependencyFor().
- a test when altering an existing dependency.

With v5 applied, I don't see the issue anymore.

Regards,

>From 6fc24a798210f730ab04833fa58074f142be968e Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email-masked]>
Date: Fri, 29 Mar 2024 15:43:26 +0000
Subject: [PATCH v5] 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              |  54 ++++++++
 src/backend/catalog/objectaddress.c           |  70 ++++++++++
 src/backend/catalog/pg_depend.c               |  12 ++
 src/include/catalog/dependency.h              |   1 +
 src/include/catalog/objectaddress.h           |   1 +
 src/test/modules/Makefile                     |   1 +
 src/test/modules/meson.build                  |   1 +
 .../test_dependencies_locks/.gitignore        |   3 +
 .../modules/test_dependencies_locks/Makefile  |  14 ++
 .../expected/test_dependencies_locks.out      | 129 ++++++++++++++++++
 .../test_dependencies_locks/meson.build       |  12 ++
 .../specs/test_dependencies_locks.spec        |  89 ++++++++++++
 12 files changed, 387 insertions(+)
  22.9% src/backend/catalog/
  43.7% src/test/modules/test_dependencies_locks/expected/
  27.2% src/test/modules/test_dependencies_locks/specs/
   4.5% src/test/modules/test_dependencies_locks/

diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index d4b5b2ade1..a49357bbe2 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -1517,6 +1517,60 @@ 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 dirty snaphot 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, errmsg("%s does not exist", object_description));
+			else
+				ereport(ERROR, 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..c2b873dd81 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -2590,6 +2590,76 @@ 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_dirty_snapshot)
+{
+	HeapTuple	tuple;
+	int			cache = -1;
+	const ObjectPropertyType *property;
+
+	if (!use_dirty_snapshot)
+	{
+		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;
+		SnapshotData DirtySnapshot;
+		Snapshot	snapshot;
+
+		if (use_dirty_snapshot)
+		{
+			InitDirtySnapshot(DirtySnapshot);
+			snapshot = &DirtySnapshot;
+		}
+		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/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..04891abcc1 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_dirty_snapshot);
 
 extern bool is_objectclass_supported(Oid class_id);
 extern const char *get_object_class_descr(Oid class_id);
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 256799f520..75f357100f 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -17,6 +17,7 @@ SUBDIRS = \
 		  test_copy_callbacks \
 		  test_custom_rmgrs \
 		  test_ddl_deparse \
+		  test_dependencies_locks \
 		  test_dsa \
 		  test_dsm_registry \
 		  test_extensions \
diff --git a/src/test/modules/meson.build b/src/test/modules/meson.build
index d8fe059d23..60305dcccd 100644
--- a/src/test/modules/meson.build
+++ b/src/test/modules/meson.build
@@ -16,6 +16,7 @@ subdir('test_bloomfilter')
 subdir('test_copy_callbacks')
 subdir('test_custom_rmgrs')
 subdir('test_ddl_deparse')
+subdir('test_dependencies_locks')
 subdir('test_dsa')
 subdir('test_dsm_registry')
 subdir('test_extensions')
diff --git a/src/test/modules/test_dependencies_locks/.gitignore b/src/test/modules/test_dependencies_locks/.gitignore
new file mode 100644
index 0000000000..bf000faac4
--- /dev/null
+++ b/src/test/modules/test_dependencies_locks/.gitignore
@@ -0,0 +1,3 @@
+# Generated subdirectories
+/log/
+/output_iso
diff --git a/src/test/modules/test_dependencies_locks/Makefile b/src/test/modules/test_dependencies_locks/Makefile
new file mode 100644
index 0000000000..7491048380
--- /dev/null
+++ b/src/test/modules/test_dependencies_locks/Makefile
@@ -0,0 +1,14 @@
+# src/test/modules/test_dependencies_locks/Makefile
+
+ISOLATION = test_dependencies_locks
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_dependencies_locks
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_dependencies_locks/expected/test_dependencies_locks.out b/src/test/modules/test_dependencies_locks/expected/test_dependencies_locks.out
new file mode 100644
index 0000000000..9b645d7aa5
--- /dev/null
+++ b/src/test/modules/test_dependencies_locks/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/modules/test_dependencies_locks/meson.build b/src/test/modules/test_dependencies_locks/meson.build
new file mode 100644
index 0000000000..92a978ab93
--- /dev/null
+++ b/src/test/modules/test_dependencies_locks/meson.build
@@ -0,0 +1,12 @@
+# Copyright (c) 2024, PostgreSQL Global Development Group
+
+tests += {
+  'name': 'test_dependencies_locks',
+  'sd': meson.current_source_dir(),
+  'bd': meson.current_build_dir(),
+  'isolation': {
+    'specs': [
+      'test_dependencies_locks',
+    ],
+  },
+}
diff --git a/src/test/modules/test_dependencies_locks/specs/test_dependencies_locks.spec b/src/test/modules/test_dependencies_locks/specs/test_dependencies_locks.spec
new file mode 100644
index 0000000000..5d04dfe9dc
--- /dev/null
+++ b/src/test/modules/test_dependencies_locks/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

---

From: Alexander Lakhin <[email-masked]>
Date: Tue, 14 May 2024 15:00:00 +0300

Hi Bertrand,

> With v5 applied, I don't see the issue anymore.

Me too. Thank you for the improved version!
I will test the patch in the background, but for now I see no other
issues with it.

Best regards,
Alexander

---

From: Michael Paquier <[email-masked]>
Date: Wed, 15 May 2024 10:14:09 +0900

+            if (object_description)
+                ereport(ERROR, errmsg("%s does not exist", object_description));
+            else
+                ereport(ERROR, errmsg("a dependent object does not ex

This generates an internal error code.  Is that intended?

--- /dev/null

This introduces a module with only one single spec.  I could get
behind an extra module if splitting the tests into more specs makes
sense or if there is a restriction on installcheck.  However, for 
one spec file filed with a bunch of objects, and note that I'm OK to
let this single spec grow more for this range of tests, it seems to me
that this would be better under src/test/isolation/.

I'm wondering if Robert has a comment about that.  It looks backwards
in a world where we try to encourage MVCC snapshots for catalog
scans (aka 568d4138c646), especially for the part related to
dependency.c and ObjectAddresses.
--
Michael

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 15 May 2024 08:31:43 +0000

Hi,

> This generates an internal error code.  Is that intended?

Thanks for looking at it!

Yes, it's like when say you want to create an object in a schema that does not
exist (see get_namespace_oid()).

> that this would be better under src/test/isolation/.

Yeah, I was not sure about this one (the location is from take 2 mentioned
up-thread). I'll look at moving the tests to src/test/isolation/.

Regards,

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 15 May 2024 08:33:05 +0000

Hi,

> I will test the patch in the background, but for now I see no other

Thanks for confirming!

Regards,

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 15 May 2024 10:31:02 +0000

Hi,

> up-thread). I'll look at moving the tests to src/test/isolation/.

Please find attached v6 (only diff with v5 is moving the tests as suggested
above).

Regards,

>From 59f7befd34b8aa4ba0483a100e85bacbc1a76707 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email-masked]>
Date: Fri, 29 Mar 2024 15:43:26 +0000
Subject: [PATCH v6] Avoid orphaned objects dependencies

Scenario 1:

Scenario 2:

- 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              |  54 ++++++++
 src/backend/catalog/objectaddress.c           |  70 ++++++++++
 src/backend/catalog/pg_depend.c               |  12 ++
 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 ++++++++++++
 8 files changed, 357 insertions(+)
  24.1% src/backend/catalog/
  45.9% src/test/isolation/expected/
  28.6% src/test/isolation/specs/

+/*
 
+/*
+ * ObjectByIdExist
+ *
 
+		/*
 
+	/*

 

 
diff --git a/src/test/isolation/expected/test_dependencies_locks.out b/src/test/isolation/expected/test_dependencies_locks.out
+++ b/src/test/isolation/expected/test_dependencies_locks.out
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
+++ b/src/test/isolation/specs/test_dependencies_locks.spec

---

From: Alexander Lakhin <[email-masked]>
Date: Sun, 19 May 2024 11:00:00 +0300

Hello Bertrand,

> exist (see get_namespace_oid()).

AFAICS, get_namespace_oid() throws not ERRCODE_INTERNAL_ERROR,
but ERRCODE_UNDEFINED_SCHEMA:

# SELECT regtype('unknown_schema.type');
ERROR:  schema "unknown_schema" does not exist
LINE 1: SELECT regtype('unknown_schema.type');
                        ^
# \echo :LAST_ERROR_SQLSTATE
3F000

Probably, it's worth to avoid ERRCODE_INTERNAL_ERROR here in light of [1]
and [2], as these errors are not that abnormal (not Assert-like).

[1] https://www.postgresql.org/message-id/Zic_GNgos5sMxKoa%40paquier.xyz
[2] https://commitfest.postgresql.org/48/4735/

Best regards,
Alexander

---

From: Bertrand Drouvot <[email-masked]>
Date: Tue, 21 May 2024 07:22:57 +0000

Hi,

> [2] https://commitfest.postgresql.org/48/4735/

Thanks for mentioning the above examples, I agree that it's worth to avoid
ERRCODE_INTERNAL_ERROR here: please find attached v7 that makes use of a new
ERRCODE: ERRCODE_DEPENDENT_OBJECTS_DOES_NOT_EXIST 

I thought about this name as it is close enough to the already existing 
"ERRCODE_DEPENDENT_OBJECTS_STILL_EXIST" but I'm open to suggestion too.

Regards,

>From 79809f88311ef1cf4e8f3250e1508fe0b7d86602 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email-masked]>
Date: Fri, 29 Mar 2024 15:43:26 +0000
Subject: [PATCH v7] Avoid orphaned objects dependencies

Scenario 1:

Scenario 2:

- 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              |  58 ++++++++
 src/backend/catalog/objectaddress.c           |  70 ++++++++++
 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, 362 insertions(+)
  24.6% src/backend/catalog/
  45.1% src/test/isolation/expected/
  28.1% src/test/isolation/specs/

diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index d4b5b2ade1..89b2f18f98 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -1517,6 +1517,64 @@ AcquireDeletionLock(const ObjectAddress *object, int flags)
 	}
 }
 
+/*
+				ereport(ERROR,
+						(errcode(ERRCODE_DEPENDENT_OBJECTS_DOES_NOT_EXIST),
+						 errmsg("%s does not exist", object_description)));
+				ereport(ERROR,
+						(errcode(ERRCODE_DEPENDENT_OBJECTS_DOES_NOT_EXIST),
+						 errmsg("a dependent object does not exist")));
 
+/*
+ * ObjectByIdExist
+ *
 
+		/*
 
+	/*
 
 	/* 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

---

From: Robert Haas <[email-masked]>
Date: Tue, 21 May 2024 08:53:06 -0400

> Please find attached v6 (only diff with v5 is moving the tests as suggested

I don't immediately know what to think about this patch. I've known
about this issue for a long time, but I didn't think this was how we
would fix it.

I think the problem here is basically that we don't lock namespaces
(schemas) when we're adding and removing things from the schema. So I
assumed that if we ever did something about this, what we would do
would be add a bunch of calls to lock schemas to the appropriate parts
of the code. What you've done here instead is add locking at a much
lower level - whenever we are adding a dependency on an object, we
lock the object. The advantage of that approach is that we definitely
won't miss anything. The disadvantage of that approach is that it
means we have some very low-level code taking locks, which means it's
not obvious which operations are taking what locks. Maybe it could
even result in some redundancy, like the higher-level code taking a
lock also (possibly in a different mode) and then this code taking
another one.

I haven't gone through the previous threads; it sounds like there's
already been some discussion of this, but I'm just telling you how it
strikes me on first look.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 22 May 2024 10:21:34 +0000

Hi,

On Tue, May 21, 2024 at 08:53:06AM -0400, Robert Haas wrote:
> I don't immediately know what to think about this patch.

> I've known about this issue for a long time, but I didn't think this was how we
> would fix it.

I started initially with [1] but it was focusing on function-schema only.

Then I proposed [2] making use of a dirty snapshot when recording the dependency.
But this approach appeared to be "scary" and it was still failing to close
some race conditions.

Then, Tom proposed another approach in [2] which is that "creation DDL will have
to take a lock on each referenced object that'd conflict with a lock taken by DROP".
This is the one the current patch is trying to implement.

> What you've done here instead is add locking at a much
> won't miss anything.

Right, as there is much more than the ones related to schemas, for example:

- function and arg type

to name a few.

> The disadvantage of that approach is that it
> not obvious which operations are taking what locks.

Right, but the new operations are "only" the ones leading to recording or altering
a dependency.

> Maybe it could

The one that is added here is in AccessShareLock mode. It could conflict with
the ones in AccessExclusiveLock means (If I'm not missing any):

- AcquireDeletionLock(): which is exactly what we want
- get_object_address()
    - get_object_address_rv()
        - ExecAlterObjectDependsStmt()
    - ExecRenameStmt()
    - ExecAlterObjectDependsStmt()
    - ExecAlterOwnerStmt()
    - RemoveObjects()
- AlterPublication()

I think there is 2 cases here:

First case: the "conflicting" lock mode is for one of our own lock then LockCheckConflicts()
would report this as a NON conflict.

Second case: the "conflicting" lock mode is NOT for one of our own lock then LockCheckConflicts()
would report a conflict. But I've the feeling that the existing code would
already lock those sessions.

One example where it would be the case:

Session 1: doing "BEGIN; ALTER FUNCTION noschemas.foo2() SET SCHEMA alterschema" would
acquire the lock in AccessExclusiveLock during ExecAlterObjectSchemaStmt()->get_object_address()->LockDatabaseObject()
(in the existing code and before the new call that would occur through changeDependencyFor()->depLockAndCheckObject()
with the patch in place).

Then, session 2: doing "alter function noschemas.foo2() owner to newrole;"
would be locked in the existing code while doing ExecAlterOwnerStmt()->get_object_address()->LockDatabaseObject()).

Means that in this example, the new lock this patch is introducing would not be
responsible of session 2 beging locked.

Was your concern about "First case" or "Second case" or both?

[1]: https://www.postgresql.org/message-id/flat/5a9daaae-5538-b209-6279-e903c3ea2157%40amazon.com
[2]: https://www.postgresql.org/message-id/flat/8369ff70-0e31-f194-2954-787f4d9e21dd%40amazon.com

Regards,