[ajitpratap0]

Summary

#555 perf(ast): fix release-path defects: double Put via JoinClause.Left, incomplete node reset, unbounded work-queue clearing

Fixes three defects in the AST release path (#555): PutSelectStatement no longer releases through Joins[i].Left (a non-owning value copy of From[0]), which previously Put the derived-table subquery twice and made two later GetSelectStatement calls return the same object; every pooled expression node is now reset by whole-struct assignment (keeping only hot slice capacity) and FunctionCall's Over/Filter/OrderBy/WithinGroup plus the children of Rollup/Cube/GroupingSets/NamedArgument/VariantPath are released instead of dropped; the PutExpression work queue is cleared only to its high-water mark, queues with cap > 1024 are not returned to the pool, and Identifier/LiteralValue take a queue-free fast path. Measured effect (benchstat, count=6): PutLeafAfterBigList 2995ns -> 19ns and PutBinaryAfterBigList 3183ns -> 47ns, both now within 1.1x of the no-big-list case; allocs/op and B/op are unchanged on every benchmark. Validated independently: race gate green across 44 packages, golangci-lint 0 issues, no exported API change, and the new regression tests fail on the pre-fix code and pass on this branch. CHANGELOG.md and performance_baselines.json are intentionally left for the consolidated follow-up PR.

#556 perf(ast): stop ReleaseAST Putting node types the parser never draws from (or make the parser draw them)

ReleaseAST/ReleaseStatements now thread an unexported releaseMode through the unchanged release walk and Put only the node types the parser actually draws (Tuple/ArrayConstructor/ArraySubscript/ArraySlice; Unsupported/Alter/Show/Describe/Replace/sequence statements), leaving Identifier/LiteralValue/BinaryExpression/FunctionCall and Select/Insert/Update/Delete to the GC; the exported Put* functions keep full releaseAll semantics and the exported API is byte-identical (542 go doc func lines). This is the issue's recommended option (b); option (a), having the parser draw hot node types from pools, remains a possible follow-up now that #555 made the pools safe. Independent validation ran the repository race gate (44 packages, 0 failures), vet/lint clean, and a 221-input corpus contamination test (176 parsed, 4,408 sequential + 3,200 concurrent parse -> ReleaseAST -> parse pairs compared against untouched-pool reference dumps, plus 10k+ drawn-pool Gets with no double Put), all passing with and without -race. A drawn-only policy test and the new repo policy tests pass on HEAD and fail on the parent commit 3facfca. DDL statements and the query inside CREATE VIEW / CREATE MATERIALIZED VIEW (and MERGE) are still fully recycled by their Put* functions, as the docs state.

Commits (3), one per issue:

• cd2e77f perf(ast): address review for perf(ast): stop ReleaseAST Putting node types the parser never draws from (or make the parser draw them) #556: precise release-policy docs, stronger double-Put test
• 2697e42 perf(ast): stop ReleaseAST putting node types the parser never draws from (perf(ast): stop ReleaseAST Putting node types the parser never draws from (or make the parser draw them) #556)
• 3facfca perf(ast): fix double Put, stale pooled fields and work-queue pinning (perf(ast): fix release-path defects: double Put via JoinClause.Left, incomplete node reset, unbounded work-queue clearing #555)

9 files changed, 1638 insertions(+), 343 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.

#555 benchstat

benchstat before.txt after.txt (count=6, Apple M4, load avg 13-20 during runs):
sec/op: PutLeafAfterBigList 2995.00n -> 19.10n (-99.36%, p=0.002); PutBinaryAfterBigList 3183.00n -> 47.33n (-98.51%, p=0.002); PutBinary 183.88n -> 44.46n (-75.82%); PutExpression/PutIdentifier 39.90n -> 23.46n (-41.21%); PutLeaf 46.34n -> 25.65n (~, p=0.065). PutLeafAfterBigList/PutLeaf = 0.74x and PutBinaryAfterBigList/PutBinary = 1.06x (criterion: within 2x). Apparent slowdowns in the noisy run (ASTPool +171%, IdentifierPool +93%, BinaryExpressionPool +76%, Parallel +80%) do not reproduce in the interleaved A/B run (ab_old.txt vs ab_new.txt, n=8): all ~ with p>0.19 and new medians lower; PutExpression/PutIdentifier -82.5% and PutLeaf -72% there.
B/op: unchanged on every benchmark (PutLeaf, PutLeafAfterBigList, PutBinary, PutBinaryAfterBigList, ASTPool, IdentifierPool, PutExpression/* all 0 B; SelectStatementPool 317->327.5 B ~ p=0.331; BinaryExpressionPool 148.5->146.5 ~; Parallel 316.5->317.5 ~).
allocs/op: identical on every benchmark (0 for all Put*/ASTPool/IdentifierPool/PutExpression; 4 for SelectStatementPool and Parallel; 2 for BinaryExpressionPool).
Out-of-tree repro (scratchpad/fix/555/repro, replace -> worktree): TestDoublePut PASS (Get#0/#1/#2 distinct), TestStaleState PASS, TestBigListSlowdown ratio=1.0x (15ns vs 15ns).
Parser TestPerformanceRegression (plan sanity item, not an acceptance criterion): FAILs under load 13-15 but readings swing 2x between consecutive runs (SimpleSelect 1279/692/1180 ns; INSERT -22.8%/+42%/+38.7%); the test calls no Put*/Release*/pool function and the diff changes only release paths, so this is machine noise, not a regression from this branch.

#555 race gate: cd /Users/ajitpratapsingh/dev/GoSQLX-worktrees/perf-ast-pool-release-fixes && CGO_ENABLED=1 go test -count=1 -race -timeout 120s $(go list ./... | grep -v /cbinding) -> exit 0; 44 packages ok, 0 FAIL, no DATA RACE/panic lines (output saved to scratchpad/fix/555/validator_race2.txt). Targeted -race -v run of the new tests also green: TestReleaseAST_DerivedTableJoin_NoDoublePut, TestPutSelectStatement_HandBuilt_LeftAliasFrom, TestPutExpression_ResetsEveryField_Reflective, TestPutExpression_FunctionCall_ReleasesOverFilterOrderBy, TestPutExpression_UnpooledParents_ReleaseChildren, TestPutExpressi
#556 benchstat

benchstat (implementer n=6 and validator 3facfca-vs-HEAD n=4 agree):
allocs/op: identical in every row (ParseRelease SampleJoin 25/23, MediumCTE 123/121, DrawnTypes 38/27; ReleaseASTOnly 0; ParserComplexity 34/217/2020/25/15; exported Put* benchmarks unchanged).
B/op: ReleaseASTOnly SampleJoin 243 -> 24 (-90.1%), MediumCTE 1399 -> 39 (-97.2%), DrawnTypes 547 -> 207 (-62.2%); ParseRelease/Release SampleJoin 3.633Ki -> 3.457Ki (-4.8%), MediumCTE 17.04Ki -> 15.60Ki (-8.5%), DrawnTypes 4.607Ki -> 4.270Ki (-7.3%); NoRelease rows and ParserComplexity identical.
ns/op: ReleaseASTOnly -47.9% / -45.7% / -33.5%; ParseRelease/Release -14.3% / -17.6% / -15.0%; untouched paths within noise.
Race gate: CGO_ENABLED=1 go test -race -timeout 120s $(go list ./... | grep -v /cbinding) -> 44 packages ok, 0 FAIL, 0 DATA RACE. go vet clean, golangci-lint 0 issues.

#556 race gate: full race gate passed (see notes)

Breaking Changes

• perf(ast): fix release-path defects: double Put via JoinClause.Left, incomplete node reset, unbounded work-queue clearing #555: none — no exported signature, type, or constant changes. Observable behaviour changes are all tightenings toward the documented contract: (a) pooled nodes returned by Get* are now fully zero (previously Not/CustomOp/Pos/Table/Try/NullTreatment/OrderBy/WithinGroup/Parameters could be stale); (b) typed PutX wrappers now release nested subtrees (FunctionCall.Over/Filter/OrderBy/WithinGroup) instead of dropping them; (c) PutExpression now releases pooled children reachable through RollupExpression, CubeExpression, GroupingSetsExpression, NamedArgument and VariantPath (parents remain GC-owned, no new pools); (d) PutSelectStatement no longer releases through Joins[i].Left (documented as a non-owning copy) and zeroes the whole JoinClause including Pos; (e) Select/Insert/Update/Delete Pos is reset on Put; (f) AST.Comments elements are cleared on ReleaseAST; (g) work queues with cap > 1024 are no longer returned to the pool (unexported constant maxPooledWorkQueueCap). No breaking changes; no CHANGELOG edit per ground rules (note the above in the PR description for the consolidated changelog PR).
• perf(ast): stop ReleaseAST Putting node types the parser never draws from (or make the parser draw them) #556: No exported signature changes and no removals. ReleaseAST and ReleaseStatements change observable pool behaviour only: they continue to return the AST container and every node type the parser draws from pools (TupleExpression, ArrayConstructorExpression, ArraySubscriptExpression, ArraySliceExpression, UnsupportedStatement, AlterStatement, ShowStatement, DescribeStatement, ReplaceStatement, sequence statements, plus DDL statements via their unchanged Put* functions), but no longer Put Identifier, LiteralValue, BinaryExpression, FunctionCall, AliasedExpression, Case/In/Between/Cast/Subquery/Unary/Exists/Any/All/List/Extract/Position/Substring/Interval expressions or Select/Insert/Update/Delete statements; those become garbage. All exported Get*/Put* functions (PutExpression, PutIdentifier, PutSelectStatement, ...) keep full semantics and remain the documented way to recycle hand-built trees. Handled by godoc updates on ReleaseAST, ReleaseStatements, PutExpression and doc.go, and by tests that pin both contracts. Not a breaking change: nothing stops compiling, no leaks, no double Put; only pool hit rates for hand-built undrawn nodes released through ReleaseAST drop, which the issue explicitly accepts. Internal additions: unexported releaseMode type, releaseExpression, releaseSelect/Insert/Update/DeleteStatement, and a mode parameter on putExpressionImpl, releaseStatement, releaseTableReference, releaseTimeTravelClause.

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: main.

Fixes #555
Fixes #556
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 11, 2026 6:41pm UTC

[github-actions]

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

---

Review of PR #581 (AST pool release fixes, #555 and #556)

I read the pool.go, pool_expression_release.go and pool_statement_release.go diffs. I did not run the tests, benchmarks or race gate. The sandbox has no merge base for origin/main, so I diffed against HEAD~3. I also did not read the test files closely.

Overall: The design is sound and the invariants are well documented. I found no blocking issues.

What looks good

• Double Put fix: Not releasing through JoinClause.Left is correct, because Left is a non-owning copy of From[0]. The doc comment added in ast_clauses.go records that contract.
• Whole-struct reset: *e = T{...}, keeping only hot slice capacity, removes the stale-field class of bugs. Clearing the kept slices first with clear(args) also avoids pinning pointers.
• High-water-mark clearing:
  • hw is updated at the end of each loop iteration, after all appends. Pops only happen at the start of an iteration, so hw is the true maximum length.
  • The deferred clear(workQueue[:hw]) runs after the overflow drain, so it covers everything.
  • Dropping queues with cap > 1024 fixes the pinning problem.
• releaseMode: Sharing a single walk between releaseAll and releaseDrawn keeps nested drawn nodes reachable under un-drawn parents. This is better than maintaining two walkers.

Suggestions and risks (non-blocking)

1. Drawn-type list can drift from the parser.
   • The releaseDrawn list (Tuple, ArrayConstructor, ArraySubscript, ArraySlice, and the Unsupported/Alter/Show/Describe/Replace/sequence statements) is hand-maintained. The comment says it must stay in sync with the parser's Get* calls.
   • If someone makes the parser draw a new type and forgets this switch, the result is silent pool growth or loss, not a failure.
   • I'd add a test that scans pkg/sql/parser for ast.Get* and ast.New*Statement calls and compares them against the policy list. If pool_release_policy_test.go already does this, ignore this point.
2. Latent double Put in releaseDrawn.
   • The hazard is documented in the putExpressionImpl comment. If a shared parent such as a named window or a reused expression is reachable twice, its drawn children are Put twice. Un-drawn parents are not reset, so nothing marks them as visited.
   • It is safe today. A cheap guard would be to have the policy test parse a corpus and assert that no drawn node is reachable twice. The PR mentions a 221-input corpus test, so this may already be covered.
3. Overflow drain with MaxCleanupDepth. At depth ≥ 100 the code accepts the leak and only bumps poolLeakCount. That is fine, but the overflow path now clears and returns queues differently, so confirm a test exercises it with more than MaxWorkQueueSize nodes in both modes.
4. Observable behaviour change for hand-built trees. After ReleaseAST, Identifier, BinaryExpression and Select nodes are no longer recycled. This is documented, but anyone who built trees with GetBinaryExpression() and relied on ReleaseAST to recycle them will lose pool hits. The doc.go note helps. A one-line mention in the consolidated CHANGELOG PR would help more.
5. Benchmark claims. The ns/op numbers come from a loaded machine. B/op and allocs/op are the reliable signal, and they match the claim of no allocation regressions. Given the noise, I would not update performance_baselines.json from the ns/op figures.

Process notes

• The PR body says CHANGELOG.md is intentionally deferred to a follow-up. That is fine, but please make sure that PR lands before the next release.
• The PR body truncates the perf(ast): fix release-path defects: double Put via JoinClause.Left, incomplete node reset, unbounded work-queue clearing #555 race-gate line ("TestPutExpressi…") and says "see notes" for the perf(ast): stop ReleaseAST Putting node types the parser never draws from (or make the parser draw them) #556 gate. Both are cosmetic.

Verdict: Approve-worthy once CI is green, including task test:race. Items 1 and 3 are the two I'd double-check.
· branch perf/ast-pool-release-fixes

[github-actions]

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

---

Review of PR #581 (AST pool release fixes, #555 and #556)

I read the test file list and the earlier diff review. I did not run the tests, benchmarks or race gate in this pass.

Update since the previous review: the latest commit (ec610ed) adds the two tests I asked for.

• Parser drift (previous item 1): TestReleaseDrawnPolicy_MatchesParser and TestReleaseDrawnPolicy_EveryListedTypeIsReleased in pool_drawn_policy_test.go now guard the drawn-type list against the parser.
• Overflow paths (previous item 3): pool_release_overflow_test.go adds WorkQueueBudget, DrawnNodesResetAcrossOverflow, DepthLimit and NestedSubqueries.

Overall: The design is sound, the invariants are documented, and I found no blocking issues. Both of my earlier concerns are covered by tests.

Remaining non-blocking notes

1. Double Put under releaseDrawn: TestReleaseAST_DrawnPoolNoContamination covers this for the corpus. A hand-built tree that shares a drawn child under two un-drawn parents would still double Put it. The code comment documents this. Consider one explicit test that pins the behavior.
2. Behavior change for hand-built trees: ReleaseAST no longer recycles Identifier, BinaryExpression or Select nodes. Please put this in the consolidated CHANGELOG PR under the breaking-changes or behavior-change heading, and land that PR before the next release.
3. Benchmarks: ns/op was measured on a loaded machine. B/op and allocs/op are the reliable signal. I would not update performance_baselines.json from the ns/op figures.
4. PR description: the perf(ast): fix release-path defects: double Put via JoinClause.Left, incomplete node reset, unbounded work-queue clearing #555 race-gate line is truncated, and the perf(ast): stop ReleaseAST Putting node types the parser never draws from (or make the parser draw them) #556 gate says "see notes". Both are cosmetic.

Verdict: Approve-worthy once CI is green, including task test:race.
· branch perf/ast-pool-release-fixes

[github-actions]

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

---

Review of PR #581 (AST pool release fixes, #555 and #556)

I did not run the tests, benchmarks or race gate in this pass. The sandbox denied go test. This is based on the earlier diff reviews and the changed-file list.

Overall: The design is sound and the invariants are documented. I found no blocking issues. The parser-drift and overflow tests from ec610ed cover the two main concerns from the earlier reviews.

What looks good

• PutSelectStatement no longer releases through Joins[i].Left, which is a non-owning copy of From[0]. This fixes the double Put.
• Whole-struct reset (*e = T{...}, keeping only hot slice capacity) removes the stale-field class of bugs.
• The work queue is cleared only to its high-water mark, and queues with cap > 1024 are not returned to the pool. This fixes the pinning problem.
• releaseMode shares a single walk between releaseAll and releaseDrawn, so nested drawn nodes stay reachable under un-drawn parents.
• The exported API is unchanged.

Non-blocking suggestions

1. Shared drawn child in a hand-built tree. releaseDrawn does not reset un-drawn parents, so a drawn child shared under two such parents would be Put twice. The code comment documents this. One explicit test would pin the behaviour.
2. Behaviour change for hand-built trees. ReleaseAST no longer recycles Identifier, BinaryExpression, Select and similar nodes. Please list this under behaviour changes in the consolidated CHANGELOG PR, and land that PR before the next release.
3. Benchmarks. The ns/op figures come from a loaded machine. B/op and allocs/op are the reliable signal. I would not update performance_baselines.json from the ns/op numbers.
4. PR description. The perf(ast): fix release-path defects: double Put via JoinClause.Left, incomplete node reset, unbounded work-queue clearing #555 race-gate line is truncated, and the perf(ast): stop ReleaseAST Putting node types the parser never draws from (or make the parser draw them) #556 gate says "see notes". Both are cosmetic.

Verdict: Approve-worthy once CI is green, including task test:race.
· branch perf/ast-pool-release-fixes

[github-actions]

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

---

Re-review of PR #581 after 74a9567

I reviewed the latest commit's diff only. I did not run tests, benchmarks or the race gate.

Latest commit (74a9567)

• It moves releaseDrawnConstructors from pool_expression_release.go into pool_drawn_policy_test.go. The variable's only consumer is the test, so keeping it out of production code is a good change.
• The releaseDrawn doc comment and the test failure message now point at the new location.
• The list contents are unchanged: 9 Get* constructors and 3 New*SequenceStatement constructors.
• Trade-off: the drift guard is now less visible to someone editing putExpressionImpl or releaseStatement. The releaseDrawn constant's doc comment mentions the test file, which mostly covers this.

Nits

1. In pool_drawn_policy_test.go, the new comment's line "...use: TestReleaseDrawnPolicy_MatchesParser compares ..." is much longer than the other lines. Please re-wrap it.
2. The commit also changes .claude/settings.json by swapping the qodo-skills plugin for qodo and reordering. It has nothing to do with this PR. Drop it, or confirm it is intended.
3. The "Notes" in the PR description say CHANGELOG.md and performance_baselines.json are untouched. That is fine. Please list the behaviour change for hand-built trees in the consolidated PR: ReleaseAST no longer recycles Identifier, BinaryExpression, Select and similar nodes.

Carried over from earlier reviews (non-blocking)

• A hand-built tree that shares a drawn child under two un-drawn parents would be Put twice in releaseDrawn mode. The code documents this. One explicit test would pin the behaviour.
• Use B/op and allocs/op for the benchmark claims. Do not update performance_baselines.json from the loaded-machine ns/op numbers.

Verdict: No blocking issues. Approve-worthy once CI is green, including task test:race.
· branch perf/ast-pool-release-fixes