Copilot commented on code in PR #2442:
URL: https://github.com/apache/age/pull/2442#discussion_r3455882031
##########
src/backend/utils/adt/age_vle.c:
##########
@@ -3291,13 +3522,17 @@ static Datum *sp_compute_paths(agtype *graph_name_agt,
agtype *start_agt,
return NULL;
}
Review Comment:
sp_compute_paths() allocates graph_name before checking for NULL start/end
endpoints, but returns early without freeing graph_name. In an SRF, that
allocation lands in the SRF-lifetime context and accumulates across calls
within the query.
##########
src/backend/utils/adt/age_vle.c:
##########
@@ -3389,16 +3634,67 @@ static Datum *sp_compute_paths(agtype *graph_name_agt,
agtype *start_agt,
return NULL;
}
+ /*
+ * Run the search and reconstruct the result path(s) in a private scratch
+ * context. The BFS bookkeeping (visited table, frontier queue, predecessor
+ * multiset) and the intermediate path arrays are only needed while we
+ * compute; the surviving result Datums are built in the caller's
+ * (SRF-lifetime) context and copied out before the scratch context is
+ * deleted. This bounds peak memory to the result set plus one search,
+ * rather than retaining the whole search state for the life of the SRF.
+ */
+ scratch = AllocSetContextCreate(oldctx, "age shortest path scratch",
+ ALLOCSET_DEFAULT_SIZES);
+ MemoryContextSwitchTo(scratch);
+
/* run the breadth-first search */
- visited = sp_run_bfs(ggctx, source, target, filter_edges, edge_label_oid,
+ visited = sp_run_bfs(ggctx, source, target, label_oids, n_label_oids,
dir, max_hops, collect_all, &target_depth, &found);
if (!found)
{
- hash_destroy(visited);
+ MemoryContextSwitchTo(oldctx);
+ MemoryContextDelete(scratch);
return NULL;
}
Review Comment:
On the !found path, scratch is deleted but graph_name/label_oids are left
allocated in the caller’s (SRF-lifetime) context. Freeing them here keeps
per-call memory bounded as intended by the scratch context change.
##########
src/backend/utils/adt/age_vle.c:
##########
@@ -3389,16 +3634,67 @@ static Datum *sp_compute_paths(agtype *graph_name_agt,
agtype *start_agt,
return NULL;
}
Review Comment:
If manage_GRAPH_global_contexts() returns NULL, sp_compute_paths() returns
without freeing graph_name/label_oids. These are allocated in the SRF-lifetime
context, so it’s better to release them on this early-exit path.
##########
src/backend/utils/adt/age_vle.c:
##########
@@ -3450,7 +3751,9 @@ static Datum *sp_compute_paths(agtype *graph_name_agt,
agtype *start_agt,
*out_count = n;
}
- hash_destroy(visited);
+ /* results are copied out; drop the BFS/enumeration scratch */
+ MemoryContextSwitchTo(oldctx);
+ MemoryContextDelete(scratch);
return paths;
Review Comment:
At the normal return from sp_compute_paths(), graph_name/label_oids are no
longer needed (the global graph context duplicates graph_name internally, and
label_oids are only used during search). Freeing them avoids retaining
user-controlled allocations for the remainder of the SRF.
##########
src/backend/utils/adt/age_vle.c:
##########
@@ -3389,16 +3634,67 @@ static Datum *sp_compute_paths(agtype *graph_name_agt,
agtype *start_agt,
return NULL;
}
+ /*
+ * Run the search and reconstruct the result path(s) in a private scratch
+ * context. The BFS bookkeeping (visited table, frontier queue, predecessor
+ * multiset) and the intermediate path arrays are only needed while we
+ * compute; the surviving result Datums are built in the caller's
+ * (SRF-lifetime) context and copied out before the scratch context is
+ * deleted. This bounds peak memory to the result set plus one search,
+ * rather than retaining the whole search state for the life of the SRF.
+ */
+ scratch = AllocSetContextCreate(oldctx, "age shortest path scratch",
+ ALLOCSET_DEFAULT_SIZES);
+ MemoryContextSwitchTo(scratch);
+
/* run the breadth-first search */
- visited = sp_run_bfs(ggctx, source, target, filter_edges, edge_label_oid,
+ visited = sp_run_bfs(ggctx, source, target, label_oids, n_label_oids,
dir, max_hops, collect_all, &target_depth, &found);
if (!found)
{
- hash_destroy(visited);
+ MemoryContextSwitchTo(oldctx);
+ MemoryContextDelete(scratch);
return NULL;
}
+ /*
+ * A minimum hop count greater than the true shortest distance can only be
+ * satisfied by longer, vertex-revisiting paths (Neo4j's exhaustive search
+ * regime). Plain BFS cannot produce those, so fall back to the VLE
+ * depth-first engine for that case. When min_hops <= target_depth the
+ * bound imposes no additional constraint and the shortest path(s) already
+ * found are returned unchanged.
+ *
+ * The VLE engine matches a single edge label only, so a multi-type filter
+ * combined with this regime is still unsupported.
+ */
+ if (min_hops > 0 && target_depth < min_hops)
+ {
+ Oid fallback_label_oid = InvalidOid;
+
+ /* the BFS scratch is no longer needed; the fallback uses its own */
+ MemoryContextSwitchTo(oldctx);
+ MemoryContextDelete(scratch);
+
+ if (n_label_oids > 1)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("%s: a minimum hop count greater than the shortest
path length is not supported with multiple relationship types",
+ fname)));
+ }
+
+ if (n_label_oids == 1)
+ {
+ fallback_label_oid = label_oids[0];
+ }
+
+ return sp_minhops_fallback(ggctx, graph_oid, graph_name, fname, source,
+ target, fallback_label_oid, dir, min_hops,
+ max_hops, collect_all, out_count);
Review Comment:
In the min_hops fallback branch, sp_compute_paths() returns directly after
sp_minhops_fallback(), leaving graph_name/label_oids allocated in the
SRF-lifetime context. Capture the result, free these temporaries, then return.
--
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]
