This is an automated email from the ASF dual-hosted git repository.
jrgemignani pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/age.git
The following commit(s) were added to refs/heads/master by this push:
new 795a881d perf(agtype): arena passthrough + binary-direct id extraction
(#2424)
795a881d is described below
commit 795a881d20d6b7cf74439a8be06289ac3e390dc4
Author: John Gemignani <[email protected]>
AuthorDate: Mon Jun 1 11:52:20 2026 -0700
perf(agtype): arena passthrough + binary-direct id extraction (#2424)
Targets the agtype value-tree allocation cost that dominates LDBC IC4 and
other sort/comparator-bound queries. Two complementary optimizations
(A1 arena, A2 fast-id) plus the sanitizer issues uncovered while
verifying the change.
A1 — per-tuple agtype arena (Pattern B applied):
Add agt_arena_begin / AGT_ARENA_BEGIN_SHARED / agt_arena_reset /
agt_arena_end helpers (agtype_util.c, agtype.h). Two usage patterns:
disposable per-call arenas for one-shot tree builds, and a long-lived
shared arena (file-static, parented under CacheMemoryContext) for hot
inner loops that would otherwise pay AllocSetCreate cost per call.
Apply to compare_agtype_scalar_containers: when the comparator
deserializes VERTEX/EDGE/PATH composites via fill_agtype_value_no_copy,
route those allocations into a shared arena and reset (O(1)) at the
end of the comparison instead of recursively pfree'ing the tree (O(N)).
Master IC4 spent 17.64% of total CPU in pfree_agtype_value_content from
this comparator; the arena collapses that walk to one MemoryContextReset.
A2 — binary-direct id extraction:
When compare_agtype_scalar_containers compares two VERTEX or two EDGE
values, only the graphid 'id' determines order
(compare_agtype_scalar_values
ignores all other fields). Master IC4 spent 73.48% inclusive in
ag_deserialize_composite called from this comparator, building the entire
agtype_value tree (label, properties, nested values) just to read one
int64.
Add extract_composite_id_fast: walks the binary VERTEX/EDGE container
directly and reads the int64 id at field index 0 (always 'id' due to
length-sorted key ordering established by uniqueify_agtype_object — the
same invariant PR #2302 leveraged for direct array indexing).
Wired into compare_agtype_scalar_containers as a fast path before the
existing arena+slow path. Falls through on cross-type comparisons
(VERTEX vs EDGE), PATH, and malformed extended-type entries.
ASAN/UBSan fixes (10 issues found while verifying A1+A2 under
-fsanitize=address -fsanitize=undefined; 1 introduced by A2, 9
pre-existing).
The 11th finding (uninitialized parent_cpstate fields in cypher_analyze.c)
was independently fixed upstream by PR #2423; this branch is rebased on
top of that fix and no longer carries a duplicate change.
- HIGH: heap-buffer-overflow in agtype_raw.c:write_container — VARSIZE
included the 4-byte varlena header but copy started past it, reading
VARHDRSZ bytes past source allocation. Subtract VARHDRSZ. ASan flagged
4 times in installcheck.
- 7 unaligned int64/float8 reads/writes (AGT_HEADER is 4 bytes, leaving
payload 4-byte- but not 8-byte-aligned). UB under strict-alignment
rules. Replaced typed loads/stores with memcpy in agtype_ext.c (4
sites),
agtype_util.c (3 sites incl. extract_composite_id_fast), agtype_raw.c
(write_graphid).
- agtype.c:agtype_hash_cmp: reading r->val.array.raw_scalar at
WAGT_END_ARRAY where the iterator does not populate *val. Replaced
with explicit raw_scalar bool stack tracked across BEGIN/END pairs.
Hash output unchanged.
- agtype_util.c:copy_to_buffer: memcpy(NULL, NULL, 0) is UB. Guard with
if (len > 0).
Performance (SF3 LDBC, paired same-session vs master):
IC sum: 201,930 -> 157,129 ms = -22.19%
IC4: 50,570 -> 7,615 ms = -84.94%
IC5: 21,779 -> 20,646 ms = -5.20%
IS, IU: within noise (<= +/-3% on sub-second queries)
Tests:
34/34 installcheck on production PG18.3
34/34 installcheck on ASAN+UBSan PG18.3 (zero unique runtime errors,
zero heap-buffer-overflows; the original audit found 10 unique
runtime issues plus 1 HBO with 4 instances, all addressed here
or upstream)
Co-authored-by: Claude [email protected]
modified: src/backend/utils/adt/agtype.c
modified: src/backend/utils/adt/agtype_ext.c
modified: src/backend/utils/adt/agtype_raw.c
modified: src/backend/utils/adt/agtype_util.c
modified: src/include/utils/agtype.h
---
src/backend/utils/adt/agtype.c | 57 +++++++-
src/backend/utils/adt/agtype_ext.c | 21 ++-
src/backend/utils/adt/agtype_raw.c | 28 +++-
src/backend/utils/adt/agtype_util.c | 282 ++++++++++++++++++++++++++++++++++--
src/include/utils/agtype.h | 59 ++++++++
5 files changed, 422 insertions(+), 25 deletions(-)
diff --git a/src/backend/utils/adt/agtype.c b/src/backend/utils/adt/agtype.c
index 624d6a88..cfff8138 100644
--- a/src/backend/utils/adt/agtype.c
+++ b/src/backend/utils/adt/agtype.c
@@ -5261,6 +5261,22 @@ Datum agtype_hash_cmp(PG_FUNCTION_ARGS)
agtype_iterator_token tok;
agtype_value *r;
uint64 seed = 0xF0F0F0F0;
+ /*
+ * Stack of "is the current open array a raw_scalar pseudo-array?" so
+ * that the WAGT_END_ARRAY case can match the choice we made at
+ * WAGT_BEGIN_ARRAY. The iterator does not populate *val on END tokens
+ * (see comment above agtype_iterator_next), so r->val.array.raw_scalar
+ * is uninitialized at that point and reading it as a bool is undefined
+ * behavior (UBSan flagged values like 127 here).
+ *
+ * Arrays in agtype are bounded by AGTYPE_CONTAINER_SIZE which fits in
+ * AGT_CMASK; a fixed-size stack of 64 levels is far more than any real
+ * agtype graph value will ever produce. Bail out with an error if we
+ * exceed it rather than silently corrupting the hash.
+ */
+#define HASH_RAW_SCALAR_STACK_DEPTH 64
+ bool raw_scalar_stack[HASH_RAW_SCALAR_STACK_DEPTH];
+ int raw_scalar_top = -1;
/* this function returns INTEGER which is 32 bits */
if (PG_ARGISNULL(0))
@@ -5277,18 +5293,51 @@ 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 */
+ if (raw_scalar_top < 0)
+ {
+ ereport(ERROR,
+ (errcode(ERRCODE_DATA_CORRUPTED),
+ errmsg("agtype_hash_cmp: WAGT_END_ARRAY without
matching WAGT_BEGIN_ARRAY")));
+ }
+ was_raw_scalar = raw_scalar_stack[raw_scalar_top];
+ raw_scalar_top--;
+ if (!was_raw_scalar)
+ {
+ seed = RIGHT_ROTATE(seed, 4);
+ }
+ }
else if (tok == WAGT_END_OBJECT)
seed = RIGHT_ROTATE(seed, 4);
seed = LEFT_ROTATE(seed, 1);
}
+#undef HASH_RAW_SCALAR_STACK_DEPTH
+
pfree_if_not_null(r);
PG_FREE_IF_COPY(agt, 0);
diff --git a/src/backend/utils/adt/agtype_ext.c
b/src/backend/utils/adt/agtype_ext.c
index 7a0ea991..e4a38e3d 100644
--- a/src/backend/utils/adt/agtype_ext.c
+++ b/src/backend/utils/adt/agtype_ext.c
@@ -57,7 +57,14 @@ bool ag_serialize_extended_type(StringInfo buffer, agtentry
*agtentry,
/* copy in the int_value data */
numlen = sizeof(int64);
offset = reserve_from_buffer(buffer, numlen);
- *((int64 *)(buffer->data + offset)) = scalar_val->val.int_value;
+ /*
+ * Use memcpy because the AGT_HEADER (4 bytes) leaves the buffer
+ * write position 4-byte-aligned but not necessarily 8-byte-aligned.
+ * Direct *((int64 *) ...) = ... is undefined behavior under strict
+ * alignment rules (UBSan flags it; works in practice on x86_64).
+ */
+ memcpy(buffer->data + offset, &scalar_val->val.int_value,
+ sizeof(int64));
*agtentry = AGTENTRY_IS_AGTYPE | (padlen + numlen + AGT_HEADER_SIZE);
break;
@@ -68,7 +75,8 @@ bool ag_serialize_extended_type(StringInfo buffer, agtentry
*agtentry,
/* copy in the float_value data */
numlen = sizeof(scalar_val->val.float_value);
offset = reserve_from_buffer(buffer, numlen);
- *((float8 *)(buffer->data + offset)) = scalar_val->val.float_value;
+ memcpy(buffer->data + offset, &scalar_val->val.float_value,
+ sizeof(float8));
*agtentry = AGTENTRY_IS_AGTYPE | (padlen + numlen + AGT_HEADER_SIZE);
break;
@@ -154,12 +162,17 @@ void ag_deserialize_extended_type(char *base_addr, uint32
offset,
{
case AGT_HEADER_INTEGER:
result->type = AGTV_INTEGER;
- result->val.int_value = *((int64 *)(base + AGT_HEADER_SIZE));
+ /* See comment in ag_serialize_extended_type. The 4-byte AGT_HEADER
+ * leaves the int64 at a 4-byte-aligned but not 8-byte-aligned
+ * address, so a direct typed load is undefined behavior. */
+ memcpy(&result->val.int_value, base + AGT_HEADER_SIZE,
+ sizeof(int64));
break;
case AGT_HEADER_FLOAT:
result->type = AGTV_FLOAT;
- result->val.float_value = *((float8 *)(base + AGT_HEADER_SIZE));
+ memcpy(&result->val.float_value, base + AGT_HEADER_SIZE,
+ sizeof(float8));
break;
case AGT_HEADER_VERTEX:
diff --git a/src/backend/utils/adt/agtype_raw.c
b/src/backend/utils/adt/agtype_raw.c
index d8bad3d2..3d56d94d 100644
--- a/src/backend/utils/adt/agtype_raw.c
+++ b/src/backend/utils/adt/agtype_raw.c
@@ -197,9 +197,20 @@ void write_graphid(agtype_build_state *bstate, graphid
graphid)
write_const(AGT_HEADER_INTEGER, AGT_HEADER_TYPE);
length += AGT_HEADER_SIZE;
- /* graphid value */
- write_const(graphid, int64);
- length += sizeof(int64);
+ /*
+ * graphid value (int64). The 4-byte AGT_HEADER above leaves the buffer
+ * write position 4-byte-aligned but not 8-byte-aligned, so the typed
+ * write done by write_const(graphid, int64) is undefined behavior under
+ * strict alignment rules. Use memcpy. (UBSan flags the typed write as
+ * "store to misaligned address ... requires 8 byte alignment".)
+ */
+ {
+ int numlen = sizeof(int64);
+ int g_offset = BUFFER_RESERVE(numlen);
+
+ memcpy(bstate->buffer->data + g_offset, &graphid, sizeof(int64));
+ length += numlen;
+ }
/* agtentry */
write_agt(AGTENTRY_IS_AGTYPE | length);
@@ -214,8 +225,15 @@ void write_container(agtype_build_state *bstate, agtype
*agtype)
/* padding */
length += BUFFER_WRITE_PAD();
- /* varlen data */
- length += write_ptr((char *) &agtype->root, VARSIZE(agtype));
+ /*
+ * Copy the inner agtype_container only, NOT the outer varlena header.
+ * VARSIZE(agtype) reports the total varlena size (including the 4-byte
+ * vl_len_ header), but we are starting our copy at &agtype->root, which
+ * is already past that header. Subtracting VARHDRSZ avoids reading
+ * VARHDRSZ bytes past the source allocation (caught by ASan as
+ * heap-buffer-overflow in __interceptor_memcpy from write_pointer).
+ */
+ length += write_ptr((char *) &agtype->root, VARSIZE(agtype) - VARHDRSZ);
/* agtentry */
write_agt(AGTENTRY_IS_CONTAINER | length);
diff --git a/src/backend/utils/adt/agtype_util.c
b/src/backend/utils/adt/agtype_util.c
index b3972341..90e54070 100644
--- a/src/backend/utils/adt/agtype_util.c
+++ b/src/backend/utils/adt/agtype_util.c
@@ -103,6 +103,66 @@ static int get_type_sort_priority(enum agtype_value_type
type);
static void pfree_iterator_agtype_value_token(agtype_iterator_token token,
agtype_value *agtv);
+/*
+ * Per-call agtype build arena.
+ *
+ * Hot Datum functions in agtype.c build an agtype_value tree, serialize it
+ * via agtype_value_to_agtype(), then recursively free it with
+ * pfree_agtype_value_content(). The recursive free is O(N) in the number of
+ * tree nodes; the arena helpers below let callers replace it with an O(K)
+ * block reset (K = number of allocated AllocSet blocks; typically 1 for
+ * small trees) by allocating the entire tree inside a short-lived
+ * MemoryContext.
+ *
+ * The arena is sized for typical agtype_value trees; it grows on demand.
+ * Cost of creating and deleting the context is amortized against the
+ * eliminated per-node pfree walk and per-node AllocSetFree calls.
+ *
+ * agt_arena_end() is a no-op on NULL, allowing simple cleanup paths.
+ *
+ * The AGT_ARENA_BEGIN_SHARED() macro (declared in agtype.h) returns a
+ * context parented under CacheMemoryContext (process lifetime) so the
+ * caller may stash it in a file-static variable for reuse across many
+ * invocations. The first caller pays AllocSetContextCreate cost;
+ * subsequent callers reuse the context and pay only MemoryContextReset
+ * (cheap; ~100 ns when nothing was allocated since last reset). Use this
+ * for hot inner loops (sort comparators, per-tuple deep-copy workspaces).
+ */
+MemoryContext agt_arena_begin(void)
+{
+ /*
+ * ALLOCSET_SMALL_SIZES (initial=1KB, max=8KB). Most agtype value trees
+ * fit in a single 1KB block.
+ */
+ return AllocSetContextCreate(CurrentMemoryContext,
+ "agtype build arena",
+ ALLOCSET_SMALL_SIZES);
+}
+
+/*
+ * Pattern B (shared, long-lived arena) is exposed as the macro
+ * AGT_ARENA_BEGIN_SHARED in agtype.h, because PG's AllocSetContextCreate
+ * enforces a compile-time-constant name via StaticAssertStmt. The macro
+ * inlines the AllocSet creation at the call site so the name literal is
+ * visible to the StaticAssert.
+ */
+
+void agt_arena_reset(MemoryContext arena)
+{
+ if (arena != NULL)
+ {
+ MemoryContextReset(arena);
+ }
+}
+
+void agt_arena_end(MemoryContext arena)
+{
+ if (arena != NULL)
+ {
+ MemoryContextDelete(arena);
+ }
+}
+
/*
* Turn an in-memory agtype_value into an agtype for on-disk storage.
*
@@ -834,12 +894,18 @@ static void fill_agtype_value_no_copy(agtype_container
*container, int index,
{
case AGT_HEADER_INTEGER:
result->type = AGTV_INTEGER;
- result->val.int_value = *((int64 *)(base + AGT_HEADER_SIZE));
+ /* See ag_serialize_extended_type: the 4-byte AGT_HEADER leaves
+ * the int64 at a 4-byte-aligned but not 8-byte-aligned address.
+ * Use memcpy to avoid undefined behavior on strict alignment
+ * platforms (caught by UBSan). */
+ memcpy(&result->val.int_value, base + AGT_HEADER_SIZE,
+ sizeof(int64));
break;
case AGT_HEADER_FLOAT:
result->type = AGTV_FLOAT;
- result->val.float_value = *((float8 *)(base + AGT_HEADER_SIZE));
+ memcpy(&result->val.float_value, base + AGT_HEADER_SIZE,
+ sizeof(float8));
break;
default:
@@ -880,8 +946,122 @@ 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.
+ *
+ * On success, writes the extracted graphid through *out and returns true.
+ * Returns false (leaving *out untouched) if the id field is not a well-formed
+ * extended AGTV_INTEGER, signaling the caller to fall back to the slow path.
+ * A bool result is used instead of a sentinel return value because a valid
+ * graphid can legitimately take any int64 value (e.g. label_id 0x8000 with
+ * entry_id 0 equals PG_INT64_MIN), so no in-band sentinel is safe.
+ *
+ * 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 bool extract_composite_id_fast(const agtype_container *outer_scalar,
+ graphid *out)
+{
+ 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 false;
+ }
+
+ {
+ graphid id;
+
+ memcpy(&id, id_extended_base + AGT_HEADER_SIZE, sizeof(int64));
+ *out = id;
+ return true;
+ }
+}
+
static int compare_agtype_scalar_containers(agtype_container *a,
agtype_container *b)
{
@@ -892,6 +1072,7 @@ static int
compare_agtype_scalar_containers(agtype_container *a,
int result;
bool need_free_a = false;
bool need_free_b = false;
+ MemoryContext saved_ctx = NULL;
Assert(AGTYPE_CONTAINER_IS_SCALAR(a));
Assert(AGTYPE_CONTAINER_IS_SCALAR(b));
@@ -900,13 +1081,77 @@ 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 hb;
+
+ /*
+ * The header is uint32 at an INTALIGN'd (4-byte) address, so a typed
+ * load would be safe, but use memcpy for consistency with the rest of
+ * this comparator (extract_composite_id_fast reads the same header via
+ * memcpy) and to stay robust if AGT_HEADER_TYPE is ever widened.
+ */
+ memcpy(&ha,
+ base_addr_a + INTALIGN(get_agtype_offset(a, 0)),
+ sizeof(AGT_HEADER_TYPE));
+ memcpy(&hb,
+ base_addr_b + INTALIGN(get_agtype_offset(b, 0)),
+ sizeof(AGT_HEADER_TYPE));
+
+ if (ha == hb &&
+ (ha == AGT_HEADER_VERTEX || ha == AGT_HEADER_EDGE))
+ {
+ graphid ida;
+ graphid idb;
+
+ /*
+ * extract_composite_id_fast returns false on a malformed id
+ * field; fall through to the slow path in that rare case rather
+ * than producing a wrong comparison.
+ */
+ if (extract_composite_id_fast(a, &ida) &&
+ extract_composite_id_fast(b, &idb))
+ {
+ if (ida == idb)
+ {
+ return 0;
+ }
+ return (ida > idb) ? 1 : -1;
+ }
+ }
+ }
+
+ /*
+ * Peek at the entry types without filling, so we can route any composite
+ * allocations into the shared arena instead of CurrentMemoryContext.
+ * This avoids the recursive pfree walk after every comparison.
+ */
+ if (AGTE_IS_AGTYPE(a->children[0]) || AGTE_IS_AGTYPE(b->children[0]))
+ {
+ if (compare_scalar_arena == NULL)
+ {
+ compare_scalar_arena =
+ AGT_ARENA_BEGIN_SHARED("agtype scalar comparator");
+ }
+ saved_ctx = MemoryContextSwitchTo(compare_scalar_arena);
+ }
+
/* Use no-copy fill to avoid allocations for simple types */
fill_agtype_value_no_copy(a, 0, base_addr_a, 0, &va);
fill_agtype_value_no_copy(b, 0, base_addr_b, 0, &vb);
/*
- * Check if we need to free the values after comparison.
- * Only VERTEX, EDGE, and PATH types allocate memory in no-copy mode.
+ * Track which sides allocated composite content (VERTEX/EDGE/PATH).
+ * For the arena-backed allocations we still need this so we know to
+ * reset the arena at the end; for the (rare) caller-context-backed
+ * fallback path we still need to recursively free.
*/
if (va.type == AGTV_VERTEX || va.type == AGTV_EDGE || va.type == AGTV_PATH)
{
@@ -936,14 +1181,17 @@ static int
compare_agtype_scalar_containers(agtype_container *a,
get_type_sort_priority(vb.type)) ? -1 : 1;
}
- /* Free any allocated memory from composite types */
- if (need_free_a)
- {
- pfree_agtype_value_content(&va);
- }
- if (need_free_b)
+ /*
+ * Cleanup. If we allocated into the shared arena, reset it (O(1));
+ * otherwise the no-copy fill made no allocations to free.
+ */
+ if (saved_ctx != NULL)
{
- pfree_agtype_value_content(&vb);
+ MemoryContextSwitchTo(saved_ctx);
+ if (need_free_a || need_free_b)
+ {
+ agt_arena_reset(compare_scalar_arena);
+ }
}
return result;
@@ -2127,7 +2375,17 @@ int reserve_from_buffer(StringInfo buffer, int len)
static void copy_to_buffer(StringInfo buffer, int offset, const char *data,
int len)
{
- memcpy(buffer->data + offset, data, len);
+ /*
+ * Guard against memcpy(dst, NULL, 0). Some callers (notably empty PATH
+ * scalars with no element bytes) reach here with data == NULL and len
+ * == 0; the C standard says memcpy with a NULL pointer is undefined even
+ * when len == 0. UBSan flags this as "null pointer passed as argument 2,
+ * which is declared to never be null".
+ */
+ if (len > 0)
+ {
+ memcpy(buffer->data + offset, data, len);
+ }
}
/*
diff --git a/src/include/utils/agtype.h b/src/include/utils/agtype.h
index b10474ce..4bddbe4f 100644
--- a/src/include/utils/agtype.h
+++ b/src/include/utils/agtype.h
@@ -32,6 +32,7 @@
#define AG_AGTYPE_H
#include "utils/array.h"
+#include "utils/memutils.h"
#include "utils/numeric.h"
#include "utils/graphid.h"
@@ -581,6 +582,64 @@ agtype_iterator_token agtype_iterator_next(agtype_iterator
**it,
agtype *agtype_value_to_agtype(agtype_value *val);
bool agtype_deep_contains(agtype_iterator **val,
agtype_iterator **m_contained, bool skip_nested);
+
+/*
+ * Per-call agtype build arena.
+ *
+ * Many top-level Datum functions in agtype.c follow the pattern:
+ * 1. Allocate / build an agtype_value tree
+ * 2. Serialize it via agtype_value_to_agtype()
+ * 3. Recursively pfree the tree
+ *
+ * Step 3 (pfree_agtype_value_content) is an O(N) walk that frees each tree
+ * node individually. By building the tree inside a dedicated AllocSet
+ * MemoryContext and resetting the context after serialization, we trade
+ * the O(N) walk for an O(K) block reset (K << N) and eliminate per-node
+ * AllocSetFree overhead. The AllocSetAlloc cost is also reduced because
+ * the arena uses a small initial block size and grows on demand.
+ *
+ * Two usage patterns:
+ *
+ * Pattern A — disposable per-call arena (one-shot):
+ * MemoryContext arena = agt_arena_begin();
+ * MemoryContext caller = MemoryContextSwitchTo(arena);
+ * ... build tree ...
+ * MemoryContextSwitchTo(caller);
+ * result = agtype_value_to_agtype(val); // copies into caller
+ * agt_arena_end(arena); // resets / discards
+ *
+ * Pattern B — long-lived shared arena (sort comparator, hot inner loop):
+ * static MemoryContext shared_arena = NULL;
+ * if (shared_arena == NULL)
+ * {
+ * shared_arena = AGT_ARENA_BEGIN_SHARED("comparator");
+ * }
+ * MemoryContext caller = MemoryContextSwitchTo(shared_arena);
+ * ... allocate / use ...
+ * MemoryContextSwitchTo(caller);
+ * agt_arena_reset(shared_arena); // O(1) cheap reset
+ *
+ * Pattern A creates a fresh context per call (microseconds). Pattern B
+ * amortizes the create cost across many calls (single AllocSetCreate at
+ * the very first call), and pays only a MemoryContextReset (~100 ns) per
+ * subsequent call. Use Pattern B when the function is invoked many times
+ * per query (e.g. a sort comparator called O(N log N) times).
+ */
+MemoryContext agt_arena_begin(void);
+/*
+ * Pattern-B shared-arena creator. Implemented as a macro because PG's
+ * AllocSetContextCreate enforces memory-context names be compile-time
+ * constants via a StaticAssert. Use with a string literal:
+ * static MemoryContext my_arena = NULL;
+ * if (my_arena == NULL)
+ * {
+ * my_arena = AGT_ARENA_BEGIN_SHARED("my comparator workspace");
+ * }
+ */
+#define AGT_ARENA_BEGIN_SHARED(_name) \
+ AllocSetContextCreate(CacheMemoryContext, (_name), ALLOCSET_SMALL_SIZES)
+void agt_arena_reset(MemoryContext arena);
+void agt_arena_end(MemoryContext arena);
void agtype_hash_scalar_value(const agtype_value *scalar_val, uint32 *hash);
void agtype_hash_scalar_value_extended(const agtype_value *scalar_val,
uint64 *hash, uint64 seed);
