Copilot commented on code in PR #2358:
URL: https://github.com/apache/age/pull/2358#discussion_r2982792835
##########
src/backend/utils/adt/agtype.c:
##########
@@ -5604,15 +5623,24 @@ static Datum get_vertex(const char *graph, const char
*vertex_label,
HeapTuple tuple;
TupleDesc tupdesc;
Datum id, properties, result;
+ AclResult aclresult;
/* get the specific graph namespace (schema) */
Oid graph_namespace_oid = get_namespace_oid(graph, false);
/* get the specific vertex label table (schema.vertex_label) */
Oid vertex_label_table_oid = get_relname_relid(vertex_label,
- graph_namespace_oid);
+ graph_namespace_oid);
/* get the active snapshot */
Snapshot snapshot = GetActiveSnapshot();
+ /* check for SELECT permission on the table */
+ aclresult = pg_class_aclcheck(vertex_label_table_oid, GetUserId(),
+ ACL_SELECT);
+ if (aclresult != ACLCHECK_OK)
+ {
+ aclcheck_error(aclresult, OBJECT_TABLE, vertex_label);
+ }
Review Comment:
`get_relname_relid()` can return `InvalidOid` when the label table doesn’t
exist. This code calls `pg_class_aclcheck()` (and later `table_open()`) with
that OID, which will raise a confusing internal error (e.g., cache lookup
failed) instead of a clean undefined-table/label error. Add an
`OidIsValid(vertex_label_table_oid)` check before the ACL check, and raise the
same user-facing error you expect for a missing label table.
##########
src/backend/executor/cypher_delete.c:
##########
@@ -533,6 +602,34 @@ static void check_for_connected_edges(CustomScanState
*node)
{
if (css->delete_data->detach)
{
+ AclResult aclresult;
+
+ /* Check that the user has DELETE permission on the edge
table */
+ aclresult = pg_class_aclcheck(relid, GetUserId(),
ACL_DELETE);
+ if (aclresult != ACLCHECK_OK)
+ {
+ aclcheck_error(aclresult, OBJECT_TABLE, label_name);
+ }
Review Comment:
In `DETACH DELETE`, the ACL check (`pg_class_aclcheck`) is executed inside
the per-tuple scan loop. Since `relid`/`label_name` are constant for the whole
scan, this should be done once per label (before `while (true)`) to avoid
repeated catalog lookups on large edge tables.
##########
src/include/utils/agtype.h:
##########
@@ -322,6 +322,109 @@ enum agtype_value_type
AGTV_BINARY
};
+/*
+ * Direct field access indices for vertex and edge objects.
+ *
+ * Vertex and edge objects are serialized with keys sorted by length first,
+ * then lexicographically (via uniqueify_agtype_object). This means field
+ * positions are deterministic and can be accessed directly without binary
+ * search, providing O(1) access instead of O(log n).
+ *
+ * Vertex keys by length: "id"(2), "label"(5), "properties"(10)
+ * Edge keys by length: "id"(2), "label"(5), "end_id"(6), "start_id"(8),
"properties"(10)
+ */
+#define VERTEX_FIELD_ID 0
+#define VERTEX_FIELD_LABEL 1
+#define VERTEX_FIELD_PROPERTIES 2
+#define VERTEX_NUM_FIELDS 3
+
+#define EDGE_FIELD_ID 0
+#define EDGE_FIELD_LABEL 1
+#define EDGE_FIELD_END_ID 2
+#define EDGE_FIELD_START_ID 3
+#define EDGE_FIELD_PROPERTIES 4
+#define EDGE_NUM_FIELDS 5
+
+/*
+ * Macros for direct field access from vertex/edge agtype_value objects.
+ * These avoid the binary search overhead of GET_AGTYPE_VALUE_OBJECT_VALUE.
+ * Validation is integrated - macros will error if field count is incorrect.
+ * Uses GCC statement expressions to allow validation within expressions.
+ */
+#define AGTYPE_VERTEX_GET_ID(v) \
+ ({ \
+ if ((v)->val.object.num_pairs != VERTEX_NUM_FIELDS) \
+ ereport(ERROR, \
+ (errcode(ERRCODE_DATA_CORRUPTED), \
+ errmsg("invalid vertex structure: expected %d fields,
found %d", \
+ VERTEX_NUM_FIELDS, (v)->val.object.num_pairs))); \
+ &(v)->val.object.pairs[VERTEX_FIELD_ID].value; \
+ })
Review Comment:
The direct-access macros only validate `num_pairs` and then assume the field
order is correct. If a value is corrupted (or constructed) such that it still
has `VERTEX_NUM_FIELDS`/`EDGE_NUM_FIELDS` pairs but the keys aren’t the
expected ones at those indices, these macros will return the wrong field
without error. Consider also validating the key names (and lengths) at the
expected indices (e.g., "id", "label", "properties") and erroring if they don’t
match.
--
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]
