[ajitpratap0]

Summary

#614 tokenizer: pooled Tokenizer keeps lineStarts capacity of the largest input it has seen

Tokenizer.Reset now keeps the lineStarts backing array only while 0 < cap <= maxRetainedLineStarts (8192 entries, 64 KiB). A larger one is replaced with a fresh 16-entry slice, matching the existing litBuf and Comments retention caps, so pooled tokenizers no longer hold about 72 MB after an input with millions of lines (#614). On the issue's 8M-newline input, lineStarts capacity after Reset drops from 9038848 to 16. Small slices are still reused, and token positions after a drop match a fresh tokenizer. The new tests fail on the parent commit and pass with the fix. The full race gate, go vet and golangci-lint are clean. An interleaved benchstat (n=8) shows no ns/op change. The only cost is +2.4% B/op (+4 allocs) on the 1 MB benchmark, because inputs above about 410 KB now reallocate lineStarts per call. Out of scope by design: changing load()'s len/50+1 pre-size heuristic, capping only in PutTokenizer, and caps for other buffers (already nil'd or capped). Issue #615 (error-string size) lands as a separate commit on this branch.

#615 errors: error string grows with the error column on long single-line input (full source line plus caret padding)

Fixes #615. The SQL context in error messages used to grow with the input: a 100 KB single-line query produced a 205 KB E1002 string, and a 1 MiB query about 1 MB. (*Error).Error(), FormatMultiLineContext and FormatContextWindow now share one unexported renderer (pkg/errors/context_window.go). It finds lines with a bounded scan instead of strings.Split over the whole SQL. Lines over 120 bytes, neighbour lines included, are clipped to about 60 bytes either side of the error column on UTF-8 boundaries, marked with '...'. Caret padding and highlight length are capped. A follow-up commit bounds the rune-boundary scans so invalid UTF-8 cannot panic.

Results:

• The issue repros now give 405 / 413 / 414 bytes, down from 205,129 / 102,714 / 1,048,891.
• Output for lines of 120 bytes or less is unchanged. Golden tests captured from the old code check this, and a 200k-case differential run against the old code was byte-identical.
• The race gate, vet and lint pass.
• benchstat shows the long-line Error() going from 49 us / 937 KiB to 1.4 us / 1.3 KiB per op, and the existing context-format benchmarks are about 20% faster with fewer allocations.

Deferred, with sound reasons:

• parser: several E2002 errors pass hint text as the SQL source, printing a bogus context snippet #616: E2002 uses the hint text as Context.SQL, so the caret points into the wrong text. Here only its size is bounded.
• Rune- and tab-aware caret columns: an existing byte-column behaviour, unrelated to size.
• Bounding Message and Hint length: the issue is about context rendering.

Commits (3), one implementation commit per issue plus test-only follow-ups:

• 80dbc72 fix(errors): address review for errors: error string grows with the error column on long single-line input (full source line plus caret padding) #615
• 238905e fix(errors): bound SQL context window in error messages for long lines (errors: error string grows with the error column on long single-line input (full source line plus caret padding) #615)
• 80aaf7f fix(tokenizer): drop oversized lineStarts on Reset so pooled tokenizers do not retain large inputs (tokenizer: pooled Tokenizer keeps lineStarts capacity of the largest input it has seen #614)

7 files changed, 838 insertions(+), 167 deletions(-)

Test Plan

• Each issue's reproduction cases were run against main before the fix (Haiku scout), and a failing regression test was written before the code change.
• Independent review (Opus, or Sonnet on small chains) with a bounded fix loop.
• Independent Opus validator ran the repository race gate (CGO_ENABLED=1 go test -race -timeout 300s $(go list ./... | grep -v /cbinding)), go vet, golangci-lint, re-ran the reproduction cases on HEAD, and confirmed the new tests fail on the parent commit.
• Perf items: benchstat -count=6 before/after on the same machine.

#614 evidence (validator)

The implementer's before.txt/after.txt (taken at load average about 28) reported ns/op swings of +45-450%. B/op and allocs/op in those files show these are load noise, and GetPut runs the same branch at cap 16. I re-ran the benchmarks interleaved: the parent 61ca6c8 test binary and the HEAD 80aaf7f binary alternated 8 times at 300ms benchtime, benchstat n=8. Every sec/op row was ~ except TokenizeString/SimpleSQL/String at +1.65% (p=0.003). That input has no newlines, so its lineStarts path is unchanged, and I treat it as noise. Geomean sec/op +1.04%; GetPut 21.07n -> 20.61n (~); VeryLargeSQL_1MB 8.988m -> 9.033m (~). B/op changed only for VeryLargeSQL_1MB, +2.38% (24.62Mi -> 25.20Mi, p=0.000), and allocs/op only there too, 6125 -> 6129. This is the expected cost: load() pre-sizes lineStarts to len/50+1, which exceeds 8192 entries for inputs above about 410 KB, so that slice is now reallocated per call instead of retained. All other B/op and allocs/op are identical. Files: scratchpad/b3/614/v_before.txt, v_after.txt, v_benchstat.txt. Repro on HEAD (scout program): 8M-newline input gives cap=9038848 (72.3 MB) after Tokenize and cap=16 after Reset; the 1 MB single-line input gives cap=20001, then 16 after Reset.

#614 race gate: PASS. CGO_ENABLED=1 go test -race -timeout 300s $(go list ./... | grep -v /cbinding) exited 0 with 44 packages ok and no FAIL lines. Output is at /private/tmp/claude-501/-Users-ajitpratapsingh-dev-GoSQLX/b99cd3cf-cb92-4294-a0db-d6a7c9a86253/scratchpad/b3/614/val_race.txt. No re-run was needed.
#615 evidence (validator)

This is an independent re-run, count=6, Apple M4. Before is parent 80aaf7f with the new benchmark file plus a stub; after is HEAD 80dbc72.
- ErrorString_LongLine: 49.2us -> 1.36us (-97.2%); 937 KiB/op -> 1.30 KiB/op (-99.9%); 21 -> 14 allocs.
- ErrorString_Short: 377 -> 289 ns (-23.5%); 728 -> 736 B (+1.1%, +8 B); 14 -> 13 allocs.
- FormatErrorWithContext: 390 -> 291 ns (~, p=0.065); 792 -> 784 B; 14 -> 13 allocs.
- FormatMultiLineContext: 381 -> 291 ns (-23.4%); 400 -> 312 B (-22%); 13 -> 10 allocs.
- FormatContextWindow: 564 -> 453 ns (-19.7%); 840 -> 688 B (-18%); 18 -> 13 allocs.
- No regression beyond noise.

Repro on HEAD through gosqlx.Parse, err.Error() length before -> after:
- 100 KB E1002: 205129 -> 405
- 100 KB E2002: 102714 -> 413
- 1 MiB E2002: 1048891 -> 414
- case 4: 311 -> 311 (unchanged)
All outputs are valid UTF-8, and the caret sits under the opening quote.

#615 race gate: PASS. I ran CGO_ENABLED=1 go test -race -timeout 300s $(go list ./... | grep -v /cbinding) on HEAD 80dbc72: exit 0, 44 packages ok, no FAIL lines, no flakes (log: scratchpad/b3/615/race.txt). go test -short on pkg/errors and pkg/gosqlx also passes.

Breaking Changes

• tokenizer: pooled Tokenizer keeps lineStarts capacity of the largest input it has seen #614: None. maxRetainedLineStarts is an unexported constant. The observable behaviour changes only in memory retention: Reset/PutTokenizer no longer keep a lineStarts backing array larger than 8192 entries. Token output, positions and errors are unchanged.
• errors: error string grows with the error column on long single-line input (full source line plus caret padding) #615: No exported signatures change. Observable behaviour changes in Error(), FormatMultiLineContext and FormatContextWindow:
• Source lines longer than 120 bytes are clipped to about 60 bytes either side of the error column, with '...' markers.
• Caret padding is capped at the displayed text (or at 120 for verbatim lines).
• The highlight is capped at 120 carets.
  Lines of 120 bytes or less render byte-identically. All new helpers and constants are unexported. The change is documented in the three functions' doc comments.

Notes

• CHANGELOG.md is intentionally not touched; the consolidated CHANGELOG PR docs: consolidate CHANGELOG for the perf audit and batch-2/batch-3 fixes; refresh performance baselines (#566) #598 gets the entries for this batch before it merges.
• Part of the batch-3 merge train (follow-ups cli: gosqlx format silently corrupts many expressions; switch the CLI to pkg/formatter #599-schema/postgres: loader misreports index columns and key flags (column order, INCLUDE, expression and partitioned indexes, read-only roles) #620); later PRs in the train are rebased onto this one after it merges.
• Base: main.

Fixes #614
Fixes #615
Follow-up to #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 14, 2026 6:07am UTC

[github-actions]

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

---

Review of PR #626 (#614 lineStarts retention, #615 bounded error context)

I read the diff for pool.go, errors.go, formatter.go and context_window.go. I did not read the new test files or run the test suite, so test quality and results below rely on the PR description.

Verdict: looks good. I found no blocking issues.

#614 Tokenizer.Reset

• The 0 < cap <= maxRetainedLineStarts check mirrors the existing litBuf and Comments caps. The append(t.lineStarts[:0], 0) form is correct and tidier than the old branch.
• The nil-slice case (cap == 0) goes through the else branch, so initialization is preserved.
• The 8192-entry cap against load()'s len/50+1 pre-size means inputs above about 410 KB reallocate lineStarts on every call. The +2.4% B/op on the 1 MB benchmark matches that, and the PR already calls it out. Nothing to change. A short comment next to the constant noting the trade-off would help the next reader.

#615 renderContext

• Equivalence with the old code. The Error() path keeps the old "no trailing newline after the last context line" behaviour through the TrimSuffix. The other two functions keep it too. Line-number width is now derived from endLine, which matches the old maxLineNum logic for 1/1 context.
• Bounds checks.
  • loc.Line > len(sql)+1 plus errIdx >= len(lines) correctly rejects out-of-range lines.
  • linesAfter is clamped, and lineRange stops scanning after the last requested line.
  • The rune-boundary scans are bounded by UTFMax-1, so invalid UTF-8 cannot loop or produce start > end.
• Behaviour changes for short lines. The verbatim caret pad is now capped at 120. A column far past the end of a short line used to get a long run of spaces, and it no longer does. The PR documents this, and I think it's the right call.
• Highlight cap. The highlight is clamped to 120 carets, so a long string-literal span is truncated visually. That is acceptable.

Nits (non-blocking)

1. clipLine and caretPad count bytes, not display columns. A multibyte or tab-containing prefix will misalign the caret. The PR notes this was already the case, so it is fine as deferred work.
2. fmt.Fprintf(sb, " %*d | %s\n", ...) per line is fine. Since this is a hot-ish path for error creation, strconv/WriteString would shave a little more, but the benchmarks already show roughly 20% improvement.
3. contextMaxLine = 2 * contextRadius makes a line of exactly 120 bytes verbatim, while a 121-byte line gets clipped to at most 120 bytes plus ellipses. That is fine, but a boundary test at 120/121 bytes, if it isn't already there, would pin it down.
4. lineRange's doc comment says it is "equivalent to strings.Split(...)[first-1:min(last,n)]". It also returns nil when first is past the last line, which the comment does mention. Good.

Tests, security and performance

• The PR describes golden tests captured from the old code, a differential run, fail-before/pass-after regression tests, and a race gate. That is strong coverage, though I haven't checked it myself.
• This closes an unbounded-output vector, since error strings are often logged or returned to clients. That is a security and robustness improvement. Message and Hint are still unbounded, as noted in the PR's deferred list.

Approve from my side, modulo CI.
· branch fix/tokenizer-linestarts-error-size

[github-actions]

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

---

Review of PR #626 (#614 lineStarts retention, #615 bounded error context)

I read pkg/errors/context_window.go and the pool.go change. I did not run the test suite. I also couldn't read the full diff, because the shallow checkout has no merge base with origin/main. So I did not review errors.go, formatter.go or the new test files, and I rely on the PR's reported results for them.

Verdict: looks good. I found no blocking issues.

#614: pool.go

• The 0 < cap <= maxRetainedLineStarts guard is correct. It matches the existing litBuf and Comments caps, and the make([]int, 1, 16) fallback keeps the lineStarts[0] = 0 invariant.
• The 64 KiB comment is accurate for 64-bit platforms. The comment already says "on 64-bit platforms".
• The +2.4% B/op on 1 MB inputs is a reasonable trade, and you document it.

#615: context_window.go

• lineRange stops scanning at the last requested line and caps its allocation hint, so it no longer splits the whole SQL string.
• clipLine bounds its rune-boundary scans to UTFMax-1 steps. That prevents the invalid-UTF-8 panic, and start <= end is preserved.
• Output is bounded. Caret padding is capped at the displayed text, the highlight is capped at 120 carets, and lines of 120 bytes or less render as before.

Minor, non-blocking notes

1. Neighbour lines clip around the error line's column. renderContext passes the same off to every line's clipLine. A long neighbour line is clipped at the error column rather than its own start or end. This is bounded and harmless, but it can show an arbitrary middle slice. Clipping neighbours from the start (off = 0) would read more naturally. If this is deliberate, a one-line comment saying so would help.
2. Invalid UTF-8 can leave a mid-rune cut. With more than 3 consecutive continuation bytes, the bounded snap leaves a cut that is not on a rune boundary. This is the tradeoff of the panic fix, and the output is still bounded. It does mean "valid UTF-8 output" only holds for valid input. Running the output through strings.ToValidUTF8 would give a hard guarantee, if you want one.
3. The loc.Line > len(sql)+1 check is loose. It is only a safety bound, and lineRange and errIdx handle the real check. You could replace it with a cheaper strings.Count bound, or just leave it.
4. Tests. The golden tests captured from the old code, the differential run and the retention tests are good coverage. I would also add a test that a \r\n input keeps \r in the displayed line, so the new renderer matches the old Split("\n") behaviour.

Nothing here should hold up the merge.

--- · branch fix/tokenizer-linestarts-error-size

[ajitpratap0]

Merge train (batch 3): rebased onto main after #625 (2cbce4d). The adversarial + security pass (Opus) found no must-fix items. It raised two should-fix edge cases in the new long-line context window, both now fixed in 5830e12:

• Tab columns: columns count each tab as 4, so with many tabs before the error the clipped window could miss the error site. The column is now mapped to a byte offset before clipping.
• Rune-boundary snap: the snap could move the window start past an invalid first byte, hiding the very byte E1001 reports.

Tests that fail before the fix and pass after were added, plus a FuzzClipLine target (60s / 10M inputs clean). A 25M-input differential fuzz on tab-free lines showed no other output change. An earlier staticcheck S1019 lint failure was fixed in 6aed561.

[github-actions]

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

---

Review of PR #626 (#614 lineStarts retention, #615 bounded error context)

I read pkg/errors/context_window.go in full. Earlier reviews covered pool.go, errors.go and formatter.go. I did not run the tests or read the new test files. Test results below rely on the PR description.

Verdict: looks good. I found no blocking issues.

#614 Tokenizer.Reset

• The 0 < cap <= maxRetainedLineStarts guard matches the litBuf and Comments caps. The fallback to a fresh 16-entry slice keeps lineStarts[0] = 0.
• Inputs above about 410 KB now reallocate lineStarts on every call, which accounts for the +2.4% B/op on the 1 MB benchmark. That is a reasonable trade, and the PR documents it.

#615 renderContext

• Bounds. lineRange stops after the last requested line, and its capacity hint is capped. clipLine limits its rune-boundary snaps to UTFMax-1 steps, so start <= end holds on invalid UTF-8. start never moves past the error byte.
• Output size. The window is about 120 bytes plus ellipses. Caret padding is capped at the displayed width, and the highlight is capped at 120 carets.
• Tab handling. columnToByte and displayWidth mirror the tokenizer's rule, where a tab counts as 4 columns. The caret should therefore line up on clipped lines that contain tabs.

Non-blocking suggestions

1. Neighbour lines clip at the error line's offset. Every line goes through clipLine(line, b) with the same b (line 253). A long neighbour line therefore shows an arbitrary middle slice. Clipping non-error lines from offset 0 would read more naturally. If the current behaviour is deliberate, add a one-line comment saying so.
2. Verbatim carets are not clamped to the line. For verbatim lines, caretPad returns min(col, 120) even when the line is shorter. This matches the old behaviour, now capped, so it is fine. A comment would make that explicit.
3. Invalid UTF-8 can leave a mid-rune cut. After more than 3 consecutive continuation bytes, the bounded snap leaves a cut that is not on a rune boundary. Output is still bounded and nothing panics. The PR text says "all outputs are valid UTF-8", which holds only for valid input. strings.ToValidUTF8 would give a hard guarantee if you want one.
4. Cheaper formatting. loc.Line > len(sql)+1 (line 219) is only a loose safety bound, and errIdx does the real check. fmt.Fprintf per line could become strconv/WriteString. The benchmarks already show about 20% improvement, so this is optional.
5. Boundary and \r\n tests. I'd add tests, if they aren't already there, for:
   • lines of exactly 120 and 121 bytes;
   • \r\n input keeping \r in the displayed line, to match the old Split("\n") behaviour;
   • a long neighbour line.
6. Comment on maxRetainedLineStarts. A note on the trade-off against load()'s len/50+1 pre-size would help the next reader.

Nothing here should hold up the merge, assuming CI passes.
· branch fix/tokenizer-linestarts-error-size