statistics: replace separate TopN merge with combined TopN+histogram merge for global stats

[mjonss]

What problem does this PR solve?

Issue Number: ref #66220

Problem Summary:
The default global stats merge uses MergePartTopN2GlobalTopN which
performs O(P²) cross-partition histogram lookups per TopN value via
EqualRowCount + BinarySearchRemoveVal. This is slow for tables
with many partitions and inflates TopN counts for values that fall
inside histogram buckets but are not at upper bounds, producing less
accurate global statistics.

What changed and how does it work?

• Add MergePartTopNAndHistToGlobal, a new combined TopN+histogram
  merge algorithm that sums TopN counts across partitions and extracts
  exact Repeat counts from histogram upper bounds in a single linear
  pass, avoiding the O(P²) cross-partition lookups
• Use it as the sole merge strategy for both the blocking and async
  global stats paths
• Deprecate tidb_merge_partition_stats_concurrency: the variable is
  kept for backward compatibility but always reads as 1; setting it to
  any other value issues a deprecation warning and is silently ignored
• Remove the old merge code: MergePartTopN2GlobalTopN,
  MergeGlobalStatsTopNByConcurrency, MergePartitionHist2GlobalHist,
  BinarySearchRemoveVal, DatumMapCache, the topN merge worker
  infrastructure, and MaxPartitionMergeBatchSize
• Simplify mergeHistBucketsToGlobalHist by removing the unused
  recalcRepeatHists parameter
• Add BenchmarkGlobalStatsMerge for cross-branch performance
  comparison

Check List

Tests

• Unit test
• Integration test
• Manual test (add detailed scripts or steps below)
• No need to test

  • I checked and no code files have been changed.

Side effects

• Performance regression: Consumes more CPU
• Performance regression: Consumes more Memory
• Breaking backward compatibility

Documentation

• Affects user behaviors
• Contains syntax changes
• Contains variable changes
• Contains experimental features
• Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

Global stats now use a new combined TopN+histogram merge algorithm
that produces more accurate statistics and better performance for
partitioned tables. The `tidb_merge_partition_stats_concurrency`
system variable is deprecated and will be removed in a future release.

Summary by CodeRabbit

• Deprecation

  • The tidb_merge_partition_stats_concurrency system variable is deprecated and fixed to value "1"; changing it no longer affects partition stats merging and will emit a deprecation warning.

• Refactor

  • Partition-to-global statistics merging now uses a single combined merge algorithm for more consistent, simplified global stats results.

[pantheon-ai]

Review Complete

Findings: 0 issues
Posted: 0
Duplicates/Skipped: 0

_{ℹ️ Learn more details on Pantheon AI.}

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

pkg/statistics/handle/globalstats/global_stats_async.go (1)

512-520: ⚠️ Potential issue | 🟠 Major

Use the outer ANALYZE session's killer here.

Line 513 reads SQLKiller from the pooled stats session created by util.CallWithSCtx(...), so canceling the user's ANALYZE will not interrupt MergePartTopNAndHistToGlobal. Capture the caller session's killer next to stmtCtx/analyzeVersion and thread that down instead.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/statistics/handle/globalstats/global_stats_async.go` around lines 512 -
520, The pooled stats session's SQLKiller (sctx.GetSessionVars().SQLKiller) is
being used inside MergePartTopNAndHistToGlobal, so cancellations of the outer
ANALYZE won't propagate; capture the caller session's killer (the outer
session's SQLKiller) alongside stmtCtx and analyzeVersion before calling
util.CallWithSCtx (or pass it into the closure), then replace the pooled
sctx.GetSessionVars().SQLKiller reference by that captured killer when invoking
statistics.MergePartTopNAndHistToGlobal (identify the call site using
MergePartTopNAndHistToGlobal, wrapper.AllTopN, wrapper.AllHg, stmtCtx,
analyzeVersion to locate the code).

🤖 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/statistics/handle/globalstats/global_stats_test.go`:
- Around line 970-989: Pin the session to deterministic global-stats and pruning
behavior by adding tk.MustExec calls before creating the table and running
ANALYZE to set the session variables that force V2 global stats and dynamic
partition pruning and a fixed bucket/topn layout (for example, set
tidb_enable_global_stats=1, tidb_analyze_version=2,
tidb_partition_prune_mode='dynamic' and explicit stats bucket/topn values) so
the assertions about partition_name='global' and bucket layout in this test
(which uses tk.MustExec, the CREATE TABLE and ANALYZE statements) are stable and
do not depend on cluster-wide defaults.

---

Duplicate comments:
In `@pkg/statistics/handle/globalstats/global_stats_async.go`:
- Around line 512-520: The pooled stats session's SQLKiller
(sctx.GetSessionVars().SQLKiller) is being used inside
MergePartTopNAndHistToGlobal, so cancellations of the outer ANALYZE won't
propagate; capture the caller session's killer (the outer session's SQLKiller)
alongside stmtCtx and analyzeVersion before calling util.CallWithSCtx (or pass
it into the closure), then replace the pooled sctx.GetSessionVars().SQLKiller
reference by that captured killer when invoking
statistics.MergePartTopNAndHistToGlobal (identify the call site using
MergePartTopNAndHistToGlobal, wrapper.AllTopN, wrapper.AllHg, stmtCtx,
analyzeVersion to locate the code).

🪄 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: adf504ec-8697-48fc-8704-8cfc2ca60bd4

📥 Commits

Reviewing files that changed from the base of the PR and between 5fb89f9 and ca440d8.

📒 Files selected for processing (10)

• pkg/executor/analyze_test.go
• pkg/sessionctx/vardef/tidb_vars.go
• pkg/sessionctx/variable/session.go
• pkg/sessionctx/variable/sysvar.go
• pkg/statistics/handle/globalstats/global_stats.go
• pkg/statistics/handle/globalstats/global_stats_async.go
• pkg/statistics/handle/globalstats/global_stats_test.go
• pkg/statistics/handle/globalstats/topn.go
• pkg/statistics/handle/util/pool.go
• pkg/statistics/handle/util/util.go

💤 Files with no reviewable changes (5)

• pkg/statistics/handle/util/util.go
• pkg/sessionctx/variable/session.go
• pkg/sessionctx/vardef/tidb_vars.go
• pkg/statistics/handle/globalstats/topn.go
• pkg/sessionctx/variable/sysvar.go

✅ Files skipped from review due to trivial changes (1)

• pkg/statistics/handle/util/pool.go

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Pin the session settings this regression test depends on.

These assertions only hold for V2 global stats under dynamic pruning, but the test currently relies on whichever defaults happen to be active. If either default changes, partition_name='global' can disappear or the bucket layout can shift for reasons unrelated to this PR.

Suggested fix

 func TestGlobalStatsMergeCombined(t *testing.T) {
 	store, dom := testkit.CreateMockStoreAndDomain(t)
 	tk := testkit.NewTestKit(t, store)
 	tk.MustExec("use test")
+	tk.MustExec("set @@tidb_partition_prune_mode='dynamic'")
+	tk.MustExec("set @@tidb_analyze_version=2")
 	tk.MustExec("drop table if exists t")

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/statistics/handle/globalstats/global_stats_test.go` around lines 970 -
989, Pin the session to deterministic global-stats and pruning behavior by
adding tk.MustExec calls before creating the table and running ANALYZE to set
the session variables that force V2 global stats and dynamic partition pruning
and a fixed bucket/topn layout (for example, set tidb_enable_global_stats=1,
tidb_analyze_version=2, tidb_partition_prune_mode='dynamic' and explicit stats
bucket/topn values) so the assertions about partition_name='global' and bucket
layout in this test (which uses tk.MustExec, the CREATE TABLE and ANALYZE
statements) are stable and do not depend on cluster-wide defaults.

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (2)

pkg/statistics/histogram.go (2)

1587-1597: ⚠️ Potential issue | 🔴 Critical

Skip nil partition TopN entries before calling TotalCount().

topNs can legitimately contain nil for histogram-only partitions (for example ANALYZE ... WITH 0 TOPN). topN.TotalCount() will panic before the merge reaches the histogram path.

Suggested fix

 	for _, topN := range topNs {
 		if err := killer.HandleSignal(); err != nil {
 			return nil, nil, err
 		}
-		if topN.TotalCount() == 0 {
+		if topN == nil || topN.TotalCount() == 0 {
 			continue
 		}
 		for _, val := range topN.TopN {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/statistics/histogram.go` around lines 1587 - 1597, The loop over topNs
must skip nil entries before calling topN.TotalCount() to avoid panics for
histogram-only partitions; update the loop in the Merge/aggregation routine that
iterates topNs (where killer.HandleSignal() is called and topN.TotalCount() is
used) to first check if topN == nil and continue, and likewise guard the inner
loop that accesses topN.TopN and uses hack.String(val.Encoded) so it never
dereferences a nil topN.

---

1600-1684: ⚠️ Potential issue | 🟠 Major

Keep honoring SQLKiller after phase 1.

After the initial TopN loop there are no more cancellation points. On large partitioned tables, a killed ANALYZE can stay stuck through bucket extraction, sorting, and mergeHistBucketsToGlobalHist.

Suggested direction

 func MergePartTopNAndHistToGlobal(
 	...
 	killer *sqlkiller.SQLKiller,
 	...
 ) (*TopN, *Histogram, error) {
 	...
 	for _, hist := range hists {
+		if err := killer.HandleSignal(); err != nil {
+			return nil, nil, err
+		}
 		for _, b := range hist.buildBucket4Merging() {
+			if err := killer.HandleSignal(); err != nil {
+				return nil, nil, err
+			}
 			...
 		}
 	}
 	...
 	globalHist, err := mergeHistBucketsToGlobalHist(
-		sc, buckets, totCount, totNull, totColSize, expBucketNumber,
+		sc, killer, buckets, totCount, totNull, totColSize, expBucketNumber,
 		...
 	)
 }

 func mergeHistBucketsToGlobalHist(
 	sc *stmtctx.StatementContext,
+	killer *sqlkiller.SQLKiller,
 	buckets []*bucket4Merging,
 	...
 ) (*Histogram, error) {
 	...
 	for i := len(buckets) - 1; i >= 0; i-- {
+		if err := killer.HandleSignal(); err != nil {
+			return nil, err
+		}
 		...
 	}

Also applies to: 1692-1701

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/statistics/histogram.go` around lines 1600 - 1684, The loop-heavy phases
after Phase 1 (especially the inner loop over hist.buildBucket4Merging(), the
Post-TopN leftover handling before sortBucketsByUpperBound, and just before
mergeHistBucketsToGlobalHist) do not check SQL cancellation; add periodic
SQLKiller checks (using the session context's cancellation API / SQLKiller) at
the end of the bucket-extraction loop (inside the for _, b := range
hist.buildBucket4Merging() loop), after adding leftover TopN entries (the loop
over leftTopN/topNMetaToDatum), and immediately before calling
sortBucketsByUpperBound and before mergeHistBucketsToGlobalHist; if the context
is cancelled return early with the canonical cancellation error to avoid
long-blocking operations in functions like sortBucketsByUpperBound and
mergeHistBucketsToGlobalHist.

🤖 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/statistics/handle/globalstats/topn_test.go`:
- Around line 278-279: The test currently uses require.InDelta(t, inputTotal,
outputTotal, inputTotal*0.01, ...) which allows a 1% tolerance; change this to
an exact equality assertion so integer-count preservation can't be masked
(replace the InDelta call with require.Equal or require.EqualValues comparing
inputTotal and outputTotal and keep the same failure message). Update the
assertion in topn_test.go (the require.InDelta call referencing inputTotal and
outputTotal) to use an exact equality assertion so the totals must match
exactly.

---

Duplicate comments:
In `@pkg/statistics/histogram.go`:
- Around line 1587-1597: The loop over topNs must skip nil entries before
calling topN.TotalCount() to avoid panics for histogram-only partitions; update
the loop in the Merge/aggregation routine that iterates topNs (where
killer.HandleSignal() is called and topN.TotalCount() is used) to first check if
topN == nil and continue, and likewise guard the inner loop that accesses
topN.TopN and uses hack.String(val.Encoded) so it never dereferences a nil topN.
- Around line 1600-1684: The loop-heavy phases after Phase 1 (especially the
inner loop over hist.buildBucket4Merging(), the Post-TopN leftover handling
before sortBucketsByUpperBound, and just before mergeHistBucketsToGlobalHist) do
not check SQL cancellation; add periodic SQLKiller checks (using the session
context's cancellation API / SQLKiller) at the end of the bucket-extraction loop
(inside the for _, b := range hist.buildBucket4Merging() loop), after adding
leftover TopN entries (the loop over leftTopN/topNMetaToDatum), and immediately
before calling sortBucketsByUpperBound and before mergeHistBucketsToGlobalHist;
if the context is cancelled return early with the canonical cancellation error
to avoid long-blocking operations in functions like sortBucketsByUpperBound and
mergeHistBucketsToGlobalHist.

🪄 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: 4c22ec68-7052-4831-b711-57ad5514b767

📥 Commits

Reviewing files that changed from the base of the PR and between ca440d8 and 454a0cd.

📒 Files selected for processing (9)

• pkg/statistics/cmsketch_util.go
• pkg/statistics/handle/globalstats/global_stats.go
• pkg/statistics/handle/globalstats/merge_worker.go
• pkg/statistics/handle/globalstats/topn.go
• pkg/statistics/handle/globalstats/topn_bench_test.go
• pkg/statistics/handle/globalstats/topn_test.go
• pkg/statistics/histogram.go
• pkg/statistics/histogram_bench_test.go
• pkg/statistics/histogram_test.go

💤 Files with no reviewable changes (5)

• pkg/statistics/histogram_test.go
• pkg/statistics/cmsketch_util.go
• pkg/statistics/handle/globalstats/merge_worker.go
• pkg/statistics/histogram_bench_test.go
• pkg/statistics/handle/globalstats/topn.go

🚧 Files skipped from review as they are similar to previous changes (1)

• pkg/statistics/handle/globalstats/topn_bench_test.go

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Assert count preservation exactly here.

This fixture only uses exact integer counts, so a 1% delta lets real loss/duplication bugs slip through. The merge invariant should be checked with exact equality.

Suggested fix

-	require.InDelta(t, inputTotal, outputTotal, inputTotal*0.01,
-		"total count should be preserved: input=%.0f, output=%.0f", inputTotal, outputTotal)
+	require.Equal(t, inputTotal, outputTotal,
+		"total count should be preserved exactly")

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	require.InDelta(t, inputTotal, outputTotal, inputTotal*0.01,
	"total count should be preserved: input=%.0f, output=%.0f", inputTotal, outputTotal)
	require.Equal(t, inputTotal, outputTotal,
	"total count should be preserved exactly")

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/statistics/handle/globalstats/topn_test.go` around lines 278 - 279, The
test currently uses require.InDelta(t, inputTotal, outputTotal, inputTotal*0.01,
...) which allows a 1% tolerance; change this to an exact equality assertion so
integer-count preservation can't be masked (replace the InDelta call with
require.Equal or require.EqualValues comparing inputTotal and outputTotal and
keep the same failure message). Update the assertion in topn_test.go (the
require.InDelta call referencing inputTotal and outputTotal) to use an exact
equality assertion so the totals must match exactly.

[coderabbitai]

Actionable comments posted: 1

🤖 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/sessionctx/variable/sysvar.go`:
- Around line 3084-3094: The TiDBMergePartitionStatsConcurrency sysvar currently
only has a Validation hook, which doesn't override persisted reads; add
GetGlobal and GetSession hooks on the vardef for
TiDBMergePartitionStatsConcurrency that unconditionally return the normalized
value "1" (and nil error) so reads via session.GetGlobalSysVar()/GetSession
always return "1" regardless of stored mysql.global_variables; keep the existing
Validation and SetSession as-is but ensure both GetGlobal and GetSession are
added to the same vardef entry to enforce the "always reads 1" contract on
upgraded clusters.

🪄 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: f8a6d4c8-38f2-4fd6-9cf7-82fc3173dffa

📥 Commits

Reviewing files that changed from the base of the PR and between 454a0cd and 1085ba4.

📒 Files selected for processing (4)

• pkg/sessionctx/vardef/tidb_vars.go
• pkg/sessionctx/variable/sysvar.go
• pkg/statistics/BUILD.bazel
• pkg/statistics/handle/globalstats/BUILD.bazel

🚧 Files skipped from review as they are similar to previous changes (3)

• pkg/statistics/BUILD.bazel
• pkg/sessionctx/vardef/tidb_vars.go
• pkg/statistics/handle/globalstats/BUILD.bazel

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Sysvar definition ==="
rg -n -C2 'TiDBMergePartitionStatsConcurrency|tidb_merge_partition_stats_concurrency' pkg/sessionctx/variable pkg/sessionctx/vardef

echo
echo "=== Validation / session hook flow ==="
rg -n -C3 'func \(sv \*SysVar\) Validate|func \(sv \*SysVar\) SetSessionFromHook|SetSystemVar\(' pkg/sessionctx/variable

echo
echo "=== Global-var load / session-init paths ==="
rg -n -C3 'builtinGlobalVariable|GetGlobalSysVar|SetSessionFromHook|load.*global.*var|mysql\.global_variables' .

echo
echo "=== Any bootstrap / upgrade rewrite for this sysvar ==="
rg -n -C2 'TiDBMergePartitionStatsConcurrency|tidb_merge_partition_stats_concurrency|upgradeToVer|bootstrapVersion' .

Repository: pingcap/tidb

Length of output: 50371

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Check if there's an upgrade function for tidb_merge_partition_stats_concurrency ==="
rg -n 'tidb_merge_partition_stats_concurrency' pkg/session

echo
echo "=== How does GetGlobalSysVar handle validation and normalization? ==="
rg -n -A10 'func.*GetGlobalSysVar' pkg/session/session.go | head -60

echo
echo "=== Check ValidateWithRelaxedValidation and its usage in read paths ==="
rg -n -B2 -A5 'ValidateWithRelaxedValidation' pkg/sessionctx/variable

echo
echo "=== Does any code apply Validation during GetGlobalSysVar? ==="
rg -n 'GetGlobalSysVar.*Validate|Validate.*GetGlobalSysVar' pkg/session pkg/sessionctx

Repository: pingcap/tidb

Length of output: 143

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== GetGlobalSysVar implementation in session.go ==="
rg -n -A20 'func.*\(s \*session\) GetGlobalSysVar' pkg/session/session.go

echo
echo "=== ValidateWithRelaxedValidation usage ==="
rg -n -B3 -A10 'ValidateWithRelaxedValidation' pkg/sessionctx/variable/variable.go

echo
echo "=== Does SysVar have GetGlobal hook? Show the struct definition ==="
rg -n -A30 'type SysVar struct' pkg/sessionctx/variable/variable.go | head -60

Repository: pingcap/tidb

Length of output: 4587

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Complete SysVar struct definition ==="
rg -n -A70 'type SysVar struct' pkg/sessionctx/variable/variable.go | head -100

echo
echo "=== GetGlobalFromHook implementation ==="
rg -n -B3 -A15 'func.*GetGlobalFromHook' pkg/sessionctx/variable/variable.go

echo
echo "=== Verify the validation path: does GetGlobalFromHook always apply ValidateWithRelaxedValidation? ==="
rg -n -B5 -A20 'func.*GetGlobalFromHook' pkg/sessionctx/variable/variable.go

Repository: pingcap/tidb

Length of output: 6315

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Where is GlobalVarsAccessor defined and what does GetGlobalSysVar do? ==="
rg -n -A15 'func.*GlobalVarsAccessor.*GetGlobalSysVar|interface.*GlobalVarsAccessor' pkg/sessionctx/variable

echo
echo "=== Look at the session implementation of GetGlobalSysVar to see if it applies validation ==="
rg -n -A30 'func.*session.*GetGlobalSysVar.*{' pkg/session/session.go | head -80

Repository: pingcap/tidb

Length of output: 133

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Find GlobalVarsAccessor interface ==="
rg -n 'type GlobalVarsAccessor' pkg/sessionctx

echo
echo "=== Look at how GetGlobalSysVar loads from mysql.global_variables and whether it validates ==="
rg -n -A40 'func.*session.*GetGlobalSysVar' pkg/session/session.go | head -100

echo
echo "=== Check the domain.GetGlobalVar method ==="
rg -n -A20 'func.*GetGlobalVar.*{' pkg/domain | head -60

Repository: pingcap/tidb

Length of output: 97

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Search for GlobalVarsAccessor ==="
fd GlobalVarsAccessor pkg/

echo
echo "=== Look at the entire GetGlobalSysVar from session.go ==="
sed -n '1404,1450p' pkg/session/session.go

echo
echo "=== Check if there's validation in the domain.GetGlobalVar path ==="
find pkg/domain -name '*.go' -exec grep -l 'GetGlobalVar\|global.*variable' {} \;

echo
echo "=== Look for ValidateWithRelaxedValidation calls to understand the flow ==="
rg -n 'ValidateWithRelaxedValidation' pkg/session pkg/sessionctx

Repository: pingcap/tidb

Length of output: 3778

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== What does ValidateFromType do vs Validation callback? ==="
rg -n -A30 'func \(sv \*SysVar\) ValidateFromType' pkg/sessionctx/variable/variable.go

echo
echo "=== Confirm: does ValidateFromType call Validation callback? ==="
sed -n '238,285p' pkg/sessionctx/variable/variable.go

Repository: pingcap/tidb

Length of output: 3180

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Check if GetGlobalFromHook is used for reading global sysvars ==="
rg -n 'GetGlobalFromHook' pkg/sessionctx pkg/session

echo
echo "=== Understand the call chain: when you do SELECT @@global.var, does it use GetGlobalFromHook or GetGlobalSysVar? ==="
rg -n -B5 -A10 'SELECT.*@@global|SHOW.*GLOBAL.*VARIABLES' pkg/executor | head -80

Repository: pingcap/tidb

Length of output: 7994

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Verify checkInt64SystemVar behavior for out-of-range values ==="
rg -n -A30 'func \(sv \*SysVar\) checkInt64SystemVar' pkg/sessionctx/variable/variable.go

echo
echo "=== Understand: when ValidateFromType checks an int value like '5', does it reject it or accept it? ==="
sed -n '171,210p' pkg/sessionctx/variable/variable.go

Repository: pingcap/tidb

Length of output: 2714

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Check the GetSessionOrGlobalSystemVar flow - is it GetGlobalFromHook or GetGlobalSysVar? ==="
sed -n '2940,2990p' pkg/sessionctx/variable/session.go

echo
echo "=== Verify which path is taken: GetGlobalFromHook or GetGlobalSysVar? ==="
rg -n -B10 'GetGlobalFromHook|GetGlobalSysVar' pkg/sessionctx/variable/session.go | head -100

Repository: pingcap/tidb

Length of output: 3025

---

🏁 Script executed:

#!/bin/bash
set -euo ipefail

echo "=== Confirm: does Validate() call both ValidateFromType AND Validation callback? ==="
sed -n '219,235p' pkg/sessionctx/variable/variable.go

echo
echo "=== And confirm ValidateFromType does NOT call Validation callback? ==="
sed -n '238,263p' pkg/sessionctx/variable/variable.go

echo
echo "=== Does session.GetGlobalSysVar (line 1441) apply ValidateWithRelaxedValidation or just ValidateFromType? ==="
sed -n '1438,1448p' pkg/session/session.go

Repository: pingcap/tidb

Length of output: 108

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== Confirm: does Validate() call both ValidateFromType AND Validation callback? ==="
sed -n '219,235p' pkg/sessionctx/variable/variable.go

echo
echo "=== And confirm ValidateFromType does NOT call Validation callback? ==="
sed -n '238,263p' pkg/sessionctx/variable/variable.go

echo
echo "=== Does session.GetGlobalSysVar (line 1441) apply ValidateWithRelaxedValidation or just ValidateFromType? ==="
sed -n '1438,1448p' pkg/session/session.go

Repository: pingcap/tidb

Length of output: 2366

---

Add GetGlobal and GetSession hooks to enforce the "always reads 1" contract on upgraded clusters.

The Validation callback only normalizes future SET statements. When reading persisted global values via session.GetGlobalSysVar() (the fallback path when no GetGlobal hook is defined), only ValidateFromType() is applied, which skips the custom Validation() callback entirely. A cluster upgraded from an older version that had a non-1 value persisted in mysql.global_variables will return that old value instead of 1, breaking the new invariant that this deprecated variable always reads as 1.

Add GetGlobal and GetSession hooks that both unconditionally return "1" to ensure the sysvar always reads as 1 on all clusters, regardless of persisted state.

Suggested fix

 	{
 		Scope: vardef.ScopeGlobal | vardef.ScopeSession, Name: vardef.TiDBMergePartitionStatsConcurrency, Value: "1", Type: vardef.TypeInt, MinValue: 1, MaxValue: vardef.MaxConfigurableConcurrency,
 		SetSession: func(_ *SessionVars, _ string) error {
 			// Deprecated: do nothing.
 			return nil
 		},
+		GetSession: func(*SessionVars) (string, error) {
+			return "1", nil
+		},
+		GetGlobal: func(context.Context, *SessionVars) (string, error) {
+			return "1", nil
+		},
 		Validation: func(vars *SessionVars, normalizedValue string, _ string, _ vardef.ScopeFlag) (string, error) {
 			if normalizedValue != "1" {
 				vars.StmtCtx.AppendWarning(ErrWarnDeprecatedSyntaxSimpleMsg.FastGen("tidb_merge_partition_stats_concurrency is deprecated and will be removed in a future release. It no longer affects behavior."))
 			}
 			return "1", nil
 		},
 	},

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	Scope: vardef.ScopeGlobal | vardef.ScopeSession, Name: vardef.TiDBMergePartitionStatsConcurrency, Value: "1", Type: vardef.TypeInt, MinValue: 1, MaxValue: vardef.MaxConfigurableConcurrency,
	SetSession: func(_ *SessionVars, _ string) error {
	// Deprecated: do nothing.
	return nil
	},
	Validation: func(vars *SessionVars, normalizedValue string, _ string, _ vardef.ScopeFlag) (string, error) {
	if normalizedValue != "1" {
	vars.StmtCtx.AppendWarning(ErrWarnDeprecatedSyntaxSimpleMsg.FastGen("tidb_merge_partition_stats_concurrency is deprecated and will be removed in a future release. It no longer affects behavior."))
	}
	return "1", nil
	},
	Scope: vardef.ScopeGlobal | vardef.ScopeSession, Name: vardef.TiDBMergePartitionStatsConcurrency, Value: "1", Type: vardef.TypeInt, MinValue: 1, MaxValue: vardef.MaxConfigurableConcurrency,
	SetSession: func(_ *SessionVars, _ string) error {
	// Deprecated: do nothing.
	return nil
	},
	GetSession: func(*SessionVars) (string, error) {
	return "1", nil
	},
	GetGlobal: func(context.Context, *SessionVars) (string, error) {
	return "1", nil
	},
	Validation: func(vars *SessionVars, normalizedValue string, _ string, _ vardef.ScopeFlag) (string, error) {
	if normalizedValue != "1" {
	vars.StmtCtx.AppendWarning(ErrWarnDeprecatedSyntaxSimpleMsg.FastGen("tidb_merge_partition_stats_concurrency is deprecated and will be removed in a future release. It no longer affects behavior."))
	}
	return "1", nil
	},

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/sessionctx/variable/sysvar.go` around lines 3084 - 3094, The
TiDBMergePartitionStatsConcurrency sysvar currently only has a Validation hook,
which doesn't override persisted reads; add GetGlobal and GetSession hooks on
the vardef for TiDBMergePartitionStatsConcurrency that unconditionally return
the normalized value "1" (and nil error) so reads via
session.GetGlobalSysVar()/GetSession always return "1" regardless of stored
mysql.global_variables; keep the existing Validation and SetSession as-is but
ensure both GetGlobal and GetSession are added to the same vardef entry to
enforce the "always reads 1" contract on upgraded clusters.

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign terry1purcell for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS
• pkg/executor/OWNERS
• pkg/sessionctx/vardef/OWNERS
• pkg/sessionctx/variable/OWNERS
• pkg/statistics/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ti-chi-bot]

@mjonss: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-unit-test-next-gen	885c34d	link	true	/test pull-unit-test-next-gen
idc-jenkins-ci-tidb/unit-test	885c34d	link	true	/test unit-test

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.