Copilot commented on code in PR #2592:
URL: https://github.com/apache/age/pull/2592#discussion_r4250870049
##########
src/backend/utils/adt/age_vle.c:
##########
@@ -639,6 +639,14 @@ static VLE_local_context
*build_local_vle_context(FunctionCallInfo fcinfo,
Oid graph_oid = InvalidOid;
int64 vle_grammar_node_id = 0;
bool use_cache = false;
+ ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
+ MemoryContext query_owner = NULL;
+
+ /* the graph context stays pinned for the rest of this query */
+ if (rsinfo != NULL && IsA(rsinfo, ReturnSetInfo) && rsinfo->econtext !=
NULL)
+ {
+ query_owner = rsinfo->econtext->ecxt_per_query_memory;
Review Comment:
The code casts `fcinfo->resultinfo` to `ReturnSetInfo *` before establishing
that it is actually a `ReturnSetInfo`. If `resultinfo` is another kind of
node/struct, `IsA(rsinfo, ReturnSetInfo)` may read the wrong tag field and can
crash. Prefer checking `fcinfo->resultinfo != NULL && IsA(fcinfo->resultinfo,
ReturnSetInfo)` first, then casting to `ReturnSetInfo *`.
##########
src/backend/utils/adt/age_global_graph.c:
##########
@@ -1980,6 +2365,233 @@ GRAPH_global_context *find_GRAPH_global_context(Oid
graph_oid)
return NULL;
}
+/*
+ * Find a graph context for materializing a VLE path. The path may come from a
+ * traversal whose context has since been retired, so retired contexts that a
+ * query still pins are considered too. The first context that holds every
+ * vertex and edge of the path is returned, or NULL if there is none.
+ */
+GRAPH_global_context *find_GRAPH_global_context_for_path(Oid graph_oid,
+ const graphid *ids,
+ int64 count)
+{
+ GRAPH_global_context *ggctx = NULL;
+
+ for (ggctx = global_graph_contexts; ggctx != NULL; ggctx = ggctx->next)
+ {
+ bool complete = true;
+ int64 i;
+
+ if (ggctx->graph_oid != graph_oid)
+ {
+ continue;
+ }
+
+ for (i = 0; i < count; i++)
+ {
+ /*
+ * get_edge_entry() asserts that the edge exists; a context may
+ * lack it here, so look the edge up directly.
+ */
+ if ((i % 2 == 0 && get_vertex_entry(ggctx, ids[i]) == NULL) ||
+ (i % 2 == 1 &&
+ agehash_lookup(ggctx->edge_table, &ids[i]) == NULL))
+ {
+ complete = false;
+ break;
+ }
+ }
Review Comment:
`find_GRAPH_global_context_for_path()` assumes the `ids` array alternates
vertex/edge IDs purely based on index parity (`i % 2`). That implicit contract
is not enforced here, and a future change to the container layout would
silently break path materialization. Consider making the invariant explicit
(e.g., documenting it in the container definition and adding an
assertion/validation), or passing/type-tagging the IDs so this function doesn’t
depend on positional parity.
##########
src/backend/utils/adt/age_global_graph.c:
##########
@@ -786,18 +863,196 @@ static bool insert_vertex_edge(GRAPH_global_context
*ggctx,
return false;
}
+/* checker for check_functions_in_node: a function that is not IMMUTABLE */
+static bool rls_function_not_immutable(Oid func_id, void *context)
+{
+ return (func_volatile(func_id) != PROVOLATILE_IMMUTABLE);
+}
+
+/*
+ * PostgreSQL's contain_mutable_functions_walker, except for the SQL value
+ * functions whose result cannot change during a statement without the reuse
+ * check noticing: the date and time ones are fixed for the transaction,
+ * CURRENT_ROLE, CURRENT_USER and USER are the current role, which the reuse
+ * check compares, and CURRENT_CATALOG is fixed for the session. SESSION_USER
+ * and CURRENT_SCHEMA can change within a statement.
+ */
+static bool rls_quals_vary_walker(Node *node, void *context)
+{
+ if (node == NULL)
+ {
+ return false;
+ }
+
+ if (check_functions_in_node(node, rls_function_not_immutable, context))
+ {
+ return true;
+ }
+
+ if (IsA(node, JsonConstructorExpr))
+ {
+ JsonConstructorExpr *ctor = (JsonConstructorExpr *) node;
+ bool is_jsonb = (ctor->returning->format->format_type ==
Review Comment:
`ctor->returning` and/or `ctor->returning->format` may be NULL depending on
how the parser constructs `JsonConstructorExpr` when no explicit RETURNING
clause is present. Dereferencing through `ctor->returning->format` without a
NULL check risks a backend crash. Add a defensive check and pick the correct
default behavior (or conservatively treat it as “varying”) when
RETURNING/format is absent.
--
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]
