Copilot commented on code in PR #2442:
URL: https://github.com/apache/age/pull/2442#discussion_r3454589736
##########
src/backend/utils/adt/age_vle.c:
##########
@@ -3236,8 +3278,191 @@ static void sp_enumerate(HTAB *visited, graphid source,
graphid cur,
alt[pos - 1] = p->edge;
sp_enumerate(visited, source, p->parent_vertex, alt, alt_len, pos - 2,
- out);
+ fname, out);
+ }
+}
+
+/*
+ * Maximum number of distinct paths the minimum-hop fallback will enumerate
+ * before giving up. The exhaustive DFS used for a minimum hop count greater
+ * than the shortest distance can explode on dense graphs, so this acts as a
+ * safety valve alongside CHECK_FOR_INTERRUPTS()/statement_timeout in the DFS.
+ */
+#define SP_MINHOPS_MAX_PATHS 1000000
+
+/*
+ * Fallback for the "minimum hop count greater than the shortest distance"
+ * regime, which plain BFS cannot satisfy (it requires longer,
vertex-revisiting
+ * paths under relationship-uniqueness). This reuses the VLE depth-first engine
+ * directly: it builds a VLE_local_context by hand (no fcinfo), enumerates
every
+ * path whose length is within [min_hops, max_hops], and keeps only those of
the
+ * smallest qualifying length. For shortest_path one such path is returned; for
+ * all_shortest_paths every tie at that length is returned. Returns NULL with
+ * *out_count == 0 when no qualifying path exists.
+ *
+ * The VLE engine matches a single edge label oid only, so a multi-type filter
+ * is rejected by the caller before reaching here. A single label_oid of
+ * InvalidOid means "any edge label".
+ */
+static Datum *sp_minhops_fallback(GRAPH_global_context *ggctx, Oid graph_oid,
+ const char *graph_name, char *fname,
+ graphid source, graphid target, Oid
label_oid,
+ cypher_rel_dir dir, int64 min_hops,
+ int64 max_hops, bool collect_all,
+ int64 *out_count)
+{
+ MemoryContext oldctx = CurrentMemoryContext;
+ MemoryContext tmpctx = NULL;
+ VLE_local_context *vlelctx = NULL;
+ agtype_value av_empty;
+ agtype *empty_constraint = NULL;
+ List *best = NIL;
+ ListCell *lc = NULL;
+ int64 best_len = PG_INT64_MAX;
+ int64 examined = 0;
+ int64 result_len = 0;
+ int64 n = 0;
+ int64 idx = 0;
+ Datum *paths = NULL;
+
+ *out_count = 0;
+
+ /* do the VLE enumeration in a private context we can throw away at the
end */
+ tmpctx = AllocSetContextCreate(oldctx, "age_shortest_path minhops",
+ ALLOCSET_DEFAULT_SIZES);
Review Comment:
This scratch MemoryContext is named "age_shortest_path minhops", but the
code is shared by age_shortest_path and age_all_shortest_paths. Using a neutral
name avoids confusing memory-context debugging output when the caller is
age_all_shortest_paths.
##########
src/backend/utils/adt/age_vle.c:
##########
@@ -3123,18 +3124,33 @@ static HTAB *sp_run_bfs(GRAPH_global_context *ggctx,
graphid source,
/*
* Optional edge label filter. When a label filter is active
- * we keep only edges whose label table oid matches. Note that
- * a label name which does not exist in this graph resolves to
+ * (n_label_oids > 0) we keep only edges whose label table oid
+ * is one of the requested relationship types. Note that a
+ * label name which does not exist in this graph resolves to
* InvalidOid; because no real edge can have an InvalidOid
- * label table, every edge is then skipped and only the
+ * label table, such a type matches no edges and only the
* zero-length (start == end) path can match -- matching the
* openCypher semantics that an unknown relationship type
Review Comment:
The comment about unknown relationship types is a bit misleading in the
multi-type case: only an *all-unknown* filter (all requested oids are
InvalidOid) causes every edge to be skipped. If the requested set includes at
least one valid oid, edges of that type can still match even when some
requested types are unknown.
##########
src/backend/utils/adt/age_vle.c:
##########
@@ -3306,74 +3538,76 @@ static Datum *sp_compute_paths(agtype *graph_name_agt,
agtype *start_agt,
if (AGT_ROOT_IS_ARRAY(label_agt) && !AGT_ROOT_IS_SCALAR(label_agt))
{
int nelems = AGT_ROOT_COUNT(label_agt);
+ int i = 0;
- if (nelems > 1)
+ if (nelems > 0)
{
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("age_shortest_path: multiple relationship
types are not yet supported")));
+ label_oids = palloc(sizeof(Oid) * nelems);
}
- if (nelems == 1)
+ for (i = 0; i < nelems; i++)
{
agtv_temp = get_ith_agtype_value_from_container(
- &label_agt->root, 0);
+ &label_agt->root, i);
if (agtv_temp->type != AGTV_STRING)
{
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("age_shortest_path: relationship type must
be a string")));
+ errmsg("%s: relationship type must be a string",
+ fname)));
}
+ /* skip empty type names; they impose no constraint */
if (agtv_temp->val.string.len != 0)
{
label_name = pnstrdup(agtv_temp->val.string.val,
agtv_temp->val.string.len);
- edge_label_oid = get_label_relation(label_name, graph_oid);
- filter_edges = true;
+ label_oids[n_label_oids] =
+ get_label_relation(label_name, graph_oid);
+ n_label_oids = n_label_oids + 1;
}
}
}
else
{
- agtv_temp = get_agtype_value("age_shortest_path", label_agt,
+ agtv_temp = get_agtype_value(fname, label_agt,
AGTV_STRING, true);
if (agtv_temp->val.string.len != 0)
{
label_name = pnstrdup(agtv_temp->val.string.val,
agtv_temp->val.string.len);
- edge_label_oid = get_label_relation(label_name, graph_oid);
- filter_edges = true;
+ label_oids = palloc(sizeof(Oid));
+ label_oids[0] = get_label_relation(label_name, graph_oid);
+ n_label_oids = 1;
Review Comment:
label_name is pnstrdup'd here but not freed; since this runs in the SRF's
long-lived context, freeing it after get_label_relation avoids retaining
unnecessary memory for the duration of the SRF.
##########
src/backend/utils/adt/age_vle.c:
##########
@@ -3306,74 +3538,76 @@ static Datum *sp_compute_paths(agtype *graph_name_agt,
agtype *start_agt,
if (AGT_ROOT_IS_ARRAY(label_agt) && !AGT_ROOT_IS_SCALAR(label_agt))
{
int nelems = AGT_ROOT_COUNT(label_agt);
+ int i = 0;
- if (nelems > 1)
+ if (nelems > 0)
{
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("age_shortest_path: multiple relationship
types are not yet supported")));
+ label_oids = palloc(sizeof(Oid) * nelems);
}
- if (nelems == 1)
+ for (i = 0; i < nelems; i++)
{
agtv_temp = get_ith_agtype_value_from_container(
- &label_agt->root, 0);
+ &label_agt->root, i);
if (agtv_temp->type != AGTV_STRING)
{
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("age_shortest_path: relationship type must
be a string")));
+ errmsg("%s: relationship type must be a string",
+ fname)));
}
+ /* skip empty type names; they impose no constraint */
if (agtv_temp->val.string.len != 0)
{
label_name = pnstrdup(agtv_temp->val.string.val,
agtv_temp->val.string.len);
- edge_label_oid = get_label_relation(label_name, graph_oid);
- filter_edges = true;
+ label_oids[n_label_oids] =
+ get_label_relation(label_name, graph_oid);
+ n_label_oids = n_label_oids + 1;
Review Comment:
label_name is pnstrdup'd for each relationship type but never freed. In SRF
usage this memory lives for the whole SRF (multi_call_memory_ctx), so a large
type array can retain unnecessary memory. Free label_name after resolving it to
an oid (both in the array and scalar cases).
##########
src/backend/utils/adt/age_vle.c:
##########
@@ -2920,7 +2921,7 @@ static cypher_rel_dir sp_agtype_to_direction(agtype *agt)
return CYPHER_REL_DIR_NONE;
}
- agtv = get_agtype_value("age_shortest_path", agt, AGTV_STRING, true);
+ agtv = get_agtype_value(fname, agt, AGTV_STRING, true);
s = pnstrdup(agtv->val.string.val, agtv->val.string.len);
Review Comment:
Direction-argument errors currently omit the function name prefix even
though other argument-resolution errors in this SRF now use "%s:". Including
fname here makes error reporting consistent and ensures age_all_shortest_paths
reports its own name when direction is invalid.
--
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]
