Copilot commented on code in PR #2303:
URL: https://github.com/apache/age/pull/2303#discussion_r2684277454
##########
src/backend/parser/cypher_expr.c:
##########
@@ -2001,6 +2068,145 @@ static bool function_exists(char *funcname, char
*extension)
return found;
}
+/*
+ * Check if function name is an accessor function that can be optimized
+ */
+static bool is_accessor_function(const char *func_name)
+{
+ return (strcasecmp("id", func_name) == 0 ||
+ strcasecmp("properties", func_name) == 0 ||
+ strcasecmp("type", func_name) == 0 ||
+ strcasecmp("label", func_name) == 0 ||
+ strcasecmp("start_id", func_name) == 0 ||
+ strcasecmp("end_id", func_name) == 0 ||
+ strcasecmp("startnode", func_name) == 0 ||
+ strcasecmp("endnode", func_name) == 0);
+}
+
+static Oid get_entity_record_type(Node *node)
+{
+ Oid typeid = InvalidOid;
+
+ if (IsA(node, RowExpr))
+ {
+ typeid = ((RowExpr *)node)->row_typeid;
+ }
+ else if (IsA(node, Var))
+ {
+ typeid = ((Var *)node)->vartype;
+ }
+ else
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("argument must be a vertex or edge record")));
+ }
+
+ if (typeid != VERTEXOID && typeid != EDGEOID)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("argument must be a vertex or edge record")));
+ }
+
+ return typeid;
+}
+
+/*
+ * Optimize accessor functions when called on RECORD entities
+ * Extracts fields directly using FieldSelect instead of building and parsing
agtype
+ */
+static Node *optimize_accessor_function(cypher_parsestate *cpstate,
+ const char *func_name,
+ FuncCall *fn,
+ List *targs)
+{
+ ParseState *pstate = &cpstate->pstate;
+ Node *arg;
+ AttrNumber fieldnum;
+ Oid fieldtype;
+ Node *result;
+ Oid entity_type;
+ bool is_node_func = (strcasecmp(func_name, "startNode") == 0 ||
+ strcasecmp(func_name, "endNode") == 0);
+
+ if (is_node_func)
+ {
+ if (list_length(targs) < 2)
+ return NULL;
+ arg = (Node *)lsecond(targs);
+ }
+ else
+ {
+ if (list_length(targs) < 1)
+ return NULL;
+ arg = (Node *)linitial(targs);
+ }
+
+ if (!is_vertex_or_edge(arg))
+ {
+ return NULL;
+ }
+
+ entity_type = get_entity_record_type(arg);
+ get_record_field_info((char *)func_name, entity_type, &fieldnum,
&fieldtype);
+
+ if (fieldnum != InvalidAttrNumber && fieldtype != InvalidOid)
+ {
+ Node *field = NULL;
+
+ /* Validate RowExpr has enough fields */
+ if (IsA(arg, RowExpr))
+ {
+ RowExpr *re = (RowExpr *)arg;
+ if (list_length(re->args) < fieldnum)
+ {
+ return NULL;
+ }
+
+ /* RowExpr args are 0-indexed, but fieldnum is 1-indexed */
+ field = (Node *)list_nth(re->args, fieldnum - 1);
+ }
+ else if (IsA(arg, Var))
+ {
+ field = (Node *)make_field_select((Var *)arg, fieldnum, fieldtype);
+ }
+
+ /* For startNode/endNode functions on edges, look up the vertex */
+ if (is_node_func && entity_type == EDGEOID)
+ {
+ Oid func_oid;
+ Node *graph_name;
+
+ graph_name = (Node *) makeConst(TEXTOID, -1, InvalidOid, -1,
+
CStringGetTextDatum(cpstate->graph_name),
+ false, false);
+
+ /* Call _get_vertex_by_graphid(graph_name, vertex_id) */
+ func_oid = get_ag_func_oid("_get_vertex_by_graphid", 2, TEXTOID,
GRAPHIDOID);
+ result = (Node *)makeFuncExpr(func_oid, AGTYPEOID,
+ list_make2(graph_name, field),
+ InvalidOid, InvalidOid,
+ COERCE_EXPLICIT_CALL);
+ }
+ else
+ {
+ result = field;
+ }
Review Comment:
The condition at line 2176 checks for is_node_func AND EDGEOID, but
startNode/endNode can only be called on edges, never on vertices. When
entity_type is VERTEXOID and is_node_func is true, neither the if block (line
2176) nor the else block (line 2192) is entered, so result remains
uninitialized. The logic should handle VERTEXOID with is_node_func as an error
case.
##########
src/backend/parser/cypher_expr.c:
##########
@@ -2001,6 +2068,145 @@ static bool function_exists(char *funcname, char
*extension)
return found;
}
+/*
+ * Check if function name is an accessor function that can be optimized
+ */
+static bool is_accessor_function(const char *func_name)
+{
+ return (strcasecmp("id", func_name) == 0 ||
+ strcasecmp("properties", func_name) == 0 ||
+ strcasecmp("type", func_name) == 0 ||
+ strcasecmp("label", func_name) == 0 ||
+ strcasecmp("start_id", func_name) == 0 ||
+ strcasecmp("end_id", func_name) == 0 ||
+ strcasecmp("startnode", func_name) == 0 ||
+ strcasecmp("endnode", func_name) == 0);
+}
+
+static Oid get_entity_record_type(Node *node)
+{
+ Oid typeid = InvalidOid;
+
+ if (IsA(node, RowExpr))
+ {
+ typeid = ((RowExpr *)node)->row_typeid;
+ }
+ else if (IsA(node, Var))
+ {
+ typeid = ((Var *)node)->vartype;
+ }
+ else
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("argument must be a vertex or edge record")));
+ }
+
+ if (typeid != VERTEXOID && typeid != EDGEOID)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("argument must be a vertex or edge record")));
+ }
+
+ return typeid;
+}
+
+/*
+ * Optimize accessor functions when called on RECORD entities
+ * Extracts fields directly using FieldSelect instead of building and parsing
agtype
+ */
+static Node *optimize_accessor_function(cypher_parsestate *cpstate,
+ const char *func_name,
+ FuncCall *fn,
+ List *targs)
+{
+ ParseState *pstate = &cpstate->pstate;
+ Node *arg;
+ AttrNumber fieldnum;
+ Oid fieldtype;
+ Node *result;
+ Oid entity_type;
+ bool is_node_func = (strcasecmp(func_name, "startNode") == 0 ||
+ strcasecmp(func_name, "endNode") == 0);
+
+ if (is_node_func)
+ {
+ if (list_length(targs) < 2)
+ return NULL;
+ arg = (Node *)lsecond(targs);
+ }
+ else
+ {
+ if (list_length(targs) < 1)
+ return NULL;
+ arg = (Node *)linitial(targs);
+ }
+
+ if (!is_vertex_or_edge(arg))
+ {
+ return NULL;
+ }
+
+ entity_type = get_entity_record_type(arg);
+ get_record_field_info((char *)func_name, entity_type, &fieldnum,
&fieldtype);
+
+ if (fieldnum != InvalidAttrNumber && fieldtype != InvalidOid)
+ {
+ Node *field = NULL;
+
+ /* Validate RowExpr has enough fields */
+ if (IsA(arg, RowExpr))
+ {
+ RowExpr *re = (RowExpr *)arg;
+ if (list_length(re->args) < fieldnum)
+ {
+ return NULL;
+ }
+
+ /* RowExpr args are 0-indexed, but fieldnum is 1-indexed */
+ field = (Node *)list_nth(re->args, fieldnum - 1);
+ }
+ else if (IsA(arg, Var))
+ {
+ field = (Node *)make_field_select((Var *)arg, fieldnum, fieldtype);
+ }
+
+ /* For startNode/endNode functions on edges, look up the vertex */
+ if (is_node_func && entity_type == EDGEOID)
+ {
+ Oid func_oid;
+ Node *graph_name;
+
+ graph_name = (Node *) makeConst(TEXTOID, -1, InvalidOid, -1,
+
CStringGetTextDatum(cpstate->graph_name),
+ false, false);
+
+ /* Call _get_vertex_by_graphid(graph_name, vertex_id) */
+ func_oid = get_ag_func_oid("_get_vertex_by_graphid", 2, TEXTOID,
GRAPHIDOID);
+ result = (Node *)makeFuncExpr(func_oid, AGTYPEOID,
+ list_make2(graph_name, field),
+ InvalidOid, InvalidOid,
+ COERCE_EXPLICIT_CALL);
+ }
+ else
+ {
+ result = field;
+ }
+ }
+ else
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("%s() argument must be an %s or null",
+ func_name,
+ (entity_type == VERTEXOID) ? "edge" : "vertex"),
+ parser_errposition(pstate, fn->location)));
+ }
+
+ return result;
Review Comment:
Missing return statement after the error. If the condition at line 2154 is
false (fieldnum is InvalidAttrNumber or fieldtype is InvalidOid), the function
reports an error at line 2199 but then continues to line 2207 to return result,
which is uninitialized. Add a return after ereport or initialize result to NULL.
##########
sql/age_scalar.sql:
##########
@@ -93,6 +93,15 @@ RETURNS NULL ON NULL INPUT
PARALLEL SAFE
AS 'MODULE_PATHNAME';
+-- Helper function for optimized startNode/endNode
+CREATE FUNCTION ag_catalog._get_vertex_by_graphid(text, graphid)
+ RETURNS agtype
+ LANGUAGE c
+ STABLE
+RETURNS NULL ON NULL INPUT
+PARALLEL SAFE
+AS 'MODULE_PATHNAME';
Review Comment:
The new function `_get_vertex_by_graphid(text, graphid)` exposes a low-level
vertex lookup to any caller without performing any privilege or RLS checks on
the underlying label tables. Its C implementation uses `get_vertex`, which
directly calls `table_open` / `table_beginscan` on the graph’s label tables by
name, bypassing PostgreSQL `GRANT` and row-level security, so any role with
`EXECUTE` on `_get_vertex_by_graphid` can fetch vertex data from graphs whose
schemas/tables they may not otherwise be allowed to `SELECT`. To prevent
unauthorized graph data access, restrict `EXECUTE` on `_get_vertex_by_graphid`
(and similar helpers) to trusted roles only, or add explicit ACL/RLS checks
before opening the target relation (e.g., verifying the caller has appropriate
privileges on the resolved label table).
```suggestion
AS 'MODULE_PATHNAME';
REVOKE ALL ON FUNCTION ag_catalog._get_vertex_by_graphid(text, graphid) FROM
PUBLIC;
```
--
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]
