perf(ast): stop heap-copying nodes in Children() so Walk/Inspect allocate less (#557)

[ajitpratap0]

Summary

#557 perf(ast): stop heap-copying nodes in Children() so Walk/Inspect allocate less

Children() in pkg/sql/ast no longer copies each value-typed child to the heap (x := x; &x). It now returns pointers into the parent's slices (&s.From[i], &s.Joins[i], &f.OrderBy[i], ...) and sizes the result slice up front. Walk and Inspect use an unexported childrenOf, so JoinClause, WindowFrame and MergeStatement are visited in place, and the exported method signatures stay unchanged. Measured with benchstat (-count=6, p=0.002):

• Inspect/select_join_window: 47 to 26 allocs/op, 3216 to 928 B/op
• Fingerprint: 182 to 170 allocs/op
• Lint/Realistic: 728 to 580 allocs/op, -36% B/op
• Extract*: 20-45% fewer allocs
• Scan* and Transpile: unchanged

Visit order is unchanged: a golden generated from the parent commit passes on both parent and HEAD, and a Walk-vs-Children equivalence test passes. One intended behaviour change: a visitor that mutates value-typed children (table references, ORDER BY items, WHEN clauses, update assignments, frame bounds, MERGE target/source) now changes the AST, and the change shows up in SQL() and the formatter. The new regression test fails on the parent and passes on HEAD. The full race gate (uncached), go vet and golangci-lint all pass.

Commits (19), one per issue:

• 7b173af perf(ast): stop heap-copying nodes in Children() so Walk/Inspect allocate less (perf(ast): stop heap-copying nodes in Children() so Walk/Inspect allocate less #557)
• 74a9567 test(ast): move releaseDrawnConstructors into the policy test file
• a7fb5ff Merge branch 'main' into perf/ast-pool-release-fixes
• 142bff2 chore(wasm): stop tracking generated playground binary (chore(wasm): stop tracking generated playground binary #580)
• 0ef0966 perf(parser): allocation-free preprocess, qualified identifiers and clause parsing (perf(parser): remove allocations from preprocess keyword remapping (toUpper) and compound-token expansion #551, perf(parser): reuse the first Identifier for qualified column refs instead of allocating a second #553, perf(parser): replace fmt and strings.ToUpper in clause parsing with allocation-free equivalents #554) (perf(parser): allocation-free preprocess, qualified identifiers and clause parsing (#551, #553, #554) #578)
• ec610ed test(ast): guard release policy against parser drift and cover overflow paths
• dbd8c76 Merge remote-tracking branch 'origin/main' into fix-581
• 809cc75 perf(schema): remove N+1 queries and nested open cursors from the Postgres/MySQL/SQLite schema loaders (perf(schema): remove N+1 queries and nested open cursors from the Postgres/MySQL/SQLite schema loaders #565) (perf(schema): remove N+1 queries and nested open cursors from the Postgres/MySQL/SQLite schema loaders (#565) #570)
• 65ac450 perf(ast): make SQL() serialization allocation-light (drop fmt/ToUpper, fix builderPool, writer-based rendering) (perf(ast): make SQL() serialization allocation-light (drop fmt/ToUpper, fix builderPool, writer-based rendering) #558) (perf(ast): make SQL() serialization allocation-light (drop fmt/ToUpper, fix builderPool, writer-based rendering) (#558) #575)
• c5b870f perf(gosqlx): extract metadata in one AST pass and key qualified collectors by struct (perf(gosqlx): extract metadata in one AST pass and key qualified collectors by struct #561) (perf(gosqlx): extract metadata in one AST pass and key qualified collectors by struct (#561) #572)
• 4598431 perf(security): add derived literal prefilters to ScanSQL and hoist lookup tables (perf(security): hoist per-call maps in ScanSQL and add verified literal prefilters ahead of regex groups #563) (perf(security): hoist per-call maps in ScanSQL and add verified literal prefilters ahead of regex groups (#563) #574)
• 498aeca perf(tokenizer): linear position tracking, ASCII fast paths, counted token-slice capacity (perf(tokenizer): make toSQLPosition O(1) amortized to remove quadratic tokenization #544, perf(tokenizer): replace per-identifier strings.ToUpper with allocation-free ASCII upper-casing #545, perf(tokenizer): add ASCII lookup-table fast path to identifier character classification #547, perf(tokenizer): fix the len/4 token-slice capacity estimate (regrows on dense SQL, over-allocates on typical SQL) #549) (perf(tokenizer): linear position tracking, ASCII fast paths, counted token-slice capacity (#544, #545, #547, #549) #577)
• 0d6f875 perf(lsp): split document once per request, share Lines snapshot, hoist static maps, single-write sendMessage (perf(lsp): stop re-splitting and copying document text per request; hoist hover docs map; single-write sendMessage #564) (perf(lsp): stop re-splitting and copying document text per request; hoist hover docs map; single-write sendMessage (#564) #573)
• 465f2a3 perf(gosqlx,transform,transpiler): release ASTs and use the parser pool in batch helpers (perf(gosqlx): release ASTs and use the parser pool in batch helpers, transform parse helpers and Transpile #560) (perf(gosqlx): release ASTs and use the parser pool in batch helpers, transform parse helpers and Transpile (#560) #571)
• 755f2fe perf(gosqlx): slice source in SplitStatements instead of copying (perf(gosqlx): make SplitStatements slice the source instead of copying every byte through strings.Builder #559) (perf(gosqlx): make SplitStatements slice the source instead of copying every byte through strings.Builder (#559) #569)
• 2f49d2a perf(linter): build default rule set once in gosqlx.Lint; alloc-free L015 scan (perf(linter): build the default rule set once in gosqlx.Lint and drop the full-SQL ToUpper in SelectIntoOutfileRule #562) (perf(linter): build the default rule set once in gosqlx.Lint and drop the full-SQL ToUpper in SelectIntoOutfileRule (#562) #568)
• d1dfe22 fix(security): resolve code-scanning and Dependabot alerts (fix(security): resolve code-scanning and Dependabot alerts #567)
• 5e54542 fix(website): always ship and serve a current playground WASM (fix(website): always ship and serve a current playground WASM #543)
• 50fb716 feat(parser,ast): preserve parenthesized expressions (fix [BUG] GoSQLX does not support parentheses #519) (feat(parser,ast): preserve parenthesized expressions (fix #519) #532)

146 files changed, 13989 insertions(+), 4969 deletions(-)

Test Plan

• Independent validator ran the repository race gate (CGO_ENABLED=1 go test -race -timeout 120s $(go list ./... | grep -v /cbinding)), go vet, and golangci-lint on this branch.
• Each change was reviewed by an independent reviewer with access to the code and benchmarks before validation.
• Benchmarks compared with benchstat -count=6 before/after on the same machine; allocs/op and B/op are the deterministic signal, ns/op is directional on a loaded machine.

#557 benchstat

I ran benchstat on scratchpad/b2/557/before.txt against after.txt (-count=6, every row p=0.002) and confirmed the HEAD numbers with fresh runs.
- Inspect/select_join_window: 47 to 26 allocs/op, 3216 to 928 B/op. A fresh HEAD run gives 26 allocs and 928 B.
- Inspect/insert_multirow: 16 to 1 allocs/op, 2144 to 576 B/op. A fresh run gives 1 alloc and 576 B.
- Fingerprint: 182 to 170 allocs/op, 20.80 to 19.02 KiB (-8.55%). A fresh run gives 170 allocs and 19481 B.
- Normalize: 178 to 166 allocs/op, 20.42 to 18.65 KiB.
- Lint/Tiny: 135 to 99 allocs/op, -32% B/op.
- Lint/Realistic: 728 to 580 allocs/op, 56.78 to 36.58 KiB.
- ExtractMetadataLarge: 587 to 466 allocs/op.
- ExtractTables 11 to 6, ExtractColumns 15 to 9, ExtractFunctions 12 to 9, ExtractMetadata 24 to 17 allocs/op.
- All 24 Scan*/Scanner_* benchmarks have unchanged allocs/op; B/op is unchanged or down by up to 0.56%.
- Transpile stays at 92 allocs/op (a fresh run agrees), B/op -0.01%.
- ns/op is directional only: the baseline was noisy (±13-191%).

Differential check: I extracted the parent commit 74a9567 with git archive and added the new test files. There, TestWalk_VisitOrderGolden and TestWalk_MatchesChildrenRecursion pass, which shows the golden matches pre-change visit order. TestWalk_VisitorMutationReachesAST fails in all 5 subtests (TableReference/OrderBy, Window/Aggregate/WithinGroup/Frame, WhenClause, UpdateExpression, MergeTargetSource). At HEAD all of them pass.

#557 race gate: PASS. Ran CGO_ENABLED=1 go test -race -timeout 120s $(go list ./... | grep -v /cbinding) twice. The first run was served from cache. The second used -count=1 so every test actually ran, and it exited 0: 44 packages ok, 18 with no test files, no FAIL, no DATA RACE, no panic. Logs are in /private/tmp/claude-501/-Users-ajitpratapsingh-dev-GoSQLX/b99cd3cf-cb92-4294-a0db-d6a7c9a86253/scratchpad/b2/557/val/race_nocache.txt.

Breaking Changes

• perf(ast): stop heap-copying nodes in Children() so Walk/Inspect allocate less #557: There are no exported signature or method-set changes. All Children()/TokenLiteral receivers stay as they are, so value JoinClause{} / WindowFrame{} / MergeStatement{} still satisfy ast.Node (interface_test.go:127-133 keeps compiling). Additions are unexported only: childrenOf in visitor.go, and childrenInPlace on *JoinClause, *WindowFrame and *MergeStatement.

The issue's step 2 (pointer receivers) is deliberately declined. It would drop Node from the value method sets, a compile-time break for external callers such as ast.Walk(v, sel.Joins[i]), and the ground rules call for non-breaking options; the in-place fast path in Walk gets the same allocation win.

Observable behaviour change, which is intended and fixes the latent bug the issue describes:

• Children() and Walk/Inspect now yield pointers that alias the AST's stored value nodes (TableReference, JoinClause, OrderByExpression, WindowSpec, WindowFrameBound, WhenClause, UpdateExpression, ColumnDef/ColumnConstraint/TableConstraint, IndexColumn, SetClause, AlterTableAction, PartitionDefinition, Identifier in TriggerEvent).
• Visitor mutations reach the tree, and pointer identity is stable across calls.
• Those pointers share the AST's lifetime (invalid after ReleaseAST).

The CHANGELOG entry (Performance, plus a behaviour note) is to be written during the later consolidation step, not on this branch.

Notes

• CHANGELOG.md and performance_baselines.json are intentionally not touched on this branch; one consolidated follow-up PR will update both for all branches of the perf audit (tracking issue perf: repo-wide optimization audit (2026-10-11) — tracking issue #566).
• Base: perf/ast-pool-release-fixes.

Fixes #557
Part of #566

🤖 Generated with Claude Code

https://claude.ai/code/session_01UZSk1292jPc8ipDboNxeve

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
gosqlx-website	Ready	Preview	Oct 13, 2026 6:53am UTC

[github-actions]

Claude finished @ajitpratap0's task in 24s —— View job

---

Review of PR #588 (perf(ast): stop heap-copying nodes in Children())

I read the non-test diff in pkg/sql/ast. I did not run the tests, benchmarks or race gate here, and I did not read the new test files.

Overall: The change looks correct and well scoped. Replacing x := x; &x with &s.From[i] is safe even for value-receiver Children() methods. The receiver copy shares the slice backing array, so the pointers still alias the real AST storage. childrenOf in visitor.go handles the cases where that doesn't hold, because the struct itself is a field of the receiver. Those cases are JoinClause, WindowFrame and MergeStatement.

• Read the diff for visitor.go, ast.go, ast_clauses.go, ast_expressions.go, dml.go and trigger.go
• Compare the benchstat claims against the code

Observations

1. Direct Children() calls still return copies for some nodes.

   • Examples are Update.Children() returning &u.Table, and JoinClause/WindowFrame called on a value.
   • Walk and Inspect now reach the real nodes through childrenOf, but direct calls don't.
   • The doc comments on JoinClause and WindowFrame say this. The Update.Table case is not documented. If Update is a value receiver, &u.Table still points into a copy. Check that TestWalk_VisitorMutationReachesAST covers Update.Table and not only the Updates slice. If it doesn't, add a note or switch to a childrenInPlace pattern for Update.
   • Walk(v, someJoinClauseValue) also still works on a copy. That is expected.

2. childrenOf is a hand-maintained type switch.

   • A future value-receiver node that takes the address of its own fields would silently regress to the copy behaviour.
   • A small interface such as interface{ childrenInPlace() []Node } would be more extensible. The cost is an interface assertion on every Walk step, which could eat into the gain. Your concrete type switch is the cheaper choice.
   • TestWalk_MatchesChildrenRecursion partly guards this. A test that fails when a value-receiver Children() returns the address of a receiver field not in the switch would close the gap.

3. Lifetime and aliasing is a behaviour change.

   • Retaining nodes collected during a walk past ReleaseAST is now use-after-release for value-typed children too. Before, those children were detached copies.
   • The doc comments in ast.go and visitor.go cover this. A CHANGELOG "Changed" note is planned for the consolidation PR. Please make sure it isn't dropped, since downstream code that collects *TableReference or *OrderByExpression nodes could break.

4. Pre-sizing is inconsistent.

   • FunctionCall, CaseExpression and WindowSpec now pre-size their result slice.
   • Select.Children() and Update.Children() still use make([]Node, 0) followed by nodifyExpressions and appends. They could be pre-sized for the same kind of win, but this isn't blocking.

5. Nit: Update.Children() allocations. &u.Table forces u to escape for a value receiver. This is minor and the same as before.

Tests

The golden-order test, the Walk-versus-Children equivalence test and the mutation regression test are the right safeguards. The golden was generated from the parent commit, which is a good way to prove the visit order is unchanged.

Security: No concerns.

Verdict: Looks good to merge once the Update.Table mutation coverage (point 1) is confirmed.

[ajitpratap0]

Base PR #581 has merged, so this branch was rebased onto main (single commit 87c84da, diff byte-identical to the previous 7b173af) and the PR base switched from perf/ast-pool-release-fixes to main. Race tests for ast/parser/gosqlx re-run green after the rebase.

[github-actions]

Claude finished @ajitpratap0's task in 17s —— View job

---

Review of PR #588: stop heap-copying nodes in Children()

I read the non-test diff in pkg/sql/ast and checked UpdateStatement and the mutation test by hand. I did not run the tests, benchmarks or race gate.

Verdict: Looks good to merge.

Correctness

• &s.From[i] and similar are safe in value-receiver Children() methods. The receiver copy shares the slice backing array, so the pointers still alias the real AST storage.
• childrenOf in visitor.go handles the cases where aliasing would fail, because the struct is itself a field of the receiver: JoinClause, WindowFrame and MergeStatement.
• The exported API is unchanged. Declining pointer receivers keeps Node in the value method sets, which avoids a compile-time break for callers.

Correction to the earlier automated review

• That review asked for Update.Table mutation coverage, but UpdateStatement has no Table field (ast_statements.go:328). It has TableName string, Assignments []UpdateExpression and From []TableReference. Those are all slice-backed, so they alias correctly.
• TestWalk_VisitorMutationReachesAST/UpdateExpression covers Assignments (visitor_mutation_test.go:150).
• The From entries are covered indirectly through the shared TableReference path. A one-line UPDATE ... FROM subtest would make that explicit, but it is not blocking.

Non-blocking suggestions

1. Hand-maintained type switch.
   • A future value-receiver node that takes the address of its own struct field would silently fall back to copy behaviour.
   • TestWalk_MatchesChildrenRecursion only partly guards this, since it compares visit order.
   • A test that checks pointer identity for every node type would close the gap.
2. Pre-sizing is inconsistent.
   • Select.Children() and Update.Children() still start from make([]Node, 0).
   • Pre-sizing them would give a further small allocation win.
3. Lifetime change.
   • Nodes collected during a walk are now use-after-release once ReleaseAST runs, including value-typed children. Previously those children were detached copies.
   • The doc comments cover this. Make sure the planned CHANGELOG "Changed" note in the consolidation PR isn't dropped.

Tests

• The golden visit-order test, the Walk-versus-Children equivalence test and the mutation regression test are the right safeguards.
• Generating the golden from the parent commit is a good way to prove the visit order is unchanged.

Security: No concerns.

[ajitpratap0]

Closing and reopening to trigger CI on the rebased branch against main (the base switch happened after the push, so the main CI workflow never ran for this PR).

[github-actions]

Claude finished @ajitpratap0's task in 18s —— View job

---

Review of PR #588: stop heap-copying nodes in Children()

I read the non-test diff (visitor.go, ast.go, dml.go, trigger.go). I did not run the tests, benchmarks or race gate, and I did not read the new test files.

Verdict: Looks good to merge. One doc/coverage gap is worth closing.

Correctness

• &s.From[i] and similar are safe in value-receiver Children() methods. The receiver copy shares the slice backing array, so the pointers alias the real AST storage.
• childrenOf handles JoinClause, WindowFrame and MergeStatement. These are structs stored as fields, so aliasing needs a pointer receiver. The exported method sets are unchanged.
• The childrenOf type switch is a cheap type-word compare. It adds no allocations.

Correction to the two earlier automated reviews

• The second review said UpdateStatement has no Table field, so the first review's Update.Table concern doesn't apply. That is true for UpdateStatement.
• dml.go has a separate Update type, which does have a Table field. Update.Children() at dml.go:119 still does &u.Table on a value receiver.
  • That pointer targets the receiver copy, not the AST. It is the same as before this PR.
  • Visitor mutations to that node therefore don't reach the tree. This is inconsistent with the new doc comments in ast.go and visitor.go.
  • Updates[i] is fine.
• If Update is used in real trees, add it to childrenOf with a childrenInPlace, or note the exception in the docs. If it is legacy and unused by the parser, a one-line comment on Update.Children() is enough.

Non-blocking suggestions

1. Hand-maintained type switch. A new value-receiver node that takes the address of its own field would silently fall back to copy semantics. A test could use reflection over all Node types to check pointer identity of the children they return. This would catch new cases.
2. Pre-sizing. Select.Children() and Update.Children() still start from make([]Node, 0) and append. Pre-sizing would remove a few more allocations. The other nodes in this PR already do it.
3. Behaviour change. Value-typed children were detached copies before. Now they are use-after-release once ReleaseAST runs. The docs cover this. Keep the planned CHANGELOG "Changed" note in the consolidation PR.

Tests and security

• The golden visit order, the Walk-versus-Children equivalence and the mutation regression are the right safeguards. Generating the golden from the parent commit is a good approach.
• No security concerns.

[ajitpratap0]

Windows CI failure was a line-ending issue: the runner checks testdata/visit_order.golden out with CRLF and the golden comparison was byte-exact. Pushed a test-only commit that normalizes CRLF→LF on read (verified locally against a CRLF-converted copy). The implementation commit is unchanged.

[github-actions]

Claude finished @ajitpratap0's task in 15s —— View job

---

Review of PR #588: stop heap-copying nodes in Children()

I read the non-test diff and checked dml.go by hand. I did not run the tests, benchmarks or race gate, and I did not read the new test files.

Verdict: Looks good to merge. One small doc gap is worth closing.

Correctness

• &s.From[i] and similar are safe in value-receiver Children() methods. The receiver copy shares the slice backing array, so the pointers alias the real AST storage.
• childrenOf covers JoinClause, WindowFrame and MergeStatement. Each is a struct stored as a field, so aliasing needs a pointer receiver. The exported method sets are unchanged.

The Update.Table question from the earlier reviews

• dml.go:104 defines Update with a Table TableReference field. Update.Children() has a value receiver (dml.go:117) and returns &u.Table.
• That pointer targets the receiver copy, not the AST. The behaviour is the same as before the PR. A visitor that mutates this node will not change the tree, which contradicts the new aliasing docs.
• Updates[i] is slice-backed and aliases correctly.
• I found no non-test constructor of Update in pkg/. Only (*Update).SQL() and statementNode() reference it. It looks legacy and unused by the parser, so the impact is low.
• Suggested fix: add a one-line comment on Update.Children() noting that Table is returned by copy. Or add Update to the childrenOf switch with a childrenInPlace method on *Update.

Non-blocking suggestions

1. Hand-maintained type switch. A new value-receiver node that takes the address of its own struct field would silently fall back to copy semantics. A reflection test over the Node types that checks child pointer identity would catch this.
2. Pre-sizing. Select.Children() and Update.Children() still start from make([]Node, 0). Pre-sizing them would remove a few more allocations.
3. Lifetime change. Value-typed children used to be detached copies. They are now use-after-release once ReleaseAST runs. The doc comments cover this. Keep the planned CHANGELOG "Changed" note in the consolidation PR.

Tests and security

• The golden visit-order test, the Walk-versus-Children equivalence test and the mutation regression test are the right safeguards. Generating the golden from the parent commit is a good approach.
• No security concerns.

[github-actions]

Claude finished @ajitpratap0's task in 13s —— View job

---

Review of PR #588: stop heap-copying nodes in Children()

I read the non-test diff and the latest commit (aa8cbea). I did not run the tests, benchmarks or race gate.

Verdict: Looks good to merge.

Correctness

• &s.From[i] and similar are safe in value-receiver Children() methods. The receiver copy shares the slice backing array, so the pointers alias the real AST storage.
• childrenOf (visitor.go:~203-214) now covers JoinClause, WindowFrame, MergeStatement, Insert, Delete and Update. Each of these is a struct stored as a field, or has a value-receiver Children() that takes the address of one of its own fields. Their pointer-receiver childrenInPlace makes Walk and Inspect visit the real nodes.
• aa8cbea closes the Update.Table gap the earlier automated reviews raised. It also covers Insert.Table and Delete.Table.
• The exported method sets are unchanged, so there is no compile-time break for callers.

Tests

• TestWalk_ChildPointersAliasParentStorage fills every listed Node type by reflection and checks that each child pointer lies inside the parent's storage. This also addresses my earlier suggestion for a test that catches future value-receiver nodes returning &field.
• The golden visit-order test, the Walk-versus-Children equivalence test and the mutation regression test remain the right safeguards. Generating the golden from the parent commit is a good approach.

Non-blocking suggestions

1. Pre-sizing. Select.Children() and Update.Children() still start from make([]Node, 0) and append. Pre-sizing them would remove a few more allocations. This can be a follow-up.
2. Lifetime change. Value-typed children used to be detached copies. They are now use-after-release once ReleaseAST runs. The doc comments cover this. Please keep the planned CHANGELOG "Changed" note in the consolidation PR.
3. Direct Children() calls on a value. For JoinClause, WindowFrame, MergeStatement, Insert, Delete and Update, calling Children() on a value still returns pointers into a copy. This is documented, and only Walk and Inspect get in-place semantics.

Security: No concerns.