From 4dd9fb201ead3325f085481e9adca9f2fd8acec3 Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat Date: Thu, 27 Aug 2026 21:07:12 +0530 Subject: [PATCH v20260827 2/2] WIP: DROP CASCADE and orphaned property graph labels and properties When dropping objects from a property graph through ALTER PROPERTY GRAPH, it explicitly drops the orphaned labels and properties. But when a DROP CASCADEs to property graph objects, it may leave orphaned labels and properties in the database. This leads to errors when a label with the same name as the orphaned label or a property with the same name as the orphaned property are created with characteristics different from the orphaned one. A pg_propgraph_property entry needs to be removed when the last referencing pg_propgraph_label_property entry is deleted. Similarly a pg_propgraph_label entry needs to be removed when the last referencing pg_propgraph_element_label entry is deleted. Right now there is no way to express a dependency which depends upon the reference count in pg_depend. Ideally we should add invent a new dependency for this and let the dependency tracking infrastructure drop the orphaned properties and labels. But doing so nearer to the release is not possible and the mechanism needs to be well thought through. Instead we modify performDeletion() and performMultipleDeletions() to collect the pg_propgraph_property and pg_propgraph_label entries from the property graph objects being dropped. We use this collection to find out the possibly orphaned entries and delete them. While doing so we take the lock on their parent property graphs so as to avoid inconsistencies being caused by a concurrent ALTER PROPERTY GRAPH. In order to avoid code duplication both AlterPropGraph() and perform*Deletion() code use the same function to remove orphaned objects. Before this change, AlterPropGraph() collected and removed all orphaned entries only ones at the end of the command. Now it would do this for every deletion. Given that ALTER PROPERTY GRAPH can drop only one parent object at a time, the probability of that leading to multiple deletions cascading to a single orphaned object is low. So there's net performance gain with the new approach. Reported-by: Andres Freund Author: Ashutosh Bapat Reviewed-by: TBD Discussion: https://postgr.es/m/dqa5mstx5mna3i7s23pdwl4m6bek7gqsgfccef44wjpswizufi@3aa6vzri3cat Backpatch-through: 19 --- src/backend/catalog/dependency.c | 98 ++++++++++ src/backend/commands/propgraphcmds.c | 177 ++++++++++-------- src/include/commands/propgraphcmds.h | 3 + .../expected/alter-propgraph-drop-cascade.out | 51 +++++ src/test/isolation/isolation_schedule | 1 + .../specs/alter-propgraph-drop-cascade.spec | 47 +++++ .../expected/create_property_graph.out | 9 + .../regress/sql/create_property_graph.sql | 6 + 8 files changed, 312 insertions(+), 80 deletions(-) create mode 100644 src/test/isolation/expected/alter-propgraph-drop-cascade.out create mode 100644 src/test/isolation/specs/alter-propgraph-drop-cascade.spec diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c index 55ee42f8dfc..3f23765ef30 100644 --- a/src/backend/catalog/dependency.c +++ b/src/backend/catalog/dependency.c @@ -76,6 +76,7 @@ #include "commands/event_trigger.h" #include "commands/extension.h" #include "commands/policy.h" +#include "commands/propgraphcmds.h" #include "commands/publicationcmds.h" #include "commands/seclabel.h" #include "commands/sequence.h" @@ -163,6 +164,10 @@ static void reportDependentObjects(const ObjectAddresses *targetObjects, static void deleteOneObject(const ObjectAddress *object, Relation *depRel, int32 flags); static void doDeletion(const ObjectAddress *object, int flags); +static void collectPropGraphCleanupCandidates(const ObjectAddresses *targetObjects, + Relation depRel, + List **labeloids, + List **propoids); static bool find_expr_references_walker(Node *node, find_expr_references_context *context); static void process_function_rte_ref(RangeTblEntry *rte, AttrNumber attnum, @@ -236,6 +241,77 @@ deleteObjectsInList(ObjectAddresses *targetObjects, Relation *depRel, } } +/* + * Collect labels and properties that might become orphaned when deleting + * property graph component objects. Existing dependency types cannot express + * deleting a referenced object only when its last dependent is removed, so + * these objects require a post-deletion cleanup pass. + */ +static void +collectPropGraphCleanupCandidates(const ObjectAddresses *targetObjects, + Relation depRel, List **labeloids, + List **propoids) +{ + for (int i = 0; i < targetObjects->numrefs; i++) + { + const ObjectAddress *object = &targetObjects->refs[i]; + Oid refobjid = InvalidOid; + Oid refclassid; + List **oids; + ScanKeyData key[2]; + SysScanDesc scan; + HeapTuple tup; + + if (object->classId == PropgraphElementLabelRelationId) + { + refclassid = PropgraphLabelRelationId; + oids = labeloids; + } + else if (object->classId == PropgraphLabelPropertyRelationId) + { + refclassid = PropgraphPropertyRelationId; + oids = propoids; + } + else + continue; + + ScanKeyInit(&key[0], + Anum_pg_depend_classid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(object->classId)); + ScanKeyInit(&key[1], + Anum_pg_depend_objid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(object->objectId)); + + scan = systable_beginscan(depRel, DependDependerIndexId, true, + NULL, 2, key); + while (HeapTupleIsValid(tup = systable_getnext(scan))) + { + Form_pg_depend depform = (Form_pg_depend) GETSTRUCT(tup); + + if (depform->refclassid == refclassid && + depform->deptype == DEPENDENCY_AUTO) + { + if (OidIsValid(refobjid)) + elog(ERROR, "multiple references found for property graph component %u", + object->objectId); + + refobjid = depform->refobjid; + } + } + systable_endscan(scan); + + /* + * Each pg_propgraph_label_property entry should have one and only one + * pg_propgraph_property entry. Similarly for + * pg_propgraph_element_label entry and pg_propgraph_label entry. + */ + Assert(OidIsValid(refobjid)); + *oids = list_append_unique_oid(*oids, refobjid); + } +} + /* * performDeletion: attempt to drop the specified object. If CASCADE * behavior is specified, also drop any dependent objects (recursively). @@ -281,6 +357,8 @@ performDeletion(const ObjectAddress *object, { Relation depRel; ObjectAddresses *targetObjects; + List *labeloids = NIL; + List *propoids = NIL; /* * We save some cycles by opening pg_depend just once and passing the @@ -315,6 +393,8 @@ performDeletion(const ObjectAddress *object, behavior, flags, object); + collectPropGraphCleanupCandidates(targetObjects, depRel, + &labeloids, &propoids); /* do the deed */ deleteObjectsInList(targetObjects, &depRel, flags); @@ -323,6 +403,13 @@ performDeletion(const ObjectAddress *object, free_object_addresses(targetObjects); table_close(depRel, RowExclusiveLock); + + RemoveOrphanedPropGraphObjects(labeloids, propoids, behavior, + flags & (PERFORM_DELETION_INTERNAL | + PERFORM_DELETION_QUIETLY | + PERFORM_DELETION_SKIP_EXTENSIONS)); + list_free(labeloids); + list_free(propoids); } /* @@ -340,6 +427,8 @@ performMultipleDeletions(const ObjectAddresses *objects, { Relation depRel; ObjectAddresses *targetObjects; + List *labeloids = NIL; + List *propoids = NIL; int i; /* No work if no objects... */ @@ -391,6 +480,8 @@ performMultipleDeletions(const ObjectAddresses *objects, behavior, flags, (objects->numrefs == 1 ? objects->refs : NULL)); + collectPropGraphCleanupCandidates(targetObjects, depRel, + &labeloids, &propoids); /* do the deed */ deleteObjectsInList(targetObjects, &depRel, flags); @@ -399,6 +490,13 @@ performMultipleDeletions(const ObjectAddresses *objects, free_object_addresses(targetObjects); table_close(depRel, RowExclusiveLock); + + RemoveOrphanedPropGraphObjects(labeloids, propoids, behavior, + flags & (PERFORM_DELETION_INTERNAL | + PERFORM_DELETION_QUIETLY | + PERFORM_DELETION_SKIP_EXTENSIONS)); + list_free(labeloids); + list_free(propoids); } /* diff --git a/src/backend/commands/propgraphcmds.c b/src/backend/commands/propgraphcmds.c index 2bdce331c49..9c22c539163 100644 --- a/src/backend/commands/propgraphcmds.c +++ b/src/backend/commands/propgraphcmds.c @@ -38,6 +38,7 @@ #include "parser/parse_oper.h" #include "parser/parse_relation.h" #include "parser/parse_target.h" +#include "storage/lmgr.h" #include "utils/acl.h" #include "utils/array.h" #include "utils/builtins.h" @@ -96,7 +97,6 @@ static Oid get_element_relid(Oid peid); static List *get_graph_label_ids(Oid graphid); static List *get_label_element_label_ids(Oid labelid); static List *get_element_label_property_names(Oid ellabeloid); -static List *get_graph_property_ids(Oid graphid); /* @@ -318,6 +318,102 @@ CreatePropGraph(ParseState *pstate, const CreatePropGraphStmt *stmt) return pgaddress; } +/* + * Remove labels and properties left unused after deleting property graph + * component objects. + */ +void +RemoveOrphanedPropGraphObjects(const List *labeloids, const List *propoids, + DropBehavior behavior, int flags) +{ + List *graphoids = NIL; + + foreach_oid(labeloid, labeloids) + { + HeapTuple tuple; + + tuple = SearchSysCache1(PROPGRAPHLABELOID, + ObjectIdGetDatum(labeloid)); + if (HeapTupleIsValid(tuple)) + { + Form_pg_propgraph_label label = (Form_pg_propgraph_label) GETSTRUCT(tuple); + + graphoids = list_append_unique_oid(graphoids, label->pglpgid); + ReleaseSysCache(tuple); + } + } + + foreach_oid(propoid, propoids) + { + HeapTuple tuple; + + tuple = SearchSysCache1(PROPGRAPHPROPOID, + ObjectIdGetDatum(propoid)); + if (HeapTupleIsValid(tuple)) + { + Form_pg_propgraph_property property = (Form_pg_propgraph_property) GETSTRUCT(tuple); + + graphoids = list_append_unique_oid(graphoids, property->pgppgid); + ReleaseSysCache(tuple); + } + } + + list_sort(graphoids, list_oid_cmp); + foreach_oid(graphoid, graphoids) + LockRelationOid(graphoid, ShareRowExclusiveLock); + list_free(graphoids); + + foreach_oid(labeloid, labeloids) + { + List *ellabeloids; + + if (!SearchSysCacheExists1(PROPGRAPHLABELOID, + ObjectIdGetDatum(labeloid))) + continue; + + ellabeloids = get_label_element_label_ids(labeloid); + if (!ellabeloids) + { + ObjectAddress object; + + ObjectAddressSet(object, PropgraphLabelRelationId, labeloid); + performDeletion(&object, behavior, flags); + } + list_free(ellabeloids); + } + + foreach_oid(propoid, propoids) + { + Relation rel; + SysScanDesc scan; + ScanKeyData key[1]; + bool in_use; + + if (!SearchSysCacheExists1(PROPGRAPHPROPOID, + ObjectIdGetDatum(propoid))) + continue; + + rel = table_open(PropgraphLabelPropertyRelationId, RowShareLock); + ScanKeyInit(&key[0], + Anum_pg_propgraph_label_property_plppropid, + BTEqualStrategyNumber, F_OIDEQ, + ObjectIdGetDatum(propoid)); + /* XXX no suitable index */ + scan = systable_beginscan(rel, InvalidOid, true, NULL, 1, key); + in_use = HeapTupleIsValid(systable_getnext(scan)); + systable_endscan(scan); + table_close(rel, RowShareLock); + + if (!in_use) + { + ObjectAddress object; + + ObjectAddressSet(object, PropgraphPropertyRelationId, propoid); + performDeletion(&object, behavior, flags); + } + } +} + /* * Process the key clause specified for an element. If key_clause is non-NIL, * then it is a list of column names. Otherwise, the primary key of the @@ -1494,21 +1590,6 @@ AlterPropGraph(ParseState *pstate, const AlterPropGraphStmt *stmt) performDeletion(&obj, stmt->drop_behavior, 0); } - /* Remove any orphaned pg_propgraph_label entries */ - if (stmt->drop_vertex_tables || stmt->drop_edge_tables) - { - foreach_oid(labeloid, get_graph_label_ids(pgrelid)) - { - if (!get_label_element_label_ids(labeloid)) - { - ObjectAddress obj; - - ObjectAddressSet(obj, PropgraphLabelRelationId, labeloid); - performDeletion(&obj, stmt->drop_behavior, 0); - } - } - } - foreach(lc, stmt->add_labels) { PropGraphLabelAndProperties *lp = lfirst_node(PropGraphLabelAndProperties, lc); @@ -1610,13 +1691,6 @@ AlterPropGraph(ParseState *pstate, const AlterPropGraphStmt *stmt) ObjectAddressSet(obj, PropgraphElementLabelRelationId, ellabeloid); performDeletion(&obj, stmt->drop_behavior, 0); - - /* Remove any orphaned pg_propgraph_label entries */ - if (!get_label_element_label_ids(labeloid)) - { - ObjectAddressSet(obj, PropgraphLabelRelationId, labeloid); - performDeletion(&obj, stmt->drop_behavior, 0); - } } if (stmt->add_properties) @@ -1714,35 +1788,6 @@ AlterPropGraph(ParseState *pstate, const AlterPropGraphStmt *stmt) check_element_label_properties(ellabeloid); } - /* Remove any orphaned pg_propgraph_property entries */ - if (stmt->drop_properties || stmt->drop_vertex_tables || stmt->drop_edge_tables || stmt->drop_label) - { - foreach_oid(propoid, get_graph_property_ids(pgrelid)) - { - Relation rel; - SysScanDesc scan; - ScanKeyData key[1]; - - rel = table_open(PropgraphLabelPropertyRelationId, RowShareLock); - ScanKeyInit(&key[0], - Anum_pg_propgraph_label_property_plppropid, - BTEqualStrategyNumber, F_OIDEQ, - ObjectIdGetDatum(propoid)); - /* XXX no suitable index */ - scan = systable_beginscan(rel, InvalidOid, true, NULL, 1, key); - if (!systable_getnext(scan)) - { - ObjectAddress obj; - - ObjectAddressSet(obj, PropgraphPropertyRelationId, propoid); - performDeletion(&obj, stmt->drop_behavior, 0); - } - - systable_endscan(scan); - table_close(rel, RowShareLock); - } - } - /* * Invalidate relcache entry of the property graph so that the queries in * the cached plans referencing the property graph will be rewritten @@ -1929,31 +1974,3 @@ get_element_label_property_names(Oid ellabeloid) return result; } - -/* - * Get a list of all property OIDs of a graph. - */ -static List * -get_graph_property_ids(Oid graphid) -{ - Relation rel; - SysScanDesc scan; - ScanKeyData key[1]; - HeapTuple tuple; - List *result = NIL; - - rel = table_open(PropgraphPropertyRelationId, AccessShareLock); - ScanKeyInit(&key[0], - Anum_pg_propgraph_property_pgppgid, - BTEqualStrategyNumber, - F_OIDEQ, ObjectIdGetDatum(graphid)); - scan = systable_beginscan(rel, PropgraphPropertyNameIndexId, true, NULL, 1, key); - while (HeapTupleIsValid(tuple = systable_getnext(scan))) - { - result = lappend_oid(result, ((Form_pg_propgraph_property) GETSTRUCT(tuple))->oid); - } - systable_endscan(scan); - table_close(rel, AccessShareLock); - - return result; -} diff --git a/src/include/commands/propgraphcmds.h b/src/include/commands/propgraphcmds.h index 1bf7d9ea217..b45f9cff143 100644 --- a/src/include/commands/propgraphcmds.h +++ b/src/include/commands/propgraphcmds.h @@ -19,5 +19,8 @@ extern ObjectAddress CreatePropGraph(ParseState *pstate, const CreatePropGraphStmt *stmt); extern ObjectAddress AlterPropGraph(ParseState *pstate, const AlterPropGraphStmt *stmt); +extern void RemoveOrphanedPropGraphObjects(const List *labeloids, + const List *propoids, + DropBehavior behavior, int flags); #endif /* PROPGRAPHCMDS_H */ diff --git a/src/test/isolation/expected/alter-propgraph-drop-cascade.out b/src/test/isolation/expected/alter-propgraph-drop-cascade.out new file mode 100644 index 00000000000..1dcad71e034 --- /dev/null +++ b/src/test/isolation/expected/alter-propgraph-drop-cascade.out @@ -0,0 +1,51 @@ +Parsed test spec with 2 sessions + +starting permutation: s1_begin s1_add_label s2_drop s1_commit s2_check +step s1_begin: BEGIN; +step s1_add_label: + ALTER PROPERTY GRAPH pgg ALTER VERTEX TABLE pgt_keep + ADD LABEL pgl PROPERTIES (a AS p); + +s2: NOTICE: drop cascades to vertex pgt_drop of property graph pgg +step s2_drop: DROP TABLE pgt_drop CASCADE; +step s1_commit: COMMIT; +step s2_drop: <... completed> +step s2_check: + SELECT + (SELECT count(*) + FROM information_schema.pg_labels + WHERE property_graph_name = 'pgg' AND label_name = 'pgl') AS labels, + (SELECT count(*) + FROM information_schema.pg_property_data_types + WHERE property_graph_name = 'pgg' AND property_name = 'p') AS properties; + +labels|properties +------+---------- + 1| 1 +(1 row) + + +starting permutation: s1_begin s1_add_label s2_drop s1_rollback s2_check +step s1_begin: BEGIN; +step s1_add_label: + ALTER PROPERTY GRAPH pgg ALTER VERTEX TABLE pgt_keep + ADD LABEL pgl PROPERTIES (a AS p); + +s2: NOTICE: drop cascades to vertex pgt_drop of property graph pgg +step s2_drop: DROP TABLE pgt_drop CASCADE; +step s1_rollback: ROLLBACK; +step s2_drop: <... completed> +step s2_check: + SELECT + (SELECT count(*) + FROM information_schema.pg_labels + WHERE property_graph_name = 'pgg' AND label_name = 'pgl') AS labels, + (SELECT count(*) + FROM information_schema.pg_property_data_types + WHERE property_graph_name = 'pgg' AND property_name = 'p') AS properties; + +labels|properties +------+---------- + 0| 0 +(1 row) + diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index 1fcf4e63238..8769a9d3332 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -130,3 +130,4 @@ test: for-portion-of test: ddl-dependency-locking test: pub-concurrent-drop test: drop-owned-grant +test: alter-propgraph-drop-cascade diff --git a/src/test/isolation/specs/alter-propgraph-drop-cascade.spec b/src/test/isolation/specs/alter-propgraph-drop-cascade.spec new file mode 100644 index 00000000000..d00f1a39320 --- /dev/null +++ b/src/test/isolation/specs/alter-propgraph-drop-cascade.spec @@ -0,0 +1,47 @@ +# Test a cascaded element-table drop concurrent with property graph changes. + +setup +{ + CREATE TABLE pgt_drop (a int PRIMARY KEY); + CREATE TABLE pgt_keep (a int PRIMARY KEY); + CREATE PROPERTY GRAPH pgg + VERTEX TABLES ( + pgt_drop LABEL pgl PROPERTIES (a AS p), + pgt_keep); +} + +teardown +{ + DROP PROPERTY GRAPH pgg; + DROP TABLE IF EXISTS pgt_drop; + DROP TABLE pgt_keep; +} + +session s1 +step s1_begin { BEGIN; } +step s1_add_label +{ + ALTER PROPERTY GRAPH pgg ALTER VERTEX TABLE pgt_keep + ADD LABEL pgl PROPERTIES (a AS p); +} +step s1_commit { COMMIT; } +step s1_rollback { ROLLBACK; } + +session s2 +step s2_drop { DROP TABLE pgt_drop CASCADE; } +step s2_check +{ + SELECT + (SELECT count(*) + FROM information_schema.pg_labels + WHERE property_graph_name = 'pgg' AND label_name = 'pgl') AS labels, + (SELECT count(*) + FROM information_schema.pg_property_data_types + WHERE property_graph_name = 'pgg' AND property_name = 'p') AS properties; +} + +# The committed association keeps the shared label and property. +permutation s1_begin s1_add_label s2_drop s1_commit s2_check + +# With the association rolled back, the drop removes the orphaned metadata. +permutation s1_begin s1_add_label s2_drop s1_rollback s2_check diff --git a/src/test/regress/expected/create_property_graph.out b/src/test/regress/expected/create_property_graph.out index 45288f99723..04696fefc6e 100644 --- a/src/test/regress/expected/create_property_graph.out +++ b/src/test/regress/expected/create_property_graph.out @@ -118,6 +118,15 @@ ERROR: property graph "g4" element "t2" label "t2" has no property "yy" -- because it remains associated with t3l1. We will verify this in the -- information schema queries outputs below. ALTER PROPERTY GRAPH g4 ALTER VERTEX TABLE t3 DROP LABEL t3l2; +-- Cascaded drop should clean up the orphaned labels and properties. We will +-- verify this in the information schema queries outputs below. +CREATE TABLE t_tmp (p_tmp int primary key); +ALTER PROPERTY GRAPH g4 ADD VERTEX TABLES (t_tmp); +ALTER PROPERTY GRAPH g3 ADD VERTEX TABLES (t_tmp); +DROP TABLE t_tmp CASCADE; +NOTICE: drop cascades to 2 other objects +DETAIL: drop cascades to vertex t_tmp of property graph g4 +drop cascades to vertex t_tmp of property graph g3 CREATE TABLE t11 (a int PRIMARY KEY); CREATE TABLE t12 (b int PRIMARY KEY); CREATE TABLE t13 ( diff --git a/src/test/regress/sql/create_property_graph.sql b/src/test/regress/sql/create_property_graph.sql index d74af881639..e407648d852 100644 --- a/src/test/regress/sql/create_property_graph.sql +++ b/src/test/regress/sql/create_property_graph.sql @@ -103,6 +103,12 @@ ALTER PROPERTY GRAPH g4 ALTER VERTEX TABLE t2 ALTER LABEL t2 DROP PROPERTIES (yy -- because it remains associated with t3l1. We will verify this in the -- information schema queries outputs below. ALTER PROPERTY GRAPH g4 ALTER VERTEX TABLE t3 DROP LABEL t3l2; +-- Cascaded drop should clean up the orphaned labels and properties. We will +-- verify this in the information schema queries outputs below. +CREATE TABLE t_tmp (p_tmp int primary key); +ALTER PROPERTY GRAPH g4 ADD VERTEX TABLES (t_tmp); +ALTER PROPERTY GRAPH g3 ADD VERTEX TABLES (t_tmp); +DROP TABLE t_tmp CASCADE; CREATE TABLE t11 (a int PRIMARY KEY); CREATE TABLE t12 (b int PRIMARY KEY); -- 2.34.1