Copilot commented on code in PR #2376:
URL: https://github.com/apache/age/pull/2376#discussion_r3085309430
##########
src/backend/utils/adt/age_global_graph.c:
##########
@@ -1277,9 +1390,41 @@ Oid get_edge_entry_label_table_oid(edge_entry *ee)
return ee->edge_label_table_oid;
}
+/*
+ * Fetch edge properties on demand from the heap via stored TID.
+ * See get_vertex_entry_properties for memory and safety notes.
+ */
Datum get_edge_entry_properties(edge_entry *ee)
{
- return ee->edge_properties;
+ Relation rel;
+ HeapTupleData tuple;
+ Buffer buffer;
+ Datum result = (Datum) 0;
+
+ rel = table_open(ee->edge_label_table_oid, AccessShareLock);
+ tuple.t_self = ee->tid;
+
+ if (heap_fetch(rel, GetActiveSnapshot(), &tuple, &buffer, true))
+ {
+ TupleDesc tupdesc = RelationGetDescr(rel);
+ bool isnull;
+ Datum props;
+
+ /* properties is column 4 (1-indexed) */
+ props = heap_getattr(&tuple, 4, tupdesc, &isnull);
+ if (!isnull)
+ result = datumCopy(props, false, -1);
+
+ ReleaseBuffer(buffer);
+ }
+
+ table_close(rel, AccessShareLock);
+
Review Comment:
`get_edge_entry_properties()` opens/closes the edge label relation per call.
Path materialization (`build_edge_list()` / `build_path()`) can call this many
times, so repeatedly doing `table_open/table_close` may become a bottleneck
even though the properties are fetched lazily. Consider caching/reusing
`Relation` objects per `edge_label_table_oid` during result construction to
reduce relcache/lock overhead.
##########
src/backend/utils/adt/age_global_graph.c:
##########
@@ -1261,9 +1328,55 @@ Oid get_vertex_entry_label_table_oid(vertex_entry *ve)
return ve->vertex_label_table_oid;
}
+/*
+ * Fetch vertex properties on demand from the heap via stored TID.
+ *
+ * Returns a datumCopy of the properties in the current memory context.
+ * The caller does not need to free the result explicitly — it will be
+ * freed when the memory context is reset (typically the SRF multi-call
+ * context for VLE, which is cleaned up when the SRF completes).
+ *
+ * If the tuple is no longer visible (e.g., concurrent mutation between
+ * cache build and fetch), the version counter should have invalidated
+ * the cache. If we get here with a stale TID, it indicates a bug in
+ * the invalidation logic.
+ */
Datum get_vertex_entry_properties(vertex_entry *ve)
{
- return ve->vertex_properties;
+ Relation rel;
+ HeapTupleData tuple;
+ Buffer buffer;
+ Datum result = (Datum) 0;
+
+ rel = table_open(ve->vertex_label_table_oid, AccessShareLock);
+ tuple.t_self = ve->tid;
+
+ if (heap_fetch(rel, GetActiveSnapshot(), &tuple, &buffer, true))
+ {
+ TupleDesc tupdesc = RelationGetDescr(rel);
+ bool isnull;
+ Datum props;
+
+ /* properties is column 2 (1-indexed) */
+ props = heap_getattr(&tuple, 2, tupdesc, &isnull);
+ if (!isnull)
+ result = datumCopy(props, false, -1);
+
+ ReleaseBuffer(buffer);
+ }
+
+ table_close(rel, AccessShareLock);
+
Review Comment:
`get_vertex_entry_properties()` opens and closes the label table relation on
every call. Since `build_path()` calls this once per vertex in every returned
path, this can add noticeable overhead (repeated relcache lookups/lock
bookkeeping) on result-heavy queries. Consider reusing an already-open
`Relation` (e.g., cache per `vertex_label_table_oid` while materializing a
path/row) so property fetches don’t repeatedly call `table_open/table_close`.
##########
src/backend/utils/adt/age_vle.c:
##########
@@ -364,54 +364,70 @@ static bool is_an_edge_match(VLE_local_context *vlelctx,
edge_entry *ee)
return false;
}
- /* get our edge's properties */
- edge_property = DATUM_GET_AGTYPE_P(get_edge_entry_properties(ee));
- /* get the containers */
- agtc_edge_property_constraint = &vlelctx->edge_property_constraint->root;
- agtc_edge_property = &edge_property->root;
- /* get the number of properties in the edge to be matched */
- num_edge_properties = AGTYPE_CONTAINER_SIZE(agtc_edge_property);
-
/*
- * Check to see if the edge_properties object has AT LEAST as many pairs
- * to compare as the edge_property_constraint object has pairs. If not, it
- * can't possibly match.
+ * Fast path: if the label matched (or wasn't constrained) and there
+ * are no property constraints, the edge is a match. This avoids
+ * accessing edge properties entirely for label-only VLE patterns
+ * like [:KNOWS*1..3] which are the common case.
*/
- if (num_edge_property_constraints > num_edge_properties)
+ if (num_edge_property_constraints == 0)
{
- return false;
+ return true;
}
/*
- * If the number of constraints are the same as the number of properties,
- * then the datums would be the same if they match.
+ * Fetch edge properties once and cache locally. With thin entries,
+ * get_edge_entry_properties() does a heap_fetch, so we avoid calling
+ * it multiple times for the same edge.
*/
- if (num_edge_property_constraints == num_edge_properties)
{
- Datum edge_props = get_edge_entry_properties(ee);
- uint32 edge_props_hash = datum_image_hash(edge_props, false, -1);
+ Datum edge_props_datum = get_edge_entry_properties(ee);
+
+ edge_property = DATUM_GET_AGTYPE_P(edge_props_datum);
+ agtc_edge_property_constraint =
&vlelctx->edge_property_constraint->root;
+ agtc_edge_property = &edge_property->root;
+ num_edge_properties = AGTYPE_CONTAINER_SIZE(agtc_edge_property);
+
+ /*
+ * Check to see if the edge_properties object has AT LEAST as many
+ * pairs to compare as the edge_property_constraint object has pairs.
+ * If not, it can't possibly match.
+ */
+ if (num_edge_property_constraints > num_edge_properties)
+ {
+ return false;
+ }
- /* check the hash first */
- if (vlelctx->edge_property_constraint_hash == edge_props_hash)
+ /*
+ * If the number of constraints are the same as the number of
+ * properties, then the datums would be the same if they match.
+ */
+ if (num_edge_property_constraints == num_edge_properties)
{
- /* if the hashes match, check the datum images */
- if (datum_image_eq(vlelctx->edge_property_constraint_datum,
- edge_props, false, -1))
+ uint32 edge_props_hash = datum_image_hash(edge_props_datum,
+ false, -1);
+ /* check the hash first */
+ if (vlelctx->edge_property_constraint_hash == edge_props_hash)
{
- return true;
+ /* if the hashes match, check the datum images */
+ if (datum_image_eq(vlelctx->edge_property_constraint_datum,
+ edge_props_datum, false, -1))
+ {
+ return true;
+ }
}
- }
- /* if we got here they aren't the same */
- return false;
- }
+ /* if we got here they aren't the same */
+ return false;
+ }
- /* get the iterators */
- constraint_it = agtype_iterator_init(agtc_edge_property_constraint);
- property_it = agtype_iterator_init(agtc_edge_property);
+ /* get the iterators */
+ constraint_it = agtype_iterator_init(agtc_edge_property_constraint);
+ property_it = agtype_iterator_init(agtc_edge_property);
- /* return the value of deep contains */
- return agtype_deep_contains(&property_it, &constraint_it, false);
+ /* return the value of deep contains */
+ return agtype_deep_contains(&property_it, &constraint_it, false);
Review Comment:
`is_an_edge_match()` now fetches edge properties via
`get_edge_entry_properties()`, which performs `datumCopy()` into the current
memory context. This function runs inside `funcctx->multi_call_memory_ctx`
during DFS (see `age_vle()`), so property-constrained traversals can accumulate
a large amount of copied property data across many visited edges, potentially
causing significant memory bloat/OOM for large graphs. Consider avoiding
`datumCopy()` for the match path (e.g., keep the buffer pinned until comparison
completes, or copy into a short-lived temp context that is reset per edge
match) so memory usage doesn’t grow with the number of edges examined.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
