fix: preserve prepared binary params across retries#25872
Conversation
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? |
gouhongshen
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
# Conflicts: # pkg/pb/pipeline/pipeline.pb.go # pkg/sql/compile/compile_test.go
What type of PR is this?
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 sameProcess, while releasing anintermediate retry
Compilealso callsProcess.Free(). That cleared theparameters 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:
pkg/container/vector,pkg/vm/process,pkg/sql/colexec,pkg/sql/plan,pkg/sql/compile, andpkg/frontendmake buildmake static-check(0 issues)