Copilot commented on code in PR #2424:
URL: https://github.com/apache/age/pull/2424#discussion_r3336218711
##########
src/backend/utils/adt/agtype_util.c:
##########
@@ -880,8 +946,113 @@ static void fill_agtype_value_no_copy(agtype_container
*container, int index,
* of the full iterator machinery. It extracts the scalar values using no-copy
* fill and compares them directly.
*
+ * For composite types (VERTEX, EDGE, PATH) the no-copy fill still
+ * deserializes into newly-allocated memory. To avoid the per-call
+ * pfree_agtype_value_content recursive walk on every sort comparator
+ * invocation, those allocations are routed through a long-lived shared
+ * arena that is reset (O(1)) at the end of each call. The arena is
+ * created lazily on the first call that needs it (i.e. the first
+ * VERTEX/EDGE/PATH comparison) and reused across all subsequent calls in
+ * the process.
+ *
+ * Additional fast path (A2): when both containers are VERTEX or EDGE of
+ * the same type, the comparator only needs the graphid `id` field
+ * (compare_agtype_scalar_values does this; all other fields are ignored).
+ * Skip the full ag_deserialize_composite agtype_value tree build entirely
+ * and walk the binary representation directly to read just the int64 id.
+ * This is the dominant cost of sort-bound queries (notably IC4) since the
+ * tree build is O(properties + label) per row when only one int64 is
+ * needed for ordering.
+ *
* Returns: negative if a < b, 0 if a == b, positive if a > b
*/
+static MemoryContext compare_scalar_arena = NULL;
+
+/*
+ * Fast int64-id extraction from a binary VERTEX or EDGE container.
+ *
+ * Layout of an extended VERTEX/EDGE entry (set up by
ag_serialize_extended_type
+ * with convert_extended_object): a 4-byte AGT_HEADER followed by a standard
+ * agtype_container holding an object whose first field (sorted by key length
+ * ascending) is "id". The id is itself stored as an extended AGTV_INTEGER:
+ * a 4-byte AGT_HEADER_INTEGER followed by an int64.
+ *
+ * Pair layout in an object container: children[0..N-1] are the keys, then
+ * children[N..2N-1] are the values; data follows. The "id" key is at index 0
+ * because "id" (length 2) is the shortest key in both VERTEX (id, label,
+ * properties) and EDGE (id, label, end_id, start_id, properties), and the
+ * agtype object representation sorts pairs by key length ascending.
+ *
+ * Returns the extracted graphid. The caller is responsible for ensuring the
+ * input container actually holds a VERTEX or EDGE (i.e. this is only called
+ * after AGTE_IS_AGTYPE indicates an extended type and the AGT_HEADER matches
+ * AGT_HEADER_VERTEX or AGT_HEADER_EDGE).
+ */
+static graphid extract_composite_id_fast(const agtype_container *outer_scalar)
+{
+ const char *base_addr;
+ const agtype_container *obj;
+ int n_pairs;
+ const agtentry *children;
+ uint32 id_value_offset;
+ char *id_value_base;
+ char *id_extended_base;
+ AGT_HEADER_TYPE id_header;
+ int id_value_index;
+
+ /* outer_scalar is a single-element pseudo-array; element 0 is our
extended type */
+ base_addr = (const char *)&outer_scalar->children[1];
+
+ /*
+ * Element 0 is an extended type. Its data starts at base_addr + 0 and
+ * begins with AGT_HEADER_VERTEX or AGT_HEADER_EDGE; the inner
+ * agtype_container follows immediately.
+ */
+ obj = (const agtype_container *)((const char *)base_addr +
AGT_HEADER_SIZE);
+ n_pairs = AGTYPE_CONTAINER_SIZE(obj);
+ children = obj->children;
+
+ /*
+ * Pair value 0 ("id") lives at child index n_pairs (values come after
+ * keys). Compute its offset within the object's data area.
+ */
+ id_value_index = n_pairs;
+ id_value_offset = get_agtype_offset(obj, id_value_index);
+
+ /* Data area for this object starts after children[2*n_pairs] */
+ id_value_base = (char *)&children[2 * n_pairs];
+
+ /*
+ * The id value is an extended AGTV_INTEGER: AGT_HEADER (aligned) followed
+ * by the int64. INTALIGN matches what ag_deserialize_extended_type does.
+ */
+ id_extended_base = id_value_base + INTALIGN(id_value_offset);
+ /*
+ * The header is uint32 (4-byte) and lives at a 4-byte-aligned address,
+ * so a typed load is fine. The int64 that follows starts at offset
+ * AGT_HEADER_SIZE = 4 from id_extended_base, so it is 4-byte- but not
+ * necessarily 8-byte-aligned; use memcpy to avoid alignment UB.
+ */
+ memcpy(&id_header, id_extended_base, sizeof(AGT_HEADER_TYPE));
+
+ /*
+ * Defensive: if the value isn't actually an extended integer, fall back
+ * to the slow deserialize path by signaling. AGT_HEADER_INTEGER is the
+ * only valid header for the id field of a well-formed VERTEX/EDGE.
+ */
+ if (id_header != AGT_HEADER_INTEGER)
+ {
+ return PG_INT64_MIN;
+ }
+
+ {
+ graphid id;
+
+ memcpy(&id, id_extended_base + AGT_HEADER_SIZE, sizeof(int64));
+ return id;
+ }
Review Comment:
Using `PG_INT64_MIN` as an error sentinel is fragile because it can collide
with a valid `graphid`. A `graphid` is an `int64` composed as `(label_id << 48)
| (entry_id & ENTRY_ID_MASK)`, with `LABEL_ID_MAX = 65535`. Any graphid whose
`label_id >= 32768` has bit 63 set and is therefore negative as int64;
specifically, `label_id == 0x8000` and `entry_id == 0` produces exactly
`PG_INT64_MIN`. The slow-path fallback is then silently skipped and a valid id
is treated as malformed (producing a wrong comparison or, in the caller, a
regression to the slow path that bypasses both fast paths). Prefer an explicit
out-parameter or returning a `bool` and writing the id through a pointer (e.g.
`static bool extract_composite_id_fast(const agtype_container *, graphid
*out)`).
##########
src/backend/utils/adt/agtype_util.c:
##########
@@ -900,13 +1072,65 @@ static int
compare_agtype_scalar_containers(agtype_container *a,
base_addr_a = (char *)&a->children[1];
base_addr_b = (char *)&b->children[1];
+ /*
+ * A2 fast path: when both sides are extended-type scalars holding the
+ * same composite kind (VERTEX or EDGE), we only need the int64 id field
+ * to determine ordering (see compare_agtype_scalar_values' AGTV_VERTEX
+ * and AGTV_EDGE cases). Skip the full agtype_value tree build entirely.
+ */
+ if (AGTE_IS_AGTYPE(a->children[0]) && AGTE_IS_AGTYPE(b->children[0]))
+ {
+ AGT_HEADER_TYPE ha = *(AGT_HEADER_TYPE *)
+ (base_addr_a +
+ INTALIGN(get_agtype_offset(a, 0)));
+ AGT_HEADER_TYPE hb = *(AGT_HEADER_TYPE *)
+ (base_addr_b +
+ INTALIGN(get_agtype_offset(b, 0)));
Review Comment:
These two `*(AGT_HEADER_TYPE *)` typed loads are inconsistent with the rest
of the PR, which deliberately replaces typed loads with `memcpy` at addresses
with no stronger than 4-byte alignment guarantee (e.g.
`extract_composite_id_fast` uses `memcpy(&id_header, ...,
sizeof(AGT_HEADER_TYPE))` at line 1036 for the exact same uint32 header). Since
`AGT_HEADER_TYPE` is `uint32` and the address is `INTALIGN`'d, the typed load
is technically safe today, but mixing both styles within the same PR (and the
same comparator) is confusing and invites future regressions if
`AGT_HEADER_TYPE` is ever widened. Use `memcpy` here as well for consistency.
##########
src/backend/utils/adt/agtype.c:
##########
@@ -5277,18 +5293,46 @@ Datum agtype_hash_cmp(PG_FUNCTION_ARGS)
{
if (IS_A_AGTYPE_SCALAR(r) && AGTYPE_ITERATOR_TOKEN_IS_HASHABLE(tok))
agtype_hash_scalar_value_extended(r, &hash, seed);
- else if (tok == WAGT_BEGIN_ARRAY && !r->val.array.raw_scalar)
- seed = LEFT_ROTATE(seed, 4);
+ else if (tok == WAGT_BEGIN_ARRAY)
+ {
+ /* push raw_scalar state for the matching END token */
+ raw_scalar_top++;
+ if (raw_scalar_top >= HASH_RAW_SCALAR_STACK_DEPTH)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("agtype_hash_cmp: array nesting depth exceeds
%d",
+ HASH_RAW_SCALAR_STACK_DEPTH)));
+ }
+ raw_scalar_stack[raw_scalar_top] = r->val.array.raw_scalar;
+ if (!r->val.array.raw_scalar)
+ {
+ seed = LEFT_ROTATE(seed, 4);
+ }
+ }
else if (tok == WAGT_BEGIN_OBJECT)
seed = LEFT_ROTATE(seed, 6);
- else if (tok == WAGT_END_ARRAY && !r->val.array.raw_scalar)
- seed = RIGHT_ROTATE(seed, 4);
+ else if (tok == WAGT_END_ARRAY)
+ {
+ bool was_raw_scalar;
+
+ /* pop matching BEGIN's raw_scalar state */
+ Assert(raw_scalar_top >= 0);
+ was_raw_scalar = raw_scalar_stack[raw_scalar_top];
+ raw_scalar_top--;
Review Comment:
The `Assert(raw_scalar_top >= 0)` is the only guard against an underflow
read of `raw_scalar_stack[-1]`. In a non-assert (release) build, a malformed
iterator stream that emits `WAGT_END_ARRAY` before a matching
`WAGT_BEGIN_ARRAY` would dereference outside the stack and decrement
`raw_scalar_top` below `-1`, then potentially overflow on the next `BEGIN`.
Consider promoting this to a runtime check (`ereport(ERROR, ...)`) — the
surrounding code already uses `ereport` for the overflow case at line 5302, so
handling underflow symmetrically would be consistent and just as cheap.
--
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]
