planner, executor, parser: support scoped FLUSH STATS_DELTA#68159
Open
0xPoe wants to merge 8 commits intopingcap:release-8.5from
Open
planner, executor, parser: support scoped FLUSH STATS_DELTA#681590xPoe wants to merge 8 commits intopingcap:release-8.5from
0xPoe wants to merge 8 commits intopingcap:release-8.5from
Conversation
…ingcap#65677) close pingcap#65668 (cherry picked from commit b268bfa)
ti-chi-bot
Bot
added
do-not-merge/cherry-pick-not-approved
release-note
|
Skipping CI for Draft Pull Request. |
1 similar comment
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
size/XXL
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a scoped FLUSH STATS_DELTA statement (parser, AST, dedup, planner checks, executor dispatch) that expands targets to table/partition IDs and calls StatsHandle.DumpStatsDeltaToKV(forceDump, tableIDs...). Also threads statement Context through executor-builder plumbing used by analyze/flush flows and updates related tests. ChangesFLUSH STATS_DELTA Feature
Executor Builder & Context Plumbing
Sequence DiagramsequenceDiagram
participant User as User
participant Parser as Parser
participant Planner as Planner
participant Executor as Executor
participant StatsHandle as StatsHandle
participant KV as KV
User->>Parser: FLUSH STATS_DELTA db.t1, db.*
Parser-->>Planner: FlushStmt{Tp:FlushStatsDelta, FlushObjects:[...], IsCluster:false}
Planner->>Planner: fill default DBs & dedup targets
Planner->>Planner: append SELECT privilege checks per scope
Planner-->>Executor: validated flush execution plan
Executor->>Executor: executeFlushStatsDelta(ctx, stmt)
Executor->>Executor: collect table/partition IDs (warn/skip missing)
Executor->>StatsHandle: DumpStatsDeltaToKV(true, tableIDs...)
StatsHandle->>KV: persist delta entries for specified tableIDs
KV-->>StatsHandle: ack
StatsHandle-->>Executor: success
Executor-->>User: FLUSH completed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
pkg/parser/parser_test.go (1)
1456-1468: ⚡ Quick winAdd missing
CLUSTERcoverage for table-only and mixed list forms inFLUSH STATS_DELTAparser tests.You already test
... CLUSTERfor*.*,db1.*,db1.t1, anddb1.t1, db2.*, plus the ambiguousflush stats_delta clustervsflush stats_delta cluster clusterbehavior. What’s missing is:
flush stats_delta t1 cluster(table-only target + trailingCLUSTER)- a mixed list like
flush stats_delta db1.t1, t2(first element isdb.tbl, second istbl)This is low-effort and should catch regressions in the remaining grammar branches.
Suggested diff (add test cases)
@@ {"flush stats_delta db1.t1", true, "FLUSH STATS_DELTA `db1`.`t1`"}, + {"flush stats_delta t1 cluster", true, "FLUSH STATS_DELTA `t1` CLUSTER"}, {"flush stats_delta db1.t1 cluster", true, "FLUSH STATS_DELTA `db1`.`t1` CLUSTER"}, {"flush stats_delta db1.t1, db2.*", true, "FLUSH STATS_DELTA `db1`.`t1`, `db2`.*"}, + {"flush stats_delta db1.t1, t2", true, "FLUSH STATS_DELTA `db1`.`t1`, `t2`"}, {"flush stats_delta db1.t1, db2.* cluster", true, "FLUSH STATS_DELTA `db1`.`t1`, `db2`.* CLUSTER"},As per coding guidelines: “Keep test changes minimal and deterministic; avoid broad golden/testdata churn unless required.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/parser_test.go` around lines 1456 - 1468, Add missing parser test cases in pkg/parser/parser_test.go's FLUSH STATS_DELTA table of cases: insert entries for the table-only CLUSTER form and the mixed db.table + table list (with optional CLUSTER). Specifically add cases analogous to the existing rows such as "flush stats_delta t1 cluster" (expect success and "FLUSH STATS_DELTA `t1` CLUSTER"), "flush stats_delta db1.t1, t2" (expect success and "FLUSH STATS_DELTA `db1`.`t1`, `t2`") and the mixed-list CLUSTER variant "flush stats_delta db1.t1, t2 cluster" (expect success and "FLUSH STATS_DELTA `db1`.`t1`, `t2` CLUSTER") so the parser branches handling table-only targets and mixed lists are covered.pkg/planner/core/logical_plans_test.go (1)
1437-1459: ⚡ Quick winAvoid relying on non-existent
test.ttttargets for robustness.The added cases use
ttt(e.g.,flush stats_delta ttt,flush stats_delta test.ttt, ...) even though this file already has a TODO notingtest.tttdoesn’t exist in the mocked InfoSchema (Line 1544). If future planner changes start validating target existence during planning (even for visitInfo collection), these tests may start failing for reasons unrelated to the privilege scoping logic.To make the tests more resilient, consider using an existing mocked table like
t.Suggested patch (use `t` instead of `ttt`)
@@ { - sql: "flush stats_delta ttt", + sql: "flush stats_delta t", ans: []visitInfo{ - {mysql.SelectPriv, "test", "ttt", "", nil, false, nil, false}, + {mysql.SelectPriv, "test", "t", "", nil, false, nil, false}, }, }, { - sql: "flush stats_delta test.ttt, test.*", + sql: "flush stats_delta test.t, test.*", ans: []visitInfo{ {mysql.SelectPriv, "test", "", "", nil, false, nil, false}, }, },As per coding guidelines, keep test changes minimal and deterministic; avoid making assertions depend on brittle fixture gaps.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/planner/core/logical_plans_test.go` around lines 1437 - 1459, The tests reference a non-existent table name "ttt" which makes them brittle; update the failing test cases to use the existing mocked table "t" instead of "ttt" so planner validations won't fail unpredictably—specifically, change the SQL strings "flush stats_delta ttt", "flush stats_delta test.ttt, test.*" (and any similar occurrences) to "flush stats_delta t", "flush stats_delta test.t, test.*" and update the corresponding visitInfo entries (the table identifier fields in the ans slices) from "ttt" to "t" so visitInfo assertions remain consistent; the affected symbols are the test cases in logical_plans_test.go and the visitInfo structs used in those cases.pkg/executor/test/simpletest/simple_test.go (1)
874-876: ⚡ Quick winMake negative scoped assertions exact (not just “!= 3”).
These checks can still pass if an unintended partial flush writes another value (e.g.,
1/2). For this scope test, assert the unrelated target remains absent (or unchanged from a captured pre-value) to avoid false positives.Proposed assertion tightening
- require.NotEqual(t, int64(3), getModifyCount(t2.Meta().ID), "unrelated table should not be flushed by table scope") - require.NotEqual(t, int64(3), getModifyCount(tp.Meta().ID), "partitioned table should not be flushed by unrelated table scope") + require.Equal(t, int64(-1), getModifyCount(t2.Meta().ID), "unrelated table should remain unflushed") + require.Equal(t, int64(-1), getModifyCount(tp.Meta().ID), "unrelated partitioned table should remain unflushed") ... - require.NotEqual(t, int64(3), getModifyCount(t2.Meta().ID), "database scope has not been flushed yet") + require.Equal(t, int64(-1), getModifyCount(t2.Meta().ID), "database scope has not been flushed yet")Also applies to: 881-881
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/test/simpletest/simple_test.go` around lines 874 - 876, Capture the pre-flush modify counts and assert exact equality instead of NotEqual: call getModifyCount for the unrelated table and partitioned table before the flush (store as e.g. preCountT2 and preCountTp) and after the flush assert getModifyCount(t2.Meta().ID) == preCountT2 and getModifyCount(tp.Meta().ID) == preCountTp (replace the current require.NotEqual assertions); also update the analogous assertion near the other occurrence (line ~881) the same way. This uses the existing getModifyCount, t2.Meta().ID and tp.Meta().ID symbols to ensure the counts are unchanged.pkg/parser/ast/stats.go (1)
412-447: ⚡ Quick winAdd doc comments for the new exported AST types.
StatsObjectScopeTypeandStatsObjectare exported but undocumented, which makes the newFLUSH STATS_DELTAtarget model harder to discover fromgo docand editor hovers.Suggested fix
+// StatsObjectScopeType identifies the scope of a FLUSH STATS_DELTA target. type StatsObjectScopeType int @@ +// StatsObject identifies a FLUSH STATS_DELTA target at table, database, or global scope. type StatsObject struct { StatsObjectScope StatsObjectScopeType DBName model.CIStr TableName model.CIStr }As per coding guidelines, "Keep exported-symbol doc comments".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/parser/ast/stats.go` around lines 412 - 447, Add Go doc comments for the exported symbols introduced: document the StatsObjectScopeType type (describe what it represents and the possible values), each exported constant (StatsObjectScopeTable, StatsObjectScopeDatabase, StatsObjectScopeGlobal) with brief meaning, and the StatsObject struct (explain fields DBName, TableName and StatsObjectScope). Also add a short comment for the Restore method on StatsObject describing its purpose (serializing the object for SQL restore). Place these comments immediately above the corresponding type/const/struct/method declarations to satisfy go doc and editor hover tooling.pkg/executor/simple.go (1)
2778-2783: ⚡ Quick winKeep the dedup assertion side-effect free.
This assertion mutates
s.FlushObjectsviaDedupFlushObjects(). In intest builds that changes executor state while validating planner output, which makes test behavior diverge from production. Compare against a cloned statement instead.Suggested fix
intest.AssertFunc(func() bool { - origCount := len(s.FlushObjects) - s.DedupFlushObjects() - return origCount == len(s.FlushObjects) + cloned := &ast.FlushStmt{ + FlushObjects: append([]*ast.StatsObject(nil), s.FlushObjects...), + } + cloned.DedupFlushObjects() + return len(cloned.FlushObjects) == len(s.FlushObjects) }, "FlushObjects should be deduplicated in the building phase")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/simple.go` around lines 2778 - 2783, The current intest.AssertFunc calls s.DedupFlushObjects() which mutates executor state; instead make the assertion side-effect free by cloning the FlushObjects slice (or a shallow copy of the statement) and calling DedupFlushObjects on that clone, then assert origCount == len(clone.FlushObjects); update the intest.AssertFunc to use the cloned variable and still reference s.FlushObjects, s.DedupFlushObjects(), and intest.AssertFunc so the assertion verifies deduplication without mutating the original statement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/planner/core/planbuilder.go`:
- Around line 5025-5029: requireSelectPrivForStatsObjects currently
early-returns when objects is empty, which bypasses privilege checks in
non-assert builds; change it to treat an empty objects slice as global-scope
stats access (or return an explicit error) so visitInfo is still added and
checks run. Specifically, in requireSelectPrivForStatsObjects, replace the
early-return branch with logic that creates/appends a visitInfo representing
global scope access (or returns a descriptive error) and then proceeds with the
same privilege-enforcement flow used for non-empty objects; ensure you reference
and populate the same structures used elsewhere in this function (visitInfo and
any privilege-checking helpers) so enforcement is not skipped.
---
Nitpick comments:
In `@pkg/executor/simple.go`:
- Around line 2778-2783: The current intest.AssertFunc calls
s.DedupFlushObjects() which mutates executor state; instead make the assertion
side-effect free by cloning the FlushObjects slice (or a shallow copy of the
statement) and calling DedupFlushObjects on that clone, then assert origCount ==
len(clone.FlushObjects); update the intest.AssertFunc to use the cloned variable
and still reference s.FlushObjects, s.DedupFlushObjects(), and intest.AssertFunc
so the assertion verifies deduplication without mutating the original statement.
In `@pkg/executor/test/simpletest/simple_test.go`:
- Around line 874-876: Capture the pre-flush modify counts and assert exact
equality instead of NotEqual: call getModifyCount for the unrelated table and
partitioned table before the flush (store as e.g. preCountT2 and preCountTp) and
after the flush assert getModifyCount(t2.Meta().ID) == preCountT2 and
getModifyCount(tp.Meta().ID) == preCountTp (replace the current require.NotEqual
assertions); also update the analogous assertion near the other occurrence (line
~881) the same way. This uses the existing getModifyCount, t2.Meta().ID and
tp.Meta().ID symbols to ensure the counts are unchanged.
In `@pkg/parser/ast/stats.go`:
- Around line 412-447: Add Go doc comments for the exported symbols introduced:
document the StatsObjectScopeType type (describe what it represents and the
possible values), each exported constant (StatsObjectScopeTable,
StatsObjectScopeDatabase, StatsObjectScopeGlobal) with brief meaning, and the
StatsObject struct (explain fields DBName, TableName and StatsObjectScope). Also
add a short comment for the Restore method on StatsObject describing its purpose
(serializing the object for SQL restore). Place these comments immediately above
the corresponding type/const/struct/method declarations to satisfy go doc and
editor hover tooling.
In `@pkg/parser/parser_test.go`:
- Around line 1456-1468: Add missing parser test cases in
pkg/parser/parser_test.go's FLUSH STATS_DELTA table of cases: insert entries for
the table-only CLUSTER form and the mixed db.table + table list (with optional
CLUSTER). Specifically add cases analogous to the existing rows such as "flush
stats_delta t1 cluster" (expect success and "FLUSH STATS_DELTA `t1` CLUSTER"),
"flush stats_delta db1.t1, t2" (expect success and "FLUSH STATS_DELTA
`db1`.`t1`, `t2`") and the mixed-list CLUSTER variant "flush stats_delta db1.t1,
t2 cluster" (expect success and "FLUSH STATS_DELTA `db1`.`t1`, `t2` CLUSTER") so
the parser branches handling table-only targets and mixed lists are covered.
In `@pkg/planner/core/logical_plans_test.go`:
- Around line 1437-1459: The tests reference a non-existent table name "ttt"
which makes them brittle; update the failing test cases to use the existing
mocked table "t" instead of "ttt" so planner validations won't fail
unpredictably—specifically, change the SQL strings "flush stats_delta ttt",
"flush stats_delta test.ttt, test.*" (and any similar occurrences) to "flush
stats_delta t", "flush stats_delta test.t, test.*" and update the corresponding
visitInfo entries (the table identifier fields in the ans slices) from "ttt" to
"t" so visitInfo assertions remain consistent; the affected symbols are the test
cases in logical_plans_test.go and the visitInfo structs used in those cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0d426b66-ea4c-4e14-bb4e-cbbcb805bfe0
📒 Files selected for processing (15)
pkg/executor/simple.gopkg/executor/test/simpletest/simple_test.gopkg/parser/ast/misc.gopkg/parser/ast/stats.gopkg/parser/keywords.gopkg/parser/keywords_test.gopkg/parser/misc.gopkg/parser/parser.gopkg/parser/parser.ypkg/parser/parser_test.gopkg/planner/core/logical_plans_test.gopkg/planner/core/planbuilder.gopkg/statistics/handle/types/interfaces.gopkg/statistics/handle/updatetest/update_test.gopkg/statistics/handle/usage/session_stats_collect.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #68159 +/- ##
================================================
Coverage ? 55.0778%
================================================
Files ? 1823
Lines ? 655816
Branches ? 0
================================================
Hits ? 361209
Misses ? 267766
Partials ? 26841
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
0xPoe
force-pushed
the
backport-flush-stats-delta-release-8.5
branch
3 times, most recently
from
cef4605 to
5a5d3e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/executor/builder.go (1)
3098-3112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle the
statsTbl == nil && hasPDcase before dereferencingRealtimeCount.If
GetPhysicalTableStats(...)returnsnilbut PD still returns an approximate count, this function falls through and later readsstatsTbl.RealtimeCount, which will panic on the ANALYZE path. Please add an explicit fallback for thestatsTbl == nil/hasPD == truebranch instead of continuing.🛠️ Suggested fix
func (b *executorBuilder) getAdjustedSampleRate(ctx context.Context, task plannercore.AnalyzeColumnsTask) (sampleRate float64, reason string) { statsHandle := domain.GetDomain(b.sctx).StatsHandle() defaultRate := 0.001 if statsHandle == nil { return defaultRate, fmt.Sprintf("statsHandler is nil, use the default-rate=%v", defaultRate) } var statsTbl *statistics.Table tid := task.TableID.GetStatisticsID() if tid == task.TblInfo.ID { statsTbl = statsHandle.GetPhysicalTableStats(task.TblInfo.ID, task.TblInfo) } else { statsTbl = statsHandle.GetPhysicalTableStats(tid, task.TblInfo) } approxiCount, hasPD := b.getApproximateTableCountFromStorage(ctx, tid, task) - // If there's no stats meta and no pd, return the default rate. - if statsTbl == nil && !hasPD { - return defaultRate, fmt.Sprintf("TiDB cannot get the row count of the table, use the default-rate=%v", defaultRate) + if statsTbl == nil { + if !hasPD { + return defaultRate, fmt.Sprintf("TiDB cannot get the row count of the table, use the default-rate=%v", defaultRate) + } + sampleRate = math.Min(1, config.DefRowsForSampleRate/approxiCount) + return sampleRate, fmt.Sprintf( + "stats_meta is missing, use min(1, %v/%v) as the sample-rate=%v", + config.DefRowsForSampleRate, approxiCount, sampleRate, + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/builder.go` around lines 3098 - 3112, In getAdjustedSampleRate, avoid dereferencing statsTbl when GetPhysicalTableStats returns nil: add an explicit branch for the case statsTbl == nil && hasPD (using approxiCount) before any access to statsTbl.RealtimeCount; for example, return the defaultRate (or compute a sensible sampleRate based on approxiCount) and a reason string mentioning that statsTbl is nil but PD provided approxiCount, so we fall back to default/PD-based sampling instead of continuing and causing a panic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/executor/coprocessor.go`:
- Around line 203-204: Change buildDAGExecutor to accept a request-scoped
context and thread it into newExecutorBuilder so cancellation/deadlines
propagate: update the buildDAGExecutor signature to add ctx context.Context,
replace context.Background() with that ctx when calling newExecutorBuilder, and
update the two callers HandleRequest and HandleStreamRequest to pass their
request context into buildDAGExecutor; ensure any downstream uses of
executorBuilder/newExecutorBuilder align with the new parameter.
In `@pkg/executor/index_merge_reader.go`:
- Around line 193-195: The call to distsql.IndexRangesToKVRanges currently
ignores its error return, which can leave invalid key ranges; change the call to
capture the error (e.g., keyRange, err :=
distsql.IndexRangesToKVRanges(e.sctx.GetDistSQLCtx(), e.table.Meta().ID, idx.ID,
e.ranges[i])) and if err != nil propagate it immediately (return or wrap and
return) instead of discarding it, then only assign e.partitionKeyRanges[i] =
[][]kv.KeyRange{keyRange.FirstPartitionRange()} when err == nil.
---
Outside diff comments:
In `@pkg/executor/builder.go`:
- Around line 3098-3112: In getAdjustedSampleRate, avoid dereferencing statsTbl
when GetPhysicalTableStats returns nil: add an explicit branch for the case
statsTbl == nil && hasPD (using approxiCount) before any access to
statsTbl.RealtimeCount; for example, return the defaultRate (or compute a
sensible sampleRate based on approxiCount) and a reason string mentioning that
statsTbl is nil but PD provided approxiCount, so we fall back to
default/PD-based sampling instead of continuing and causing a panic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c1a2a444-7e69-4a25-b001-cc9054647d85
📒 Files selected for processing (22)
pkg/executor/adapter.gopkg/executor/analyze.gopkg/executor/analyze_col_v2.gopkg/executor/analyze_test.gopkg/executor/analyze_utils_test.gopkg/executor/benchmark_test.gopkg/executor/brie.gopkg/executor/builder.gopkg/executor/coprocessor.gopkg/executor/executor_required_rows_test.gopkg/executor/foreign_key.gopkg/executor/index_merge_reader.gopkg/executor/point_get.gopkg/executor/select.gopkg/executor/test/analyzetest/analyze_test.gopkg/planner/cardinality/selectivity_test.gopkg/statistics/handle/globalstats/global_stats_test.gopkg/statistics/handle/handletest/handle_test.gopkg/statistics/handle/storage/dump_test.gopkg/statistics/handle/storage/gc_test.gopkg/testkit/testkit.gopkg/ttl/ttlworker/job_manager_integration_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/statistics/handle/globalstats/global_stats_test.go
0xPoe
force-pushed
the
backport-flush-stats-delta-release-8.5
branch
from
5a5d3e8 to
02eaee9
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/executor/simple.go`:
- Around line 2785-2835: The code only treats StatsObjectScopeGlobal as global
when it's the sole entry; change the logic so any presence of a flushObject with
StatsObjectScopeGlobal causes the function to take the global path: inspect
s.FlushObjects for any flushObject.StatsObjectScope ==
ast.StatsObjectScopeGlobal (e.g. set isGlobalScope = true if found) before
iterating, or handle that case early in the loop by immediately returning the
global DumpStatsDeltaToKV(true) path; keep references to s.FlushObjects,
isGlobalScope, StatsObjectScopeGlobal, appendStatsDeltaTargetTableIDs and the
final domain.GetDomain(e.Ctx()).StatsHandle().DumpStatsDeltaToKV(true,
targetIDs...) so the global scope always triggers flushing all deltas even when
mixed with other scopes.
In `@pkg/executor/test/simpletest/simple_test.go`:
- Around line 872-885: The negative scope checks use require.NotEqual(..., 3)
which can hide incorrect non-3 changes; instead capture the exact pre-flush
modify counts and assert equality against those known values. Before calling
tk.MustExec("flush stats_delta t1") (or before each flush), record preT2 :=
getModifyCount(t2.Meta().ID), preTp := getModifyCount(tp.Meta().ID) and any
partition counts (preP0 := getModifyCount(p0ID), preP1 := getModifyCount(p1ID))
and then replace the require.NotEqual assertions with require.Equal(t, preT2,
getModifyCount(t2.Meta().ID)) and require.Equal(t, preTp,
getModifyCount(tp.Meta().ID)) (and likewise for partitions) so the test verifies
exact pre-flush state using getModifyCount, t2.Meta().ID, tp.Meta().ID, p0ID and
p1ID.
In `@pkg/planner/core/planbuilder.go`:
- Around line 3819-3823: The code mutates the original FlushStmt AST via
fillDefaultDBForStatsObjects and DedupFlushObjects on raw.FlushObjects, which
can corrupt the shared Simple.Statement pointer; instead clone the AST or the
FlushObjects slice before modifying it and use that clone for
fillDefaultDBForStatsObjects, DedupFlushObjects, and
requireSelectPrivForStatsObjects so the original raw (and Simple.Statement)
remain immutable. Locate use of raw.FlushObjects and replace in-place
modifications with operations on a copied slice or a cloned FlushStmt (e.g., new
variable like clonedFlush or clonedStmt) and pass that to
fillDefaultDBForStatsObjects, DedupFlushObjects, and
requireSelectPrivForStatsObjects. Ensure no changes are written back to
raw.FlushObjects.
In `@pkg/statistics/handle/storage/gc_test.go`:
- Around line 93-99: The FIXME comment is incomplete: update the comment near
the assertions in gc_test.go to state that the extra mysql.stats_meta row is
introduced by the DumpStatsDeltaToKV change (it now writes the
logical/non-partition table meta row in addition to partition rows), and that
GCStats(dom.InfoSchema(), ddlLease) still cannot remove that logical row due to
bug `#68076`; mention that this causes a permanent orphan per dropped partitioned
table in production until `#68076` is fixed and reference DumpStatsDeltaToKV,
mysql.stats_meta, and GCStats to make the root cause and impact explicit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c08ddc30-aa90-42ce-b7f1-3d637cd331af
📒 Files selected for processing (33)
pkg/executor/adapter.gopkg/executor/analyze.gopkg/executor/analyze_col_v2.gopkg/executor/analyze_test.gopkg/executor/analyze_utils_test.gopkg/executor/benchmark_test.gopkg/executor/brie.gopkg/executor/builder.gopkg/executor/coprocessor.gopkg/executor/executor_required_rows_test.gopkg/executor/foreign_key.gopkg/executor/index_merge_reader.gopkg/executor/point_get.gopkg/executor/select.gopkg/executor/simple.gopkg/executor/test/analyzetest/analyze_test.gopkg/executor/test/simpletest/BUILD.bazelpkg/executor/test/simpletest/simple_test.gopkg/parser/ast/stats.gopkg/parser/parser.gopkg/parser/parser.ypkg/parser/parser_test.gopkg/planner/cardinality/selectivity_test.gopkg/planner/core/logical_plans_test.gopkg/planner/core/planbuilder.gopkg/statistics/handle/globalstats/global_stats_test.gopkg/statistics/handle/handletest/handle_test.gopkg/statistics/handle/storage/dump_test.gopkg/statistics/handle/storage/gc_test.gopkg/statistics/handle/types/interfaces.gopkg/statistics/handle/usage/session_stats_collect.gopkg/testkit/testkit.gopkg/ttl/ttlworker/job_manager_integration_test.go
✅ Files skipped from review due to trivial changes (11)
- pkg/planner/cardinality/selectivity_test.go
- pkg/executor/executor_required_rows_test.go
- pkg/statistics/handle/types/interfaces.go
- pkg/executor/analyze_col_v2.go
- pkg/executor/index_merge_reader.go
- pkg/executor/brie.go
- pkg/statistics/handle/handletest/handle_test.go
- pkg/parser/parser_test.go
- pkg/ttl/ttlworker/job_manager_integration_test.go
- pkg/executor/test/analyzetest/analyze_test.go
- pkg/executor/builder.go
🚧 Files skipped from review as they are similar to previous changes (9)
- pkg/executor/analyze_utils_test.go
- pkg/executor/coprocessor.go
- pkg/executor/point_get.go
- pkg/executor/select.go
- pkg/planner/core/logical_plans_test.go
- pkg/parser/ast/stats.go
- pkg/statistics/handle/globalstats/global_stats_test.go
- pkg/testkit/testkit.go
- pkg/parser/parser.y
0xPoe
force-pushed
the
backport-flush-stats-delta-release-8.5
branch
from
02eaee9 to
756ba3e
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/executor/builder.go (1)
3098-3142:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't downgrade canceled PD lookups into a fallback sample rate.
getAdjustedSampleRatenow takes the statementctx, butgetApproximateTableCountFromStorageonly gives you(count, hasPD). Inpkg/executor/internal/pdhelper/pd.go:87-105, anyGetPDRegionStats(ctx, ...)failure — includingcontext.Canceledor deadline exceeded — is flattened intohasPD == false. From here, this function falls back to the default rate orsampleRate=1, so a canceled statement can keep building analyze tasks with a different sampling policy instead of aborting.🛠️ Suggested direction
-func (b *executorBuilder) getAdjustedSampleRate(ctx context.Context, task plannercore.AnalyzeColumnsTask) (sampleRate float64, reason string) { +func (b *executorBuilder) getAdjustedSampleRate(ctx context.Context, task plannercore.AnalyzeColumnsTask) (sampleRate float64, reason string, err error) { statsHandle := domain.GetDomain(b.sctx).StatsHandle() defaultRate := 0.001 if statsHandle == nil { - return defaultRate, fmt.Sprintf("statsHandler is nil, use the default-rate=%v", defaultRate) + return defaultRate, fmt.Sprintf("statsHandler is nil, use the default-rate=%v", defaultRate), nil } ... - approxiCount, hasPD := b.getApproximateTableCountFromStorage(ctx, tid, task) + approxiCount, hasPD, err := b.getApproximateTableCountFromStorage(ctx, tid, task) + if err != nil { + return 0, "", err + } ... - return sampleRate, fmt.Sprintf("use min(1, %v/%v) as the sample-rate=%v", config.DefRowsForSampleRate, statsTbl.RealtimeCount, sampleRate) + return sampleRate, fmt.Sprintf("use min(1, %v/%v) as the sample-rate=%v", config.DefRowsForSampleRate, statsTbl.RealtimeCount, sampleRate), nil }You'd also need
pkg/executor/internal/pdhelper/pd.goto surface context errors instead of collapsing them intohasPD == false.As per coding guidelines, "Keep error handling actionable and contextual; avoid silently swallowing errors."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ec10f0bc-55d0-41e9-bde3-88bbdfad6b2f
📒 Files selected for processing (23)
pkg/executor/adapter.gopkg/executor/analyze.gopkg/executor/analyze_col.gopkg/executor/analyze_col_v2.gopkg/executor/analyze_test.gopkg/executor/analyze_utils_test.gopkg/executor/benchmark_test.gopkg/executor/brie.gopkg/executor/builder.gopkg/executor/coprocessor.gopkg/executor/executor_required_rows_test.gopkg/executor/foreign_key.gopkg/executor/index_merge_reader.gopkg/executor/point_get.gopkg/executor/select.gopkg/executor/test/analyzetest/analyze_test.gopkg/planner/cardinality/selectivity_test.gopkg/statistics/handle/globalstats/global_stats_test.gopkg/statistics/handle/handletest/handle_test.gopkg/statistics/handle/storage/dump_test.gopkg/statistics/handle/storage/gc_test.gopkg/testkit/testkit.gopkg/ttl/ttlworker/job_manager_integration_test.go
✅ Files skipped from review due to trivial changes (6)
- pkg/executor/select.go
- pkg/testkit/testkit.go
- pkg/executor/point_get.go
- pkg/statistics/handle/handletest/handle_test.go
- pkg/statistics/handle/globalstats/global_stats_test.go
- pkg/planner/cardinality/selectivity_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- pkg/executor/index_merge_reader.go
- pkg/executor/analyze.go
- pkg/executor/analyze_utils_test.go
- pkg/executor/brie.go
- pkg/statistics/handle/storage/dump_test.go
- pkg/statistics/handle/storage/gc_test.go
- pkg/executor/test/analyzetest/analyze_test.go
- pkg/ttl/ttlworker/job_manager_integration_test.go
- pkg/executor/foreign_key.go
- pkg/executor/benchmark_test.go
- pkg/executor/adapter.go
0xPoe
force-pushed
the
backport-flush-stats-delta-release-8.5
branch
5 times, most recently
from
d307f29 to
26aed30
Compare
Member
Author
|
/retest |
2 similar comments
Member
Author
|
/retest |
|
/retest |
0xPoe
force-pushed
the
backport-flush-stats-delta-release-8.5
branch
from
26aed30 to
5e0b865
Compare
0xPoe
force-pushed
the
backport-flush-stats-delta-release-8.5
branch
from
5e0b865 to
5ad7219
Compare
ref pingcap#65668 (cherry picked from commit d6709d2)
…cap#67609) ref pingcap#66888 (cherry picked from commit 921c4c1)
…objects (pingcap#67728) ref pingcap#66888 (cherry picked from commit 324c404)
…p#67766) close pingcap#66888 (cherry picked from commit ce92298)
…ap#67939) close pingcap#22934 (cherry picked from commit 42118f3)
…68146) Backport pingcap#68146 to release-8.5.
ref pingcap#22934 (cherry picked from commit 4ff0610)
0xPoe
force-pushed
the
backport-flush-stats-delta-release-8.5
branch
from
5ad7219 to
627e14d
Compare
ti-chi-bot
Bot
added
cherry-pick-approved
qw4990
approved these changes
May 11, 2026
terry1purcell
approved these changes
May 11, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: qw4990, terry1purcell The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: ref #65668
Problem Summary:
Backport the
FLUSH STATS_DELTASQL feature torelease-8.5so users can force pending optimizer statistics deltas to be persisted without relying on test-only handle calls.What changed and how does it work?
FLUSH STATS_DELTAwith scoped objects:*.*,db.*,tbl, anddb.tbl.CLUSTERsyntax is accepted by the parser, but execution returns an unsupported error because this release branch does not have the newertipbBroadcastQuery support needed by upstream cluster execution.Check List
Tests
Executed:
make parser_yacccd pkg/parser && go test . -run 'TestDBAStmt|TestKeywordsLength'go test -tags=intest ./pkg/executor/test/simpletest -run 'TestFlushStatsDelta'go test -tags=intest ./pkg/statistics/handle/updatetest -run 'TestFillMissingStatsMeta'go test -tags=intest ./pkg/planner/core -run 'TestVisitInfo/test_non-column_privilege_visit_info'Side effects
Documentation
Release note
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Tests