fix(frontend): reuse prepared DML specialization by semantic domain - #27802
fix(frontend): reuse prepared DML specialization by semantic domain#27802daviszhen wants to merge 11 commits into
Conversation
…origin#27493)" This reverts commit 8b0a5ff because it regressed TPCC performance. Preserve the later direct prepared-result specialization from matrixorigin#27713. Explicitly keep INSERT, UPDATE, DELETE, and MERGE on the cached parameterized compile path so removing the reverted DML write-root rewriter cannot reintroduce positional write corruption.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
…pcc-regression # Conflicts: # pkg/frontend/computation_wrapper.go # pkg/frontend/computation_wrapper_test.go
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review completed on exact head 09d6dc1. Two blockers remain.
-
[P1 correctness/lifecycle] Do not publish a background-session specialization Compile into the client PrepareStmt cache. A procedure/background EXECUTE can miss the runtime category and stage
runtimeCacheTargetat computation_wrapper.go:1497-1527.createCompilethen builds the candidate with the backgroundcwft.proc(lines 544-558), and a successful Run publishes it into the upstream/clientPrepareStmt(lines 1616-1639). The next foreground EXECUTE can hit and reuse that Compile (lines 1507-1511, 1560-1562). This crosses the process ownership boundary that lines 1569-1572 explicitly say is unsafe.Compile.Resetchanges onlyc.proc;Scope.resetForReusekeeps each materializeds.Procand merely copies a few fields (pkg/sql/compile/scope.go:267-295), so the cached pipeline remains tied to the background process/txn/parameter context. The reachable sequence is: client PREPARE -> procedure/background category miss -> successful candidate publication -> client same-category EXECUTE -> stale/wrong process-owned pipeline, risking wrong parameters/transaction state or execution failure. Keep runtime Compile publication process-local (the simplest safe rule is not to stage/publish candidates forexecutionSes.IsBackgroundSession()), and add a regression that executes a domain-sensitive prepared DML from a procedure/background session and then from the owning client (also cover replacing an existing client category from background). -
[Validation/performance] The linked issue requires TPCC100 server-prepared throughput to return within 5% of the direct-parent baseline, plus stable-domain and type-switch evidence. The PR currently reports only
BenchmarkBinaryDMLRuntimeSpecializationCache; locally this is 4.275 us/op, 1496 B/op, 26 allocs/op, but it measuresinitExecuteStmtParamcache lookup and does not execute Compile.Reset/Run or compare TPCC throughput. Since the regression being fixed is ~30% end-to-end, this does not establish that the issue is solved. Please attach same-environment TPCC evidence (zero errors and consistency check) against the stated baseline, and include the stable-domain/type-switch comparison required by #27804.
Coverage completed: all three changed files/hunks; semantic-domain key and ParamRef restoration; cache hit/miss/category replacement; compile failure, Run failure, retry/rebuild, release/close; background process ownership; boundedness (one-entry cache); hot-path allocations; existing DML/type-switch consumers. Focused new tests passed, exact-head CI is green, and the cache benchmark reproduced. These do not close the two blockers above.
…pcc-regression # Conflicts: # pkg/frontend/computation_wrapper.go # pkg/frontend/computation_wrapper_test.go
|
Addressed the background-process ownership blocker and merged current Changes:
Local validation on the exact head:
Repeated local benchmark (
I also attempted to dispatch the same 128-runner/full/5-minute TPCC workflow for exact head
Please trigger the authorized same-environment TPCC100 run and re-review this exact head. The code ownership blocker and stable/type-switch evidence are now covered. |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed the exact head ec37a28 from design, correctness, Process/Compile ownership, failure/retry cleanup, cache boundedness, semantic-key compatibility, and performance angles.
The previous P1 code blocker is fixed: background/procedure execution can neither consume the client runtime Compile nor stage/publish a replacement, and the new regression covers miss, same-domain hit avoidance, type switch, and subsequent foreground reuse. The one-entry cache, publish-after-success ordering, failed Run/plan-generation invalidation, old-category retirement, ParamRef restoration, and mode/protocol/type key separation look internally consistent. I found no new code-correctness blocker in those paths.
The performance acceptance blocker is still open. #27804 is specifically a roughly 30% end-to-end TPCC100 regression and requires the same 128-runner, server-prepared TPCC100 run to return within 5% of the 42,778.91 tpmC / 95,161.74 tpmTOTAL direct-parent baseline, with zero errors and consistency verification, plus TPCC10/100 and an ordinary prepared SELECT control. The reported microbenchmark exercises initExecuteStmtParam cache mechanics only; 6.69-6.77 us/op and 26 allocs/op are useful stable/type-switch evidence, but they do not execute Compile.Reset/Run, transaction/network work, or establish recovered TPCC throughput. The PR description itself says the required workflow could not be dispatched, so the primary issue outcome remains unverified.
Please attach an authorized same-environment run on this exact patch (or a rebased patch-identical head) and the baseline comparison. This is not waiting for generic CI; it is the defining performance acceptance test for a hot-path fix. Once that evidence meets #27804, the prior code blocker is already closed.
aunjgr
left a comment
There was a problem hiding this comment.
Re-reviewed exact head ec37a28b46ce51c2d4ce68feafc1a6daea0553d1 against merge base 48ffaa5df8eb6918cebcf4435800596d48dbac0e and newest main@4661c00ce016cf0d075d5395c73fc02c75307f08.
The prior code/lifecycle blocker is closed. Background/procedure execution neither consumes the client-owned runtime Compile nor stages/publishes a Process-bound replacement; the regression covers miss, same-domain avoidance, type switch, and later foreground reuse. Foreground specialization remains a bounded one-entry semantic-domain cache, publishes only after successful execution, retires replaced categories, restores ParamRefs so hits read current values, separates generic/numeric/direct-result/protocol domains, and invalidates correctly on run/retry/plan-generation failure. I found no remaining code-correctness, Process ownership, cleanup, cache-generation, or compatibility blocker.
The defining performance acceptance gate for #27804 is still missing. The reported regression is roughly 30% in the 128-runner server-prepared TPCC100 workload, and the issue requires the exact-head result to return within 5% of the 42,778.91 tpmC / 95,161.74 tpmTOTAL direct-parent baseline with zero errors and a successful consistency check, plus the specified TPCC10/100 and ordinary prepared SELECT controls. BenchmarkBinaryDMLRuntimeSpecializationCache establishes useful stable-domain/type-switch mechanics, but it exercises initExecuteStmtParam only; it does not run Compile.Reset/Run, transactions, RPC/network, or the TPCC statement mix, so 6.69–6.77 us/op cannot prove the product regression is recovered.
Attach an authorized same-environment run on this exact or patch-identical rebased head and its baseline comparison. Once that acceptance evidence passes, I have no remaining implementation blocker.
aptend
left a comment
There was a problem hiding this comment.
Deep review completed at exact head ec37a28b46ce51c2d4ce68feafc1a6daea0553d1, including the full diff, prior review history, cache/process ownership, replacement/failure paths, background-session isolation, and exact-head tests.
Blocking validation gap at pkg/frontend/computation_wrapper_test.go:1499: the added BenchmarkBinaryDMLRuntimeSpecializationCache measures repeated calls to initExecuteStmtParam, but it does not execute the affected server-prepared TPCC path through Compile.Reset/Compile.Run, transactions, RPC/network handling, or the TPCC statement mix. The defining #27804 repro regressed the same 128-runner server-prepared tpcc100.100 workload from 42,778.91 to 29,731.21 tpmC (about 30.5%), while the acceptance criterion is within 5% of the direct-parent baseline with zero errors. Consequently, the current microbenchmark cannot show that the user-visible regression is fixed or that correctness/throughput remains stable under the real trigger.
Please provide an authorized, same-environment run on this exact or patch-identical head covering the requested tpcc100.100 server-prepared case, with the direct-parent control, repeated throughput/error results and consistency checks; also include the #27804 TPCC 10/100 and ordinary prepared-SELECT controls. This is release-defining evidence for a performance-regression fix, so CI/UT and the isolated helper benchmark are not substitutes.
The earlier Process/Compile ownership blocker appears closed in this head: background executions neither consume nor publish the client cache, replacement publishes only after successful execution, and failure/rebuild paths retain safe ownership. Local validation passed: full go test ./pkg/frontend -count=1, focused specialization/lifecycle tests, and the benchmark (stable-domain 5750 ns/op; type-switch 28115 ns/op).
What type of PR is this?
Which issue(s) this PR fixes:
issue #27804
What this PR does / why we need it:
The original full revert exposed correctness regressions already covered by current main, including forced multi-CN predicates and
INSERT IGNORE ... SELECT. This update keeps the correctness machinery from #27493 and fixes the TPCC regression by reusing execute-time specialization for stable prepared-statement semantic domains.ParamRefprovenance in cached plans so a cache hit reads the current execution values rather than values from the first execution;mainat4cbdc2cf46and integrates its prepared numeric-overload path.Validation
go test ./pkg/frontend ./pkg/sql/plan ./pkg/sql/plan/function ./pkg/sql/colexec -count=1go test ./pkg/tests/dml -run '^TestForcedMultiCNDeleteAndInsertIgnore$' -count=1go test ./pkg/tests/issues -run '^TestIssue27443BinaryPreparedDMLAndAggregate$' -count=1go vet ./pkg/frontend ./pkg/sql/plan ./pkg/sql/plan/function ./pkg/sql/colexec ./pkg/tests/dml ./pkg/tests/issuesgolangci-lint run -c .golangci.yml ./pkg/frontendBenchmarkBinaryDMLRuntimeSpecializationCache: 9.1 us/op, 1496 B/op, 26 allocs/op (100 iterations on the local validation host)git diff --check