Skip to content

fix: preserve prepared binary params across retries#25872

Open
ck89119 wants to merge 14 commits into
matrixorigin:mainfrom
ck89119:issue-25421-main
Open

fix: preserve prepared binary params across retries#25872
ck89119 wants to merge 14 commits into
matrixorigin:mainfrom
ck89119:issue-25421-main

Conversation

@ck89119

@ck89119 ck89119 commented Jul 20, 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 #25421

What this PR does / why we need it:

Preserves per-parameter binary metadata for user variables passed through SQL
EXECUTE ... USING, including local expression evaluation, plan substitution,
deep copies, and remote CN process serialization.

The previous implementation released owned prepared parameters from
Process.Free(). Compile retries reuse the same Process, while releasing an
intermediate retry Compile also calls Process.Free(). That cleared the
parameters before the next retry and could cause a nil pointer panic under
TPCC workloads.

This change detaches the complete prepared-parameter state before releasing an
intermediate retry compile and restores it afterward. The transferred state
includes the parameter vector, per-parameter binary flags, and ownership.
Normal final cleanup still releases owned vectors exactly once.

Validation:

  • Full tests for pkg/container/vector, pkg/vm/process, pkg/sql/colexec,
    pkg/sql/plan, pkg/sql/compile, and pkg/frontend
  • Focused race tests for repeated retry release and parameter ownership
  • Package build and vet for all affected packages
  • make build
  • make static-check (0 issues)
  • Prepared binary parameter BVT: 29/29 statements passed

@mergify mergify Bot added the kind/bug Something isn't working label Jul 20, 2026
@matrix-meow matrix-meow added the size/L Denotes a PR that changes [500,999] lines label Jul 20, 2026
@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 →

@gouhongshen gouhongshen 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.

Codex automated review

No blocking or non-blocking defects found. Binary metadata is preserved through execution, plan copying, remote serialization, and retry ownership; final cleanup releases owned parameters. Diff check passed and protobuf tests passed; CGo-transitive focused tests were unavailable locally because cgo/libmo.dylib is absent.

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

P1: Negotiate binary-parameter metadata before remote execution.

The new PrepareParamInfo.is_bin field is additive, so an older CN silently ignores it. That older worker then evaluates ParamExpression without setting Vector.IsBin and treats a binary prepared parameter as text. A new coordinator can still schedule such a query to an old CN during a rolling upgrade, producing incorrect indexed or scanned comparison results with no error.

Please gate remote placement on worker protocol capability whenever any prepared parameter has a binary flag, or preserve the semantics through a representation understood by old workers. Add a new-coordinator to old-worker compatibility regression; decoding an old payload on a new worker covers only the opposite direction.

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

No blocking findings on the latest head.

This PR mainly improves prepared-parameter lifecycle correctness across retries and remote execution: it preserves the owned parameter vector together with per-parameter binary metadata (is_bin) when an intermediate retry Compile is released, restores that state before the next retry, and keeps final cleanup exactly-once. The same binary metadata now survives local expression evaluation, plan/literal substitution, deep copies, user-variable resolution for EXECUTE ... USING, and remote CN process serialization / reconstruction.

I did not find a new correctness, ownership, or unhappy-path blocker on the updated diff.

# Conflicts:
#	pkg/pb/pipeline/pipeline.pb.go

@gouhongshen gouhongshen 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.

Codex automated review

The retry ownership logic itself is sound, but two blocking shard-RPC issues remain: decoded parameters leak, and rolling upgrades can silently lose binary metadata. Focused local tests could not build because required native headers/libraries are absent; current remote CI is otherwise green except for a cancelled multi-CN compose job.

P1 - Release owned parameters decoded for shard reads (pkg/vm/process/process_codec.go:227)

Decode now copies prepared-parameter data into the long-lived codec mpool and marks the vector owned. Its production caller, getTxnTable, is invoked by every shard handler—including each ReadNext request—but neither it nor the handlers call proc.Free; error exits after Decode also omit cleanup. Mpool allocations remain registered until Vector.Free, so repeated prepared shard reads retain their data/area and allocator entries indefinitely. Establish an explicit process lifetime covering normal, error, and reader-lifetime paths.

P1 - Guard binary metadata on shard RPCs during rolling upgrades (pkg/vm/process/process_codec.go:83)

The CommitID filtering cited in the earlier review reply protects query-worker discovery, but shard reads bypass it: txnTableDelegate embeds BuildProcessInfo in shard.ReadParam, and shardservice routes directly to ShardReplica.CN without version filtering. A new CN can therefore send is_bin to an old shard replica, whose protobuf decoder silently drops field 5; remote Ranges/ReaderBuild then evaluates prepared binary filters as text, potentially pruning matching blocks and returning wrong results. The mixed-version test covers scheduled query workers only. Shard routing needs capability/version gating or a representation understood by old handlers.

@gouhongshen gouhongshen 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.

Codex automated review

A blocking resource leak exists in the new mixed-version compatibility rejection. Earlier mixed-version correctness concerns are addressed by the current routing guard. Focused tests could not compile because required CGo headers are unavailable.

P1 - Compatibility rejection leaks already-sent RPC futures (pkg/shardservice/service_read.go:144)

Read creates futures and defers only futureSlice.close(), which merely clears pooled references; it does not call Future.Close(). The normal success path closes each future after Get(). If an earlier compatible shard has already been sent and a later shard fails validateRemoteReadCompatibility here, Read returns before draining or closing the earlier future. That future remains registered in morpc's backend future map, and its response/resources can remain retained. Repeated multi-shard binary reads with mixed target versions can therefore leak active futures and response buffers. Validate all remote targets before sending, or explicitly close/drain every already-created future on this error path.

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 size/XL Denotes a PR that changes [1000, 1999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants