perf(diff): batch node field summary retrieval over paged queries - #10106
perf(diff): batch node field summary retrieval over paged queries#10106ajtmccarty wants to merge 1 commit into
Conversation
Aggregating every changed node's altered field names in a single query does not scale with large diffs and can exhaust the database heap. Page the query over changed nodes instead, with each row carrying one node's altered fields, and fold the per-node summaries into per-kind NodeDiffFieldSummary instances in the repository. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
2 issues found across 4 files
Confidence score: 3/5
- In
backend/infrahub/core/diff/repository/repository.py, offset pagination means late pages (ORDER BY ... SKIP) can force work over almost the entire changed-node set, which can spike latency and heap usage on very large diffs; switch to keyset/cursor pagination using the last(uuid, elementId)to keep per-page cost bounded. - In
backend/infrahub/core/diff/query/field_summary.py, growing offsets can make the final page sort nearly all changed nodes beforeLIMIT, creating unbounded query and memory pressure for large diffs; adopt the same(uuid, elementId)keyset cursor approach here to prevent progressive slowdown.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/infrahub/core/diff/repository/repository.py">
<violation number="1" location="backend/infrahub/core/diff/repository/repository.py:615">
P1: Very large diffs can still cause progressively larger query work and heap use: offset pagination makes the last `ORDER BY ... SKIP` page process nearly every changed node. Cursor/keyset pagination using the last `(uuid, elementId)` would keep each query bounded and meet the stated memory goal.</violation>
</file>
<file name="backend/infrahub/core/diff/query/field_summary.py">
<violation number="1" location="backend/infrahub/core/diff/query/field_summary.py:58">
P2: Large diffs can still consume unbounded query work/memory as offsets grow: the final page must order nearly every changed node before `LIMIT` applies. Use keyset pagination (last `(uuid, elementId)` cursor) so each query seeks after the previous node and limits immediately.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| kind_summary.merge(node_summary) | ||
| if num_nodes < node_limit: | ||
| break | ||
| node_offset += node_limit |
There was a problem hiding this comment.
P1: Very large diffs can still cause progressively larger query work and heap use: offset pagination makes the last ORDER BY ... SKIP page process nearly every changed node. Cursor/keyset pagination using the last (uuid, elementId) would keep each query bounded and meet the stated memory goal.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/core/diff/repository/repository.py, line 615:
<comment>Very large diffs can still cause progressively larger query work and heap use: offset pagination makes the last `ORDER BY ... SKIP` page process nearly every changed node. Cursor/keyset pagination using the last `(uuid, elementId)` would keep each query bounded and meet the stated memory goal.</comment>
<file context>
@@ -588,11 +588,32 @@ async def get_affected_node_uuids(
+ kind_summary.merge(node_summary)
+ if num_nodes < node_limit:
+ break
+ node_offset += node_limit
+ return list(summaries_by_kind.values())
</file context>
| WHERE attr_name IS NOT NULL | ||
| RETURN collect({name: attr_name, node_uuids: attr_node_uuids}) AS attr_name_uuids | ||
| WITH n | ||
| ORDER BY n.uuid, elementId(n) |
There was a problem hiding this comment.
P2: Large diffs can still consume unbounded query work/memory as offsets grow: the final page must order nearly every changed node before LIMIT applies. Use keyset pagination (last (uuid, elementId) cursor) so each query seeks after the previous node and limits immediately.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/infrahub/core/diff/query/field_summary.py, line 58:
<comment>Large diffs can still consume unbounded query work/memory as offsets grow: the final page must order nearly every changed node before `LIMIT` applies. Use keyset pagination (last `(uuid, elementId)` cursor) so each query seeks after the previous node and limits immediately.</comment>
<file context>
@@ -51,48 +54,40 @@ async def query_init(self, db: InfrahubDatabase, **kwargs: Any) -> None: # noqa
- WHERE attr_name IS NOT NULL
- RETURN collect({name: attr_name, node_uuids: attr_node_uuids}) AS attr_name_uuids
+ WITH n
+ ORDER BY n.uuid, elementId(n)
+ SKIP $node_offset
+ LIMIT $node_limit
</file context>
Encountered an issue with the query to retrieve diff summaries of nodes when running merge performance testing locally on a very large branch. Could be that it was just too much data for my machine to handle, but better safe than sorry and this batching will cost us very little.
Why
Retrieving the node field summaries for a diff currently runs one query that aggregates the changed field names and node UUIDs of every changed node per kind in a single transaction. On large diffs (tens of thousands of changed nodes) that aggregation does not scale and can exhaust the Neo4j heap (
Java heap spaceerrors during merge/rebase validation).This PR pages the retrieval over the changed nodes so memory use per query is bounded by
database.query_size_limit, regardless of diff size.Non-goals: no change to what the summaries contain or how consumers (schema-constraint validation) use them.
What changed
Behavioral changes:
DiffRepository.get_node_field_summaries()now issues one query per page of changed nodes instead of a single aggregate query. Results are identical; only the retrieval strategy changed.Implementation notes:
EnrichedDiffNodeFieldSummaryQuerynow selects one page of changed nodes (SKIP/LIMITover a strictORDER BY n.uuid, elementId(n)) and collects each node's altered attribute/relationship names per row, rather than aggregating across the whole diff per kind.get_node_field_rows()yields one single-nodeNodeDiffFieldSummaryper row; the repository folds them into per-kind summaries via the newNodeDiffFieldSummary.merge().DIFF_HAS_NODEmatch staysOPTIONALso a diff root with no changed nodes still produces a (node-less) row. Those rows are skipped when yielding summaries; this cannot terminate pagination early because nulls sort after every real node in theORDER BY.What stayed the same: the shape of
NodeDiffFieldSummary(per-field node-UUID maps and derived views) and the repository's public API are unchanged.How to review
The part deserving the most scrutiny is the pagination-termination logic: the loop breaks when a page yields fewer rows than
query_size_limit, and the node-less rows from theOPTIONAL MATCHoccupy page slots without being counted. This is safe only because nulls sort last in the query'sORDER BY— see theget_node_field_rows()docstring.Impact & rollout
query_size_limitchanged nodes.Checklist
uv run towncrier create ...)