agora inbox for [email protected]
help / color / mirror / Atom feedFrom: Bertrand Drouvot <[email protected]>
Subject: [PATCH v3] Avoid orphaned objects dependencies
Date: Fri, 29 Mar 2024 15:43:26 +0000
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 locking the object, 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).
The patch adds a few tests for some dependency cases (that would currently produce
orphaned objects):
- schema and function (as the above scenarios)
- function and type
- table and type
---
src/backend/catalog/dependency.c | 54 ++++++++++++++
src/backend/catalog/objectaddress.c | 70 +++++++++++++++++++
src/backend/catalog/pg_depend.c | 6 ++
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 | 49 +++++++++++++
.../test_dependencies_locks/meson.build | 12 ++++
.../specs/test_dependencies_locks.spec | 39 +++++++++++
12 files changed, 251 insertions(+)
40.5% src/backend/catalog/
29.6% src/test/modules/test_dependencies_locks/expected/
18.7% src/test/modules/test_dependencies_locks/specs/
8.3% 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 f85a898de8..6bb218f5dd 100644
--- a/src/backend/catalog/pg_depend.c
+++ b/src/backend/catalog/pg_depend.c
@@ -106,6 +106,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),
diff --git a/src/include/catalog/dependency.h b/src/include/catalog/dependency.h
index ec654010d4..8915548711 100644
--- a/src/include/catalog/dependency.h
+++ b/src/include/catalog/dependency.h
@@ -94,6 +94,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..d0980f77d5
--- /dev/null
+++ b/src/test/modules/test_dependencies_locks/expected/test_dependencies_locks.out
@@ -0,0 +1,49 @@
+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_create_function_with_type s2_drop_foo_type s1_commit
+step s1_begin: BEGIN;
+step s1_create_function_with_type: CREATE FUNCTION footype(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_type s2_commit
+step s2_begin: BEGIN;
+step s2_drop_foo_type: DROP TYPE public.foo;
+step s1_create_function_with_type: CREATE FUNCTION footype(num foo) RETURNS int AS 'select 1' LANGUAGE sql; <waiting ...>
+step s2_commit: COMMIT;
+step s1_create_function_with_type: <... completed>
+ERROR: type foo 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
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..fd15bd2a78
--- /dev/null
+++ b/src/test/modules/test_dependencies_locks/specs/test_dependencies_locks.spec
@@ -0,0 +1,39 @@
+setup
+{
+ CREATE SCHEMA testschema;
+ CREATE TYPE public.foo as enum ('one', 'two');
+ CREATE TYPE public.footab as enum ('three', 'four');
+}
+
+teardown
+{
+ DROP FUNCTION IF EXISTS testschema.foo();
+ DROP FUNCTION IF EXISTS footype(num foo);
+ DROP TABLE IF EXISTS tabtype;
+ DROP SCHEMA IF EXISTS testschema;
+ DROP TYPE IF EXISTS public.foo;
+ DROP TYPE IF EXISTS public.footab;
+}
+
+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_type" { CREATE FUNCTION footype(num foo) RETURNS int AS 'select 1' LANGUAGE sql; }
+step "s1_create_table_with_type" { CREATE TABLE tabtype(a footab); }
+step "s1_commit" { COMMIT; }
+
+session "s2"
+
+step "s2_begin" { BEGIN; }
+step "s2_drop_schema" { DROP SCHEMA testschema; }
+step "s2_drop_foo_type" { DROP TYPE public.foo; }
+step "s2_drop_footab_type" { DROP TYPE public.footab; }
+step "s2_commit" { COMMIT; }
+
+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"
+permutation "s1_begin" "s1_create_function_with_type" "s2_drop_foo_type" "s1_commit"
+permutation "s2_begin" "s2_drop_foo_type" "s1_create_function_with_type" "s2_commit"
+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"
--
2.34.1
--+Mfbf1cGg0Kwp7Pc--
view thread (17190+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected]
Subject: Re: [PATCH v3] Avoid orphaned objects dependencies
In-Reply-To: <no-message-id-194087@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox