Skip to content

fix(plan): sanitize non-finite plan stats - #24643

Merged
XuPeng-SH merged 6 commits into
matrixorigin:mainfrom
ck89119:issue-24520
May 28, 2026
Merged

fix(plan): sanitize non-finite plan stats#24643
XuPeng-SH merged 6 commits into
matrixorigin:mainfrom
ck89119:issue-24520

Conversation

@ck89119

@ck89119 ck89119 commented May 27, 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 #24520

What this PR does / why we need it:

  • Guard selectivity and stats ratio calculations against zero or non-finite denominators so plan stats do not produce +Inf or NaN.
  • Sanitize non-finite float values before plan JSON serialization as a final protection against json: unsupported value: +Inf.
  • Add regression tests for zero Cost/TableCnt stats paths and marshal-plan non-finite stats.

Test Plan

  • go test ./pkg/sql/plan -run 'TestSafeStatsRatiosAvoidNonFiniteSelectivity|TestSerializePlanToJson|TestDMLToJson' -count=1
  • go test ./pkg/frontend -run 'TestSerializePlanToJson|TestMarshalPlanHandlerSanitizesNonFinitePlanStats' -count=1
  • go test ./pkg/sql/plan -count=1
  • go test ./pkg/frontend -count=1
  • git diff --check
  • make
  • make static-check

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

@XuPeng-SH
XuPeng-SH merged commit b2bed28 into matrixorigin:main May 28, 2026
0 of 3 checks passed
@ck89119
ck89119 deleted the issue-24520 branch May 28, 2026 11:46
XuPeng-SH added a commit that referenced this pull request May 28, 2026
## What type of PR is this?

- [ ] API-change
- [x] BUG
- [ ] Improvement
- [ ] Documentation
- [ ] Feature
- [ ] Test and CI
- [ ] Code Refactoring

## Which issue(s) this PR fixes:

issue #24520

## What this PR does / why we need it:

Cherry-pick PR #24643 to 4.0-dev.

This PR prevents non-finite planner statistics from leaking into plan
JSON and cost estimation paths by:

- guarding stats ratio calculations against division by zero and
non-finite results;
- sanitizing non-finite float values before plan JSON marshaling;
- clamping expression and join selectivity to the valid [0, 1] range;
- clamping ANTI JOIN right-side selectivity before computing output
rows;
- adding regression coverage for zero stats, year(...) equality
selectivity, NOT/JOIN propagation, ANTI JOIN, and JSON marshal
sanitization.

---------

Co-authored-by: XuPeng-SH <xupeng3112@163.com>
@XuPeng-SH

XuPeng-SH commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Correction after tracing the timeline for #26175:

#24643 added the in-place reflective sanitizer, but it was not the recent trigger by itself. The regression was introduced by #25843 (merge commit b8a190a, 2026-07-23), which changed NewJsonPlanHandler from eagerly calling h.Marshal(ctx) to retaining marshalHandler and invoking Marshal later during asynchronous statement export.

The retained ExplainData still shallow-copies PhyPlan, so PhyOperator.OpStats remains shared with the compile graph. The execution path calls RecordExecPlan and then Compile.Release(); release/reuse can reset OperatorStats (including OperatorMetrics) before deferred Marshal runs. The sanitizer from #24643 then writes the stale/shared map through SetMapIndex. A reset/reuse between IsNil and SetMapIndex yields assignment to entry in nil map.

Root cause: #25843 introduced deferred serialization without snapshotting the shared PhyPlan. #24643 is the pre-existing unsafe mutator that makes the ownership violation visible. Fix: deep-copy PhyPlan (including OperatorStats maps and BackgroundQueries) before retaining it for deferred marshal, and add a regression test that releases/resets the source plan before Marshal.

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/M Denotes a PR that changes [100,499] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants