Skip to content

fix(frontend): reuse prepared DML specialization by semantic domain - #27802

Open
daviszhen wants to merge 11 commits into
matrixorigin:mainfrom
daviszhen:revert-8b0a5ff873-tpcc-regression
Open

fix(frontend): reuse prepared DML specialization by semantic domain#27802
daviszhen wants to merge 11 commits into
matrixorigin:mainfrom
daviszhen:revert-8b0a5ff873-tpcc-regression

Conversation

@daviszhen

@daviszhen daviszhen commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

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.

  • retains runtime type specialization and DML write-expression preservation;
  • caches the specialized plan and Compile for stable prepared DML parameter domains instead of deep-copying, rebinding, and recompiling on every execution;
  • preserves ParamRef provenance in cached plans so a cache hit reads the current execution values rather than values from the first execution;
  • separates generic, numeric-prefix, numeric-overload, and direct-result modes in the cache key, including binary-protocol provenance and the canonical runtime domain;
  • misses the cache and rebuilds when the runtime semantic domain changes;
  • carries the same specialization mode through compile retries;
  • merges the latest upstream main at 4cbdc2cf46 and integrates its prepared numeric-overload path.

Validation

  • go test ./pkg/frontend ./pkg/sql/plan ./pkg/sql/plan/function ./pkg/sql/colexec -count=1
  • go test ./pkg/tests/dml -run '^TestForcedMultiCNDeleteAndInsertIgnore$' -count=1
  • go test ./pkg/tests/issues -run '^TestIssue27443BinaryPreparedDMLAndAggregate$' -count=1
  • focused runtime cache, semantic-key, numeric-overload, and compile-retry tests
  • go vet ./pkg/frontend ./pkg/sql/plan ./pkg/sql/plan/function ./pkg/sql/colexec ./pkg/tests/dml ./pkg/tests/issues
  • golangci-lint run -c .golangci.yml ./pkg/frontend
  • BenchmarkBinaryDMLRuntimeSpecializationCache: 9.1 us/op, 1496 B/op, 26 allocs/op (100 iterations on the local validation host)
  • git diff --check

…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-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@daviszhen daviszhen changed the title revert: prepared DML and aggregate runtime specialization fix(frontend): keep prepared DML on cached execution path Aug 28, 2026
@matrix-meow matrix-meow added size/S Denotes a PR that changes [10,99] lines and removed size/XXL Denotes a PR that changes 2000+ lines labels Aug 28, 2026
@mergify mergify Bot added the kind/test-ci label Aug 28, 2026
…pcc-regression

# Conflicts:
#	pkg/frontend/computation_wrapper.go
#	pkg/frontend/computation_wrapper_test.go

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep review completed on exact head 09d6dc1. Two blockers remain.

  1. [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 runtimeCacheTarget at computation_wrapper.go:1497-1527. createCompile then builds the candidate with the background cwft.proc (lines 544-558), and a successful Run publishes it into the upstream/client PrepareStmt (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.Reset changes only c.proc; Scope.resetForReuse keeps each materialized s.Proc and 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 for executionSes.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).

  2. [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 measures initExecuteStmtParam cache 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
@daviszhen

Copy link
Copy Markdown
Contributor Author

Addressed the background-process ownership blocker and merged current main on exact head ec37a28b46.

Changes:

  • Runtime specialization caching is now disabled at the background/procedure execution boundary. A background EXECUTE neither consumes the client PrepareStmt.runtimeCompile nor stages/publishes a candidate built with the background Process.
  • Added TestBackgroundPreparedDMLDoesNotConsumeOrPublishClientRuntimeCompile, covering:
    1. a background DML category miss with no existing client category;
    2. a background same-domain execution with an existing client category;
    3. a background type switch attempting to replace that category;
    4. a subsequent foreground execution still reusing the original client Compile/plan.
  • Resolved the main merge conflicts while retaining the direct-result runtime-position fix from fix: restore direct prepared numeric result metadata #27557.
  • Extended BenchmarkBinaryDMLRuntimeSpecializationCache with stable-domain and type-switch lanes.

Local validation on the exact head:

  • full go test ./pkg/frontend -count=1
  • TestIssue26725PreparedBit64Numeric
  • TestIssue27443BinaryPreparedDMLAndAggregate
  • TestForcedMultiCNDeleteAndInsertIgnore
  • affected-package go vet
  • frontend golangci-lint: 0 issues

Repeated local benchmark (-benchmem -count=3):

  • stable domain: 6.69–6.77 us/op, 1496 B/op, 26 allocs/op
  • type switch: 33.63–33.65 us/op, 11512 B/op, 141 allocs/op

I also attempted to dispatch the same 128-runner/full/5-minute TPCC workflow for exact head ec37a28b46, but this GitHub account has read-only access to matrixorigin/mo-nightly-regression; workflow dispatch returns HTTP 404. The exact requested dispatch is:

  • workflow: MO Branch Nightly Regression On 128
  • Repo: daviszhen/matrixone
  • Branch: ec37a28b46
  • Run_Mode/Nightly_Level/Case_Duration: full/full/5

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 XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 aunjgr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 aptend left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working kind/enhancement kind/test-ci size/L Denotes a PR that changes [500,999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants