perf(gosqlx): build wrapped error messages lazily [W1-03] by ajitpratap0 · Pull Request #905 · ajitpratap0/GoSQLX

ajitpratap0 · 2026-10-31T19:57:57Z

Summary

Wave-1 task W1-03 (MEM-3) from the 2026-11 performance audit, including the review change from CRITIQUE item 14. Codex ported the change, and Opus reviewed it and ran the full gate.

pkg/gosqlx had 21 call sites using fmt.Errorf("%w: %w", sentinel, err). Each one built the full message on every error, even though many callers only check the error with errors.Is. They now return an unexported wrapErr:

Storage: both errors live in a [2]error array.
Error(): builds the same "sentinel: inner" text on demand.
Unwrap() []error: returns a slice of that array, so errors.Is and errors.As walk it without allocating. Same contract as the two-%w fmt.Errorf.

Call sites whose message adds extra context text keep fmt.Errorf as before.

Wave 0 is now on main; this branch has merged origin/main, the W0-02 goldens pass, and our private BenchmarkParseError/BenchmarkParseSuccess file was dropped in favour of the wave-0 BenchmarkParseError/* matrix. A CHANGELOG [Unreleased] entry was added.

Test Plan

Compatibility suite (wrap_compat_test.go, wrap_test.go): 242 distinct invalid inputs across every public entry point that wraps errors. Expected errors are built separately from direct tokenizer and parser calls with the old fmt.Errorf` construction, so a missing sentinel or a changed prefix can't pass by matching its own output.
- Error() text is byte-identical.
- errors.Is matches exactly the same sentinels.
- errors.As finds the same structured error, with the same code and position.
- It also preserves an existing quirk: context-based parsing reports a different location for empty input.
Review fix: I fixed a staticcheck ST1008 in the test helper; its error return now comes last.
Gate, run outside the Codex sandbox:

Check	Result
`go run honnef.co/go/tools/cmd/staticcheck@latest ./pkg/gosqlx/...`	✅
`go test -race -count=1 ./pkg/gosqlx/...`	✅
`task check`	✅
`task test:cbinding`	✅
`go test -count=1 ./pkg/compatibility/...`	✅

Local A/B (wave-0 BenchmarkParseError/*, task bench:ab BASE=origin/main). Apple M4, shared and heavily loaded machine (load ~14, timing CIs of +-40-60%), so timings are indicative only; B/op and allocs/op are deterministic.

Case (parse / validate)	time (local, noisy)	B/op	allocs/op
syntax_mid	-27% / -29%	-55%	21 -> 12
syntax_end	-32% / -23%	-36%	25 -> 16
tokenize	-60% / -41%	-83% / -54%	19 -> 4 / 30 -> 21
long_prefix	-14% / -16% (p 0.25 / 0.08, not significant)	-3.4%	229 -> 219
geomean	-30% (p=0.052)	-43.7%	-34.8%

CI amd64 perf gate (shadow): PASS, "no gated metric moved past its threshold". The gated suite (ParseE2E, ParseRW, ParseFresh, PoolGetPut, Tokenize) is neutral: geomean -0.13% (p=0.436), no allocs/op or B/op change, instructions/op within +-0.41%, A/A control clean. These are success-path benchmarks, so neutrality is expected and confirms no regression.

Message-materialized case (reviewer re-measurement, n=6, Parse then err.Error(), base origin/main vs PR). BenchmarkParseError never calls err.Error(), so the table above mostly reflects never formatting the inner *errors.Error. When the message is built, which the LSP (createDiagnosticFromError) and CLI do, the gain is small: allocs/op syntax_mid 21 -> 19, syntax_end 25 -> 23, tokenize 19 -> 17, long_prefix 229 -> 227; B/op -2.8% to -4.2% (long_prefix not significant); time indistinguishable on the loaded M4. The large gain applies to callers that only check errors.Is/errors.As or drop the message.

Behaviour note. Error() is now built lazily, so it reflects later mutation of the inner *errors.Error reached via errors.As; with fmt.Errorf the text was fixed at creation. Not a documented contract; no in-repo caller mutates errors. Noted in CHANGELOG.

Review follow-up. Added wrap_ctx_test.go (context whose Err() flips to Canceled after N calls) which kills the two previously surviving mutants (newWrap(ErrTimeout, ctxErr) in ParseWithContext mid-tokenize and mid-parse paths); verified by mutating both lines. Gate re-run on the new head: task check, task test:cbinding, ./pkg/compatibility/..., staticcheck on ./pkg/gosqlx/... all pass.

Still open (merge blocker, outside this PR's files): add ./pkg/gosqlx ^BenchmarkParseError$/^(syntax_mid|tokenize)$/^(parse|validate)$ to scripts/perf/gate-suite.txt (W0 follow-up), re-run the gate here, and merge only on a statistically significant amd64 gain.

Targets and caveats (reported, not hidden):

The gated CI suite does not include BenchmarkParseError/*, so there is NO amd64 CI measurement of this task's target benchmarks. Evidence for the gain is the deterministic allocation drop (B/op -35% to -83% on short inputs, allocs -35% to -79%) plus local arm64 timings. An amd64 run of BenchmarkParseError is an open question for the orchestrator (PLAN 5.3 rule 5).
PLAN gain (ParseError/* <= -12%, tokenize <= -40%): met on syntax_mid, syntax_end and tokenize/parse (-60%); tokenize/validate is -41% locally (earlier run -28%, load-dependent).
long_prefix is not met: only -3.4% B/op and -4.4% allocs, and the time change is not significant. Its cost is tokenizing and parsing 128 identifiers, which lazy wrapping cannot move.

Breaking Changes

None. The concrete type of the returned error changes from fmt's unexported wrapper to this package's unexported wrapErr. Message text and errors.Is/errors.As behaviour are identical.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2

CI gate status (updated at 04b78ae): the shadow perf gate (base 77c08d2) is PASS and neutral on the gated success-path benchmarks (geomean -0.13%, p=0.436, allocs/B/op unchanged). BenchmarkParseError/*, this PR's target, is NOT in scripts/perf/gate-suite.txt, so its amd64 gain is unmeasured. It will be reported here once a gate-suite follow-up (or #921 matrix mode) lands and the gate is re-run. Merge only if it shows a significant gain; otherwise close.

The 21 fmt.Errorf("%w: %w", sentinel, err) sites in pkg/gosqlx formatted the full message on every error, although many callers only test the error with errors.Is. They now return an unexported wrapErr holding both errors in a [2]error array: Error() builds the identical "sentinel: inner" text on demand, and Unwrap() []error returns a slice of the array, so errors.Is and errors.As traverse it without allocating. Sites with extra context text keep fmt.Errorf unchanged. Tests: a compatibility suite over 242 distinct invalid inputs across every public entry point that wraps errors compares Error() text, errors.Is for every sentinel and errors.As results against the former fmt.Errorf construction, computed independently from the tokenizer and parser. M4, GOGC=off -cpu 1, 10 interleaved process pairs: Parse syntax_mid -28.1%, syntax_end -28.8%, tokenize -62.8%; Validate syntax_mid -26.9%, syntax_end -29.9%, tokenize -28.3% (all p <= 0.029); allocs e.g. 21 -> 12, 20 -> 4. long_prefix (128 identifiers before the error) -1% to -3%. Success paths unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2

vercel · 2026-10-31T19:57:59Z

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

1 Skipped Deployment

Project	Deployment	Actions	Updated
gosqlx-website	Ignored	Preview	Nov 1, 2026 4:09pm UTC

github-actions · 2026-10-31T19:58:25Z

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

Review of PR #905: lazy `wrapErr`

I reviewed the diff only. I did not run the tests or benchmarks, and I did not read wrap_compat_test.go or error_bench_test.go.

Overall: The change is small and the approach is sound. Unwrap() []error over an inline [2]error array matches the two-%w fmt.Errorf contract. Every call site I read passes non-nil values (lint.go is guarded by its switch, and err != nil guards the others). The countedWrapError test shows that nothing is formatted until Error() is called. The compat suite compares against independently built legacy errors, which is the right way to test this.

Things to check before merging

Lazy formatting and pooled state (the main risk).
- The old code captured the message text at wrap time. Now Error() runs later, after PutTokenizer/PutParser and the tokscratch buffers have been released by the deferred calls.
- If an inner error (a tokenizer or parser *errors.Error) holds a slice, a []byte view, or a pointer into pooled or scratch memory, the text could change or be corrupted once those objects are reused.
- It would show up under concurrency, and only if you call Error() after the pools are reused.
- Please confirm the inner errors own their strings. A cheap safeguard is a -race test that:
  1. gets an error from Parse;
  2. runs many other parses concurrently;
  3. compares err.Error() to the text captured immediately.
- The existing tests call Error() right away, so they wouldn't catch this.
errors.Join in tree.go. This path now returns newWrap(ErrSyntax, joined). errors.Is and errors.As still traverse correctly. A joined of a single error still prints as before, so the text should be the same. The compat suite seems to cover it, but it is worth confirming that a recovery case with 2+ diagnostics is among the 242 inputs.
Nil safety. Error() panics if either error is nil, whereas fmt.Errorf would print %!w(<nil>). The doc comment says both must be non-nil, which is fine. For an unexported helper you could add a cheap guard, or just keep the comment.
Aliasing in Unwrap. w.errs[:] hands callers a mutable view of the internal array. Standard library walkers don't write to it, so this is fine in practice. Mention it only if you want to be defensive.
Concrete-type change. The PR says there are no breaking changes. Anyone who type-asserted on *fmt.wrapErrors could not have done so, because the type is unexported, so I agree.
Missed targets. The PR states honestly that long_prefix and Validate/tokenize missed the plan's thresholds. Since this is a draft gated on W0-02 and W0-09, I don't see that as a blocker.
Minor. error_bench_test.go should keep benchmarks deterministic. Error() builds the string on every call, so callers that log an error repeatedly pay the cost each time. Caching it with a sync.Once is probably not worth it.

Verdict: Looks good. Please address item 1 (pooled-memory lifetime) with a test or a quick audit of the inner error types. The rest are nits.

…-wrap

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2

github-actions · 2026-11-01T15:41:48Z

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

Review of PR #905: lazy `wrapErr`

I reviewed the diff and the pkg/errors.Error type. I did not run the tests or benchmarks.

Verdict: Looks good. The change is small, and the approach matches the old two-%w fmt.Errorf contract.

Correctness

Unwrap() []error over an inline [2]error array behaves like fmt.Errorf("%w: %w") for errors.Is and errors.As.
TestWrapErrDefersFormatting checks that nothing is formatted until Error() is called. It also checks that the text matches the legacy output and that errors.As visits the sentinel first.
The dialect-specific test builds its expected errors independently of the code under test. That is the right way to catch a changed prefix or a missing sentinel.
Every call site I read passes non-nil values: lint.go is guarded by its switch, and the others are guarded by err != nil.

Pooled-memory lifetime

This was the main concern in the earlier review, and I checked it.
errors.Error holds only string, Location, *ErrorContext, Cause and HelpURL. ErrorContext.SQL is a string. There are no []byte views or pointers into pooled tokenizer or parser memory.
Calling Error() after PutTokenizer/PutParser is therefore safe.
If you want a regression guard, add a -race test that:
1. takes an error from Parse;
2. runs many parses concurrently;
3. checks that err.Error() equals the text captured immediately.

Minor points (non-blocking)

In tree.go, the recovery path returns newWrap(ErrSyntax, errors.Join(errs...)). Please confirm the 242 compat inputs include a case with two or more diagnostics. A single-error join prints identically, but a multi-error join prints newline-separated text.
Error() panics on a nil member, whereas fmt.Errorf prints %!w(<nil>). The doc comment states the non-nil requirement, which is enough for an unexported type.
Unwrap returns w.errs[:], which is a mutable view of the internal array. The standard library walkers don't write to it, so this is fine.
Each Error() call rebuilds the string. Callers that log the same error repeatedly pay for it each time. Caching it isn't worth the added complexity.
long_prefix and Validate/tokenize miss the plan's targets. The PR says so openly, and the PR is a draft gated on W0-02 and W0-09, so this isn't a blocker.
Changing the concrete type is not a breaking change. The old type was unexported, so nobody could assert on it.

github-actions · 2026-11-01T15:55:13Z

Perf gate: PASS (shadow mode: report only, this check never fails)

Base 77c08d2921f0, head 5736a9282776. 10 interleaved rounds, benchtime 200ms, -cpu 1, pinned to CPU 3 (SMT siblings 2-3) with taskset.

no gated metric moved past its threshold

Rules: time regresses at >= +3% with p < 0.01 (Mann-Whitney U), or a suite geomean >= +1.5% (per-round geomeans, same test); allocs/op must not increase; B/op must not rise more than 2%; instructions/op must not rise more than 1.5%. An A/A control with more than 10% of benchmarks moving over 3% is noisy: re-run once, then inconclusive (neutral).

Gated suite (GOGC=off, GOMAXPROCS=1)

benchmark	time	p	A/A	B/op	allocs/op	status
`ParseE2E/simple_select`	+2.45%	<0.001	+0.05%	+0.00%	5 → 5	ok
`ParseE2E/select_where`	+0.81%	0.011	+0.28%	+0.00%	18 → 18	ok
`ParseE2E/join`	-0.47%	0.112	-0.40%	+0.00%	17 → 17	ok
`ParseE2E/aggregate`	+0.11%	0.970	-0.08%	+0.00%	18 → 18	ok
`ParseE2E/subquery`	+0.67%	0.037	+0.00%	+0.00%	14 → 14	ok
`ParseE2E/cte`	+1.19%	0.003	+0.06%	+0.00%	21 → 21	ok
`ParseE2E/window`	+0.73%	0.130	+0.13%	+0.00%	11 → 11	ok
`ParseE2E/complex`	+0.18%	0.315	+0.18%	+0.00%	50 → 50	ok
`ParseE2E/insert`	+0.98%	<0.001	+0.22%	+0.00%	17 → 17	ok
`ParseE2E/update`	+0.85%	0.002	-0.06%	+0.00%	13 → 13	ok
`ParseE2E/delete`	+2.03%	<0.001	+0.30%	+0.00%	5 → 5	ok
`ParseRW/postgresql/medium`	+1.40%	0.003	+0.14%	+0.00%	646 → 646	ok
`ParseRW/mysql/medium`	+0.59%	0.015	-0.10%	+0.00%	365 → 365	ok
`ParseRW/mariadb/medium`	+1.15%	0.003	-0.20%	+0.00%	523 → 523	ok
`ParseRW/sqlserver/medium`	+1.02%	0.009	-0.05%	+0.00%	936 → 936	ok
`ParseRW/oracle/medium`	+1.08%	<0.001	-0.02%	+0.00%	1093 → 1093	ok
`ParseRW/sqlite/medium`	+1.24%	<0.001	-0.16%	+0.00%	346 → 346	ok
`ParseRW/snowflake/medium`	+0.86%	0.002	-0.15%	+0.00%	777 → 777	ok
`ParseRW/clickhouse/medium`	+0.72%	0.015	-0.02%	+0.00%	384 → 384	ok
`ParseFresh/simple_select`	+1.13%	<0.001	+0.01%	+0.00%	6 → 6	ok
`ParseFresh/join`	+0.41%	0.290	+0.34%	+0.00%	18 → 18	ok
`ParseFresh/complex`	+1.05%	<0.001	+0.31%	+0.00%	51 → 51	ok
`PoolGetPut/serial`	+0.05%	1.000	+0.00%	+0.00%	0 → 0	ok
`Tokenize/scratch/join`	+0.04%	0.622	-0.08%	+0.00%	0 → 0	ok
`Tokenize/scratch/complex`	-0.01%	0.427	-0.13%	+0.00%	0 → 0	ok
`Tokenize/scratch/insert`	+0.38%	0.062	+0.29%	+0.00%	0 → 0	ok
geomean	+0.79%	<0.001	+0.03%

A/A control: 0 of 26 benchmarks moved more than 3%.

Instructions per operation (cmd/perfdriver)

case	instructions/op	status
`e2e/aggregate`	+0.07%	ok
`e2e/complex`	+0.20%	ok
`e2e/cte`	-0.17%	ok
`e2e/delete`	+0.07%	ok
`e2e/insert`	-0.03%	ok
`e2e/join`	-0.03%	ok
`e2e/select_where`	-0.04%	ok
`e2e/simple_select`	+0.03%	ok
`e2e/subquery`	-0.05%	ok
`e2e/update`	+0.00%	ok
`e2e/window`	+0.00%	ok
`rw/postgresql/medium`	+0.04%	ok

Secondary metric: default GC, GOMAXPROCS=1 (informational)

benchmark	time	p
`ParseE2E/simple_select`	+1.78%	<0.001
`ParseE2E/select_where`	+1.28%	0.001
`ParseE2E/join`	-0.42%	0.289
`ParseE2E/aggregate`	-0.25%	0.880
`ParseE2E/subquery`	+0.75%	0.003
`ParseE2E/cte`	+0.55%	0.034
`ParseE2E/window`	+0.55%	0.049
`ParseE2E/complex`	+0.20%	0.529
`ParseE2E/insert`	+1.85%	<0.001
`ParseE2E/update`	+1.25%	0.003
`ParseE2E/delete`	+2.62%	<0.001
`ParseRW/postgresql/medium`	+0.83%	<0.001
`ParseRW/mysql/medium`	+1.11%	<0.001
`ParseRW/mariadb/medium`	+0.79%	0.002
`ParseRW/sqlserver/medium`	+0.33%	0.105
`ParseRW/oracle/medium`	+0.98%	0.001
`ParseRW/sqlite/medium`	+0.23%	0.052
`ParseRW/snowflake/medium`	+0.39%	0.105
`ParseRW/clickhouse/medium`	+0.33%	0.165
`ParseFresh/simple_select`	+0.69%	0.052
`ParseFresh/join`	-0.64%	0.043
`ParseFresh/complex`	+0.79%	0.001
`PoolGetPut/serial`	-0.05%	0.520
`Tokenize/scratch/join`	+0.29%	0.520
`Tokenize/scratch/complex`	+0.07%	0.529
`Tokenize/scratch/insert`	+0.29%	0.494
geomean	+0.64%

Environment

date:        2026-11-01T16:11:51Z
base:        77c08d2921f06fcbe5d26e725bec9bcc0250bdfe
head:        working tree of 5736a928277680bf0e2f12b146e73b6f0b2af2d7
go:          go version go1.26.1 linux/amd64
kernel:      Linux 7.0.0-1017-azure x86_64
pinning:     pinned to CPU 3 (SMT siblings 2-3) with taskset
clocksource: tsc
load:        1.42, 0.42, 0.15
instr count: cachegrind
perf probe:  Error: No supported events found. The instructions:u event is not supported. 

Architecture:                            x86_64
CPU(s):                                  4
Model name:                              AMD EPYC 9V74 80-Core Processor
Thread(s) per core:                      2
Core(s) per socket:                      2
Socket(s):                               1
Hypervisor vendor:                       Microsoft
L1d cache:                               64 KiB (2 instances)
L1i cache:                               64 KiB (2 instances)
L2 cache:                                2 MiB (2 instances)
L3 cache:                                32 MiB (1 instance)

benchstat (primary metric)

goos: linux
goarch: amd64
pkg: github.com/ajitpratap0/GoSQLX/pkg/gosqlx
cpu: AMD EPYC 9V74 80-Core Processor                
                          │    base     │                head                │
                          │   sec/op    │   sec/op     vs base               │
ParseE2E/simple_select      1.833µ ± 0%   1.878µ ± 1%  +2.45% (p=0.000 n=10)
ParseE2E/select_where       3.755µ ± 1%   3.785µ ± 1%  +0.81% (p=0.008 n=10)
ParseE2E/join               5.946µ ± 1%   5.918µ ± 1%       ~ (p=0.109 n=10)
ParseE2E/aggregate          4.167µ ± 0%   4.172µ ± 1%       ~ (p=0.956 n=10)
ParseE2E/subquery           3.652µ ± 2%   3.677µ ± 0%       ~ (p=0.034 n=10)
ParseE2E/cte                4.908µ ± 1%   4.967µ ± 1%  +1.19% (p=0.001 n=10)
ParseE2E/window             3.338µ ± 1%   3.362µ ± 0%       ~ (p=0.128 n=10)
ParseE2E/complex            15.38µ ± 1%   15.41µ ± 0%       ~ (p=0.315 n=10)
ParseE2E/insert             2.702µ ± 0%   2.729µ ± 0%  +0.98% (p=0.001 n=10)
ParseE2E/update             2.405µ ± 1%   2.425µ ± 3%  +0.85% (p=0.001 n=10)
ParseE2E/delete             1.478µ ± 1%   1.509µ ± 3%  +2.03% (p=0.000 n=10)
ParseRW/postgresql/medium   171.8µ ± 1%   174.2µ ± 0%  +1.40% (p=0.003 n=10)
ParseRW/mysql/medium        95.98µ ± 1%   96.55µ ± 0%       ~ (p=0.015 n=10)
ParseRW/mariadb/medium      138.8µ ± 0%   140.4µ ± 0%  +1.15% (p=0.003 n=10)
ParseRW/sqlserver/medium    265.3µ ± 1%   268.0µ ± 0%  +1.02% (p=0.009 n=10)
ParseRW/oracle/medium       320.8µ ± 0%   324.3µ ± 2%  +1.08% (p=0.000 n=10)
ParseRW/sqlite/medium       82.53µ ± 1%   83.56µ ± 2%  +1.24% (p=0.000 n=10)
ParseRW/snowflake/medium    198.0µ ± 0%   199.7µ ± 1%  +0.86% (p=0.002 n=10)
ParseRW/clickhouse/medium   92.66µ ± 1%   93.33µ ± 1%       ~ (p=0.015 n=10)
ParseFresh/simple_select    8.209µ ± 0%   8.302µ ± 0%  +1.13% (p=0.000 n=10)
ParseFresh/join             18.59µ ± 1%   18.67µ ± 1%       ~ (p=0.289 n=10)
ParseFresh/complex          31.10µ ± 1%   31.43µ ± 1%  +1.05% (p=0.000 n=10)
PoolGetPut/serial           107.3n ± 1%   107.4n ± 1%       ~ (p=1.000 n=10)
geomean                     13.99µ        14.11µ       +0.88%

                          │     base     │                head                 │
                          │     B/s      │     B/s       vs base               │
ParseE2E/simple_select      13.53Mi ± 0%   13.20Mi ± 1%  -2.40% (p=0.000 n=10)
ParseE2E/select_where       26.16Mi ± 1%   25.95Mi ± 1%  -0.80% (p=0.008 n=10)
ParseE2E/join               20.21Mi ± 1%   20.30Mi ± 1%       ~ (p=0.118 n=10)
ParseE2E/aggregate          19.23Mi ± 0%   19.20Mi ± 1%       ~ (p=0.897 n=10)
ParseE2E/subquery           21.41Mi ± 2%   21.27Mi ± 0%       ~ (p=0.037 n=10)
ParseE2E/cte                24.09Mi ± 1%   23.81Mi ± 1%  -1.19% (p=0.001 n=10)
ParseE2E/window             26.86Mi ± 1%   26.66Mi ± 0%       ~ (p=0.128 n=10)
ParseE2E/complex            23.43Mi ± 1%   23.39Mi ± 0%       ~ (p=0.314 n=10)
ParseE2E/insert             26.83Mi ± 0%   26.56Mi ± 0%  -0.98% (p=0.001 n=10)
ParseE2E/update             21.81Mi ± 1%   21.62Mi ± 3%  -0.85% (p=0.001 n=10)
ParseE2E/delete             33.54Mi ± 1%   32.87Mi ± 3%  -2.00% (p=0.000 n=10)
ParseRW/postgresql/medium   20.88Mi ± 1%   20.59Mi ± 0%  -1.37% (p=0.003 n=10)
ParseRW/mysql/medium        28.20Mi ± 1%   28.03Mi ± 0%       ~ (p=0.015 n=10)
ParseRW/mariadb/medium      25.55Mi ± 0%   25.26Mi ± 0%  -1.14% (p=0.003 n=10)
ParseRW/sqlserver/medium    25.08Mi ± 1%   24.83Mi ± 0%  -1.01% (p=0.008 n=10)
ParseRW/oracle/medium       26.13Mi ± 0%   25.85Mi ± 2%  -1.08% (p=0.000 n=10)
ParseRW/sqlite/medium       25.03Mi ± 1%   24.72Mi ± 2%  -1.24% (p=0.000 n=10)
ParseRW/snowflake/medium    25.68Mi ± 0%   25.46Mi ± 1%  -0.85% (p=0.001 n=10)
ParseRW/clickhouse/medium   24.32Mi ± 1%   24.15Mi ± 1%       ~ (p=0.014 n=10)
geomean                     23.73Mi        23.51Mi       -0.92%

                          │      base      │                 head                  │
                          │      B/op      │     B/op      vs base                 │
ParseE2E/simple_select        816.0 ± 0%       816.0 ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/select_where       1.523Ki ± 0%     1.523Ki ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/join               2.305Ki ± 0%     2.305Ki ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/aggregate          1.812Ki ± 0%     1.812Ki ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/subquery           1.828Ki ± 0%     1.828Ki ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/cte                2.336Ki ± 0%     2.336Ki ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/window             1.281Ki ± 0%     1.281Ki ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/complex            6.399Ki ± 0%     6.399Ki ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/insert               824.0 ± 0%       824.0 ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/update               736.0 ± 0%       736.0 ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/delete               400.0 ± 0%       400.0 ± 0%       ~ (p=1.000 n=10) ¹
ParseRW/postgresql/medium   70.52Ki ± 0%     70.52Ki ± 0%       ~ (p=0.211 n=10)
ParseRW/mysql/medium        43.73Ki ± 0%     43.73Ki ± 0%       ~ (p=1.000 n=10) ¹
ParseRW/mariadb/medium      64.84Ki ± 0%     64.84Ki ± 0%  +0.00% (p=0.008 n=10)
ParseRW/sqlserver/medium    90.59Ki ± 0%     90.59Ki ± 0%       ~ (p=0.191 n=10)
ParseRW/oracle/medium       113.8Ki ± 0%     113.8Ki ± 0%       ~ (p=0.132 n=10)
ParseRW/sqlite/medium       39.89Ki ± 0%     39.89Ki ± 0%       ~ (p=1.000 n=10) ¹
ParseRW/snowflake/medium    83.38Ki ± 0%     83.38Ki ± 0%       ~ (p=1.000 n=10) ¹
ParseRW/clickhouse/medium   37.13Ki ± 0%     37.13Ki ± 0%       ~ (p=1.000 n=10) ¹
ParseFresh/simple_select      896.0 ± 0%       896.0 ± 0%       ~ (p=1.000 n=10) ¹
ParseFresh/join             2.383Ki ± 0%     2.383Ki ± 0%       ~ (p=1.000 n=10) ¹
ParseFresh/complex          6.477Ki ± 0%     6.477Ki ± 0%       ~ (p=1.000 n=10) ¹
PoolGetPut/serial             0.000 ± 0%       0.000 ± 0%       ~ (p=1.000 n=10) ¹
geomean                                  ²                 +0.00%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                          │     base      │                 head                 │
                          │   allocs/op   │  allocs/op   vs base                 │
ParseE2E/simple_select       5.000 ± 0%      5.000 ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/select_where        18.00 ± 0%      18.00 ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/join                17.00 ± 0%      17.00 ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/aggregate           18.00 ± 0%      18.00 ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/subquery            14.00 ± 0%      14.00 ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/cte                 21.00 ± 0%      21.00 ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/window              11.00 ± 0%      11.00 ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/complex             50.00 ± 0%      50.00 ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/insert              17.00 ± 0%      17.00 ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/update              13.00 ± 0%      13.00 ± 0%       ~ (p=1.000 n=10) ¹
ParseE2E/delete              5.000 ± 0%      5.000 ± 0%       ~ (p=1.000 n=10) ¹
ParseRW/postgresql/medium    646.0 ± 0%      646.0 ± 0%       ~ (p=1.000 n=10) ¹
ParseRW/mysql/medium         365.0 ± 0%      365.0 ± 0%       ~ (p=1.000 n=10) ¹
ParseRW/mariadb/medium       523.0 ± 0%      523.0 ± 0%       ~ (p=1.000 n=10) ¹
ParseRW/sqlserver/medium     936.0 ± 0%      936.0 ± 0%       ~ (p=1.000 n=10) ¹
ParseRW/oracle/medium       1.093k ± 0%     1.093k ± 0%       ~ (p=1.000 n=10) ¹
ParseRW/sqlite/medium        346.0 ± 0%      346.0 ± 0%       ~ (p=1.000 n=10) ¹
ParseRW/snowflake/medium     777.0 ± 0%      777.0 ± 0%       ~ (p=1.000 n=10) ¹
ParseRW/clickhouse/medium    384.0 ± 0%      384.0 ± 0%       ~ (p=1.000 n=10) ¹
ParseFresh/simple_select     6.000 ± 0%      6.000 ± 0%       ~ (p=1.000 n=10) ¹
ParseFresh/join              18.00 ± 0%      18.00 ± 0%       ~ (p=1.000 n=10) ¹
ParseFresh/complex           51.00 ± 0%      51.00 ± 0%       ~ (p=1.000 n=10) ¹
PoolGetPut/serial            0.000 ± 0%      0.000 ± 0%       ~ (p=1.000 n=10) ¹
geomean                                 ²                +0.00%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                          │    base     │                head                │
                          │  sec/stmt   │  sec/stmt    vs base               │
ParseRW/postgresql/medium   14.32µ ± 1%   14.52µ ± 0%  +1.39% (p=0.003 n=10)
ParseRW/mysql/medium        12.00µ ± 1%   12.07µ ± 0%       ~ (p=0.015 n=10)
ParseRW/mariadb/medium      15.42µ ± 0%   15.60µ ± 0%  +1.15% (p=0.003 n=10)
ParseRW/sqlserver/medium    12.63µ ± 1%   12.76µ ± 0%  +1.03% (p=0.009 n=10)
ParseRW/oracle/medium       13.37µ ± 0%   13.51µ ± 2%  +1.08% (p=0.000 n=10)
ParseRW/sqlite/medium       13.76µ ± 1%   13.93µ ± 2%  +1.24% (p=0.000 n=10)
ParseRW/snowflake/medium    15.23µ ± 0%   15.36µ ± 1%  +0.87% (p=0.001 n=10)
ParseRW/clickhouse/medium   13.24µ ± 1%   13.33µ ± 1%       ~ (p=0.015 n=10)
geomean                     13.70µ        13.84µ       +1.01%

pkg: github.com/ajitpratap0/GoSQLX/pkg/sql/tokenizer
                         │    base     │                head                │
                         │   sec/op    │   sec/op     vs base               │
Tokenize/scratch/join      2.626µ ± 2%   2.627µ ± 0%       ~ (p=0.617 n=10)
Tokenize/scratch/complex   6.807µ ± 1%   6.807µ ± 1%       ~ (p=0.424 n=10)
Tokenize/scratch/insert    1.198µ ± 0%   1.202µ ± 0%       ~ (p=0.057 n=10)
geomean                    2.777µ        2.780µ       +0.14%

                         │     base     │                head                 │
                         │     B/s      │     B/s       vs base               │
Tokenize/scratch/join      45.76Mi ± 2%   45.74Mi ± 0%       ~ (p=0.630 n=10)
Tokenize/scratch/complex   52.96Mi ± 1%   52.96Mi ± 1%       ~ (p=0.424 n=10)
Tokenize/scratch/insert    60.52Mi ± 0%   60.31Mi ± 0%       ~ (p=0.051 n=10)
geomean                    52.74Mi        52.67Mi       -0.13%

                         │     base     │                head                 │
                         │     B/op     │    B/op     vs base                 │
Tokenize/scratch/join      0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Tokenize/scratch/complex   0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Tokenize/scratch/insert    0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
geomean                               ²               +0.00%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                         │     base     │                head                 │
                         │  allocs/op   │ allocs/op   vs base                 │
Tokenize/scratch/join      0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Tokenize/scratch/complex   0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
Tokenize/scratch/insert    0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=10) ¹
geomean                               ²               +0.00%                ²
¹ all samples are equal
² summaries must be >0 to compute geomean

Run, raw data and benchstat

…zed-message gain [W1-03] Add a flipping-context test that kills the two surviving mutants in ParseWithContext (mid-tokenize and mid-parse cancel paths). CHANGELOG now states the gain applies to callers that drop the message, gives the materialized-message numbers, and notes Error() reflects later mutation of the inner error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2

github-actions · 2026-11-01T16:06:39Z

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

Review of PR #905: lazy `wrapErr`

I read the diff and the new tests. I did not run the tests or benchmarks. My attempt to build and vet was blocked by a permission prompt, so I'm relying on the passing CI gate for that.

Verdict: Looks good to merge. The earlier review items are either resolved or non-blocking.

Correctness

wrapErr keeps both errors in a [2]error array. Unwrap() []error returns a slice of that array, so errors.Is and errors.As see the same tree as the two-%w fmt.Errorf, and the sentinel is visited first.
Error() builds sentinel: inner on demand. This matches the old text, including the joined multi-error case in tree.go.
All 21 call sites pass non-nil values. The ones I checked are in gosqlx.go, lint.go and tree.go.
Call sites that add extra context text still use fmt.Errorf, as intended.
wrap_ctx_test.go is a good addition. It sweeps the cancellation point and requires ErrTimeout plus context.Canceled, and never ErrTokenize or ErrSyntax.

Earlier review points

Pooled-memory lifetime: errors.Error holds only strings, Location, *ErrorContext, Cause and HelpURL. A prior review confirmed this, and I found nothing that changes it. So calling Error() after PutTokenizer/PutParser is safe.
Recovery with 2+ diagnostics: wrap_compat_test.go has ParseTreeRecovery, ParseReaderRecovery and ParseWithRecovery subtests. They go through the errors.Join path. I did not confirm that any input in the suite produces two or more diagnostics, so please check that.

Suggestions (non-blocking)

Add a small -race regression test. It would take an error from Parse, run many parses concurrently, then compare err.Error() with the text captured immediately. It would guard against future inner error types holding pooled memory. No current code needs it.
Error() panics on a nil member, where fmt.Errorf would print %!w(<nil>). The doc comment states the non-nil requirement, which is enough for an unexported helper.
Unwrap returns w.errs[:], which is a mutable view of the internal array. The standard library walkers don't write to it, so this is fine.
Error() rebuilds the string on every call. That is the right trade-off for the goal, because caching would add state and cost on the success path.
long_prefix misses the plan target, and ParseError/* has no amd64 CI measurement. The PR discloses both, and the shadow perf gate shows no regression on the success path.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2

github-actions · 2026-11-01T16:13:43Z

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

Review of PR #905: lazy `wrapErr`

I read the changed files and spot-checked the recovery tests. I did not run tests or benchmarks. I'm relying on the passing CI and shadow perf gate.

Verdict: Looks good to merge once the gate-suite follow-up you listed is done. I found no new blocking issues.

Correctness

wrapErr stores both errors in a [2]error array. Unwrap() []error matches the two-%w fmt.Errorf contract for errors.Is and errors.As.
Error() produces the same sentinel: inner text. Call sites that add extra context still use fmt.Errorf, which is correct.
The pooled-memory concern from earlier reviews doesn't apply. errors.Error holds only strings, Location, *ErrorContext, Cause and HelpURL, so calling Error() after PutTokenizer/PutParser is safe.
wrap_ctx_test.go closes the mutation gap on the ErrTimeout paths. The Err()-flip approach is a good way to test cancellation mid-tokenize and mid-parse.

Open points

Recovery with 2+ diagnostics. The recovery subtests in wrap_compat_test.go (around lines 266-283) compare against errors.Join(errs...), so the join path is exercised. I could not confirm that any of the 242 inputs yields two or more diagnostics. A multi-error join prints newline-separated text. A one-line assertion len(recoveryInner) >= 2 for at least one input would pin that down.
Mutable Unwrap view. Unwrap returns w.errs[:], an alias of the internal array. Standard walkers don't write to it. Copying would cost an allocation and defeat the purpose, so I'd leave it and keep the doc comment.
Nil members. Error() panics on a nil member, where fmt.Errorf would print %!w(<nil>). All 21 call sites guard against nil, and the doc comment states the requirement. That is enough for an unexported type.
Lazy Error() and mutation. The CHANGELOG notes that Error() now reflects later mutation of the inner error. That is fine. Repeated Error() calls rebuild the string each time, which is the right trade-off for the error-path goal.
Merge blocker (yours). There is still no amd64 CI measurement of BenchmarkParseError/*. Add it to scripts/perf/gate-suite.txt and re-run before merging, as you planned. The materialized-message gain is small (2 allocs and 3-4% B/op), and the PR discloses it.
Optional. A -race test that captures err.Error(), runs many concurrent parses, then compares the text. No current code needs it, but it would guard against future inner error types that hold pooled memory.

github-actions Bot added the performance label Oct 31, 2026

ajitpratap0 and others added 2 commits November 1, 2026 21:06

Merge remote-tracking branch 'origin/main' into perf/w1-03-lazy-error…

02bc23f

…-wrap

perf(gosqlx): drop duplicate error benchmarks, add changelog [W1-03]

f953bbf

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2

ajitpratap0 marked this pull request as ready for review November 1, 2026 15:41

github-actions Bot added the documentation Improvements or additions to documentation label Nov 1, 2026

vercel Bot deployed to Preview November 1, 2026 15:41

View deployment

vercel Bot deployed to Preview November 1, 2026 16:03

View deployment

style(gosqlx): add license header to wrap_ctx_test.go [W1-03]

04b78ae

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnbRnTFErhoMJdAjVEeCg2

Navigation Menu

Repository navigation

perf(gosqlx): build wrapped error messages lazily [W1-03] - #905

perf(gosqlx): build wrapped error messages lazily [W1-03]#905
ajitpratap0 wants to merge 5 commits into
mainfrom
perf/w1-03-lazy-error-wrap

ajitpratap0 commented Oct 31, 2026 •

edited

Loading

Uh oh!

vercel Bot commented Oct 31, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Oct 31, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Nov 1, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Nov 1, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Nov 1, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Nov 1, 2026 •

edited

Loading

Uh oh!

This branch was successfully deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

ajitpratap0 commented Oct 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Breaking Changes

Uh oh!

vercel Bot commented Oct 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Oct 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review of PR #905: lazy wrapErr

Uh oh!

github-actions Bot commented Nov 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review of PR #905: lazy wrapErr

Uh oh!

github-actions Bot commented Nov 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Perf gate: PASS (shadow mode: report only, this check never fails)

Gated suite (GOGC=off, GOMAXPROCS=1)

Instructions per operation (cmd/perfdriver)

Uh oh!

github-actions Bot commented Nov 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review of PR #905: lazy wrapErr

Uh oh!

github-actions Bot commented Nov 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review of PR #905: lazy wrapErr

Uh oh!

This branch was successfully deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

ajitpratap0 commented Oct 31, 2026 •

edited

Loading

vercel Bot commented Oct 31, 2026 •

edited

Loading

github-actions Bot commented Oct 31, 2026 •

edited

Loading

Review of PR #905: lazy `wrapErr`

github-actions Bot commented Nov 1, 2026 •

edited

Loading

Review of PR #905: lazy `wrapErr`

github-actions Bot commented Nov 1, 2026 •

edited

Loading

github-actions Bot commented Nov 1, 2026 •

edited

Loading

Review of PR #905: lazy `wrapErr`

github-actions Bot commented Nov 1, 2026 •

edited

Loading

Review of PR #905: lazy `wrapErr`