*: support descending-order indexes (#2519)#68049
Draft
takaidohigasi wants to merge 18 commits intopingcap:masterfrom
Draft
*: support descending-order indexes (#2519)#68049takaidohigasi wants to merge 18 commits intopingcap:masterfrom
takaidohigasi wants to merge 18 commits intopingcap:masterfrom
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
do-not-merge/needs-tests-checked
do-not-merge/work-in-progress
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds end-to-end descending-index support: persisted per-column Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Parser
participant DDLExecutor
participant PD
participant Storage
Client->>Parser: CREATE INDEX ... DESC
Parser->>DDLExecutor: DDL job
DDLExecutor->>DDLExecutor: Check vardef.EnableDescendingIndex
alt Gate enabled
DDLExecutor->>PD: Get store list
PD-->>DDLExecutor: stores with versions
DDLExecutor->>DDLExecutor: validate TiKV store semver >= MinTiKVVersionForDescIndex
alt All compatible
DDLExecutor->>Storage: Persist IndexInfo (Version=1, IndexColumn.Desc=true)
Storage-->>DDLExecutor: OK
DDLExecutor-->>Client: Success
else Incompatible
DDLExecutor-->>Client: Error: upgrade TiKV
end
else Gate disabled
DDLExecutor->>Storage: Persist IndexInfo (Desc dropped, Version=0)
Storage-->>DDLExecutor: OK
DDLExecutor-->>Client: Success
end
sequenceDiagram
participant Client
participant Planner
participant PropMatcher
participant Encoder
participant TiKV
Client->>Planner: SELECT ... ORDER BY (mixed ASC/DESC)
Planner->>Planner: check indices servable (IndexInfo.IsServable)
alt Unservable index
Planner-->>Client: Error: index version too new
else Servable
Planner->>PropMatcher: match property to index
PropMatcher-->>Planner: AccessPath + MatchedScanDesc (per-column)
Planner->>Encoder: Build KV ranges (pass desc[] flags)
Encoder->>Encoder: EncodeIndexKeyWithDesc (invert bytes for DESC cols)
Encoder-->>TiKV: KV ranges
TiKV-->>Planner: Ordered rows
Planner-->>Client: Results
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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)
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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
ti-chi-bot
Bot
added
contribution
|
Hi @takaidohigasi. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
Hi @takaidohigasi. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions 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. |
ti-chi-bot
Bot
added
release-note
takaidohigasi
force-pushed
the
feature/desc-index
branch
from
e35d2ed to
b1504e5
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This was referenced Apr 25, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/executor/admin.go (1)
821-829:⚠️ Potential issue | 🟠 MajorResume-key bootstrap can skip DESC index rows on first batch
After Line 821 switched to DESC-aware encoding, the initial sentinel key from
init()is no longer guaranteed to sort before all real index keys. UsingPrefixNext()at Line 829 on that sentinel can start too far ahead, soADMIN CLEANUP INDEXmay miss dangling entries for descending indexes.🔧 Suggested fix
diff --git a/pkg/executor/admin.go b/pkg/executor/admin.go @@ -import ( - "context" - "math" +import ( + "context" @@ - keyRanges.FirstPartitionRange()[0].StartKey = kv.Key(e.lastIdxKey).PrefixNext() + if len(e.lastIdxKey) > 0 { + keyRanges.FirstPartitionRange()[0].StartKey = kv.Key(e.lastIdxKey).PrefixNext() + } @@ func (e *CleanupIndexExec) init() error { e.idxChunk = chunk.New(e.getIdxColTypes(), e.InitCap(), e.MaxChunkSize()) e.idxValues = kv.NewHandleMap() e.batchKeys = make([]kv.Key, 0, e.batchSize) e.idxValsBufs = make([][]types.Datum, e.batchSize) - sc := e.Ctx().GetSessionVars().StmtCtx - idxKey, _, err := e.index.GenIndexKey(sc.ErrCtx(), sc.TimeZone(), []types.Datum{{}}, kv.IntHandle(math.MinInt64), nil) - if err != nil { - return err - } - e.lastIdxKey = idxKey + // First scan should start from full-range lower bound. + // Resume key is populated after scanning real rows. + e.lastIdxKey = nil return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/admin.go` around lines 821 - 829, The resume sentinel advancement using PrefixNext() can skip DESC-encoded index rows; update the code that sets keyRanges.FirstPartitionRange()[0].StartKey (currently using kv.Key(e.lastIdxKey).PrefixNext()) to not call PrefixNext and instead use the raw sentinel key so it sorts at-or-before real index entries (i.e., set StartKey = kv.Key(e.lastIdxKey) or otherwise compute a resume start key that is encoding-aware for DESC columns). Modify the assignment in admin.go (the block using distsql.IndexRangesToKVRangesWithDesc, keyRanges.SetToNonPartitioned, and keyRanges.FirstPartitionRange()) to remove PrefixNext and ensure the resume key logic respects DESC index encoding.
🧹 Nitpick comments (2)
pkg/executor/executor_pkg_test.go (1)
67-67: Add one DESC-path unit case forbuildKvRangesForIndexJoin.All updated call sites in this file pass
desc=nil, so this test file currently validates only the ASC-compatible path. A focused case with at least onedesc=trueflag would better guard the new descending-index range-build path.Also applies to: 97-97, 119-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/executor_pkg_test.go` at line 67, Current tests only exercise the ascending path because all calls to buildKvRangesForIndexJoin pass desc=nil; add at least one unit test invocation that supplies a non-nil desc slice with at least one true to exercise the descending-index path. Locate the test cases calling buildKvRangesForIndexJoin in executor_pkg_test.go (the calls shown around the existing lines) and add a parallel case that reuses the same ctx, joinKeyRows, indexRanges, keyOff2IdxOff but passes desc like []bool{false, true} (or matching index column count) and asserts the returned kvRanges and err behave as expected for DESC. Ensure you update all similar call sites in that file so one covers DESC semantics while keeping existing ASC cases intact.pkg/ddl/index.go (1)
444-449: Version fence wiring is correct; minor future-proofing note.Gating
idxInfo.Version = 1onHasDescColumn()correctly couples the metadata version to the feature actually being persisted (sinceDescis already filtered throughdescEnabledinbuildIndexColumns), so v0 binaries cannot resurface unknown-format indexes via theIsServablefence. Good.Optional: if a future change introduces another reason to bump
IndexInfo.Version(e.g., a new on-disk format), an unconditional= 1here will silently downgrade it. A defensiveif idxInfo.Version < 1 { idxInfo.Version = 1 }(or a small helper) would make it safe under composition. Not blocking — there is no other writer today.♻️ Optional defensive write
- if idxInfo.HasDescColumn() { - idxInfo.Version = 1 - } + if idxInfo.HasDescColumn() && idxInfo.Version < 1 { + idxInfo.Version = 1 + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ddl/index.go` around lines 444 - 449, The current code unconditionally sets idxInfo.Version = 1 when idxInfo.HasDescColumn() is true; change this to only raise the version if it's lower than 1 to avoid silently downgrading future higher versions — i.e., replace the assignment with a conditional check such as if idxInfo.Version < 1 { idxInfo.Version = 1 } within the same HasDescColumn() branch (refer to idxInfo.Version, idxInfo.HasDescColumn(), and buildIndexColumns to locate the logic).
🤖 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/ddl/executor.go`:
- Around line 1059-1069: The DESC-index TiKV compatibility check currently lives
only in CreateTable (iterating tbInfo.Indices and calling
checkTiKVSupportsDescIndex), which lets CreateTableWithInfo and
BatchCreateTableWithInfo bypass the gate; move the check into the shared
createTableWithInfoJob path so every code path that persists a table runs it: in
createTableWithInfoJob, iterate over the passed tbInfo.Indices and call
checkTiKVSupportsDescIndex(ctx) (returning the error if present) before
persisting the table; remove the duplicate loop from CreateTable to avoid
redundant checks.
- Around line 5028-5038: The code currently skips stores with empty or
unparsable s.Version (using semver.NewVersion and logging via
logutil.DDLLogger().Warn), which lets CREATE INDEX proceed unsafely; change the
logic in the DESC-index support check so that an empty s.Version or a parseErr
from semver.NewVersion is treated as a blocking error (do not continue), e.g.,
return a descriptive error (or set the overall check to fail) with context
including store ID and version and the parseErr, instead of log-and-continue;
ensure the error surfaces up from the function that performs the DESC-index
support check so index creation is prevented when any store version is unknown
or unparsable.
In `@pkg/distsql/request_builder.go`:
- Around line 705-723: The wrapper helpers IndexRangesToKVRanges,
IndexRangesToKVRangesWithDesc, and IndexRangesToKVRangesWithInterruptSignal
currently pass desc=nil which causes DESC indexes to be encoded as ascending;
update these wrappers so when desc==nil they fetch the real per-column desc
flags from the index metadata (model.IndexInfo) for the given tid/idxID and pass
that slice into IndexRangesToKVRangesWithDescAndInterruptSignal (or
alternatively change the wrappers to return an error/compile-time safeguard to
prevent use without real metadata); also update internal call sites like
appendRanges() and BuildTableRanges() to stop calling the nil-desc helpers (or
ensure they propagate IndexInfo-derived desc) so DESC indexes produce correct
byte ranges.
In `@pkg/executor/builder.go`:
- Around line 4137-4138: The assumption that common-handle PK columns are always
ascending is unsafe when EnableDescendingIndex may set IndexColumn.Desc; either
explicitly reject DESC on clustered PKs or implement DESC-aware encoding for
common-handle paths. Fix options: (A) add validation in CreatePrimaryKey (or
buildIndexColumns) to error if IndexColumn.Desc is true for a clustered primary
key when the table uses common handles (check EnableDescendingIndex and TiKV
support) and update the comment at the buildKvRangesForIndexJoin callsites; or
(B) implement DESC handling by updating buildKvRangesForIndexJoin and the
distsql.CommonHandleRangesToKVRanges code to encode/decode common-handle ranges
with per-column DESC bits, and then adjust callers (including places noted in
find_best_task.go) and the comment to reflect proper DESC support. Ensure the
chosen fix updates the comment at the referenced buildKvRangesForIndexJoin
callsites to accurately state whether DESC is unsupported or now handled.
In `@pkg/planner/core/find_best_task.go`:
- Around line 1175-1186: The code assumes appended CommonHandle PK suffix is
always ascending by initializing thisIdxDesc to false when colIdx >=
len(path.Index.Columns); instead, determine the direction from the table's
primary/index metadata: when colIdx >= len(path.Index.Columns) look up the
corresponding primary key column's Desc flag from the primary index (e.g., via
the plan/path's primary index or table primary key columns) and set thisIdxDesc
accordingly so thisScanDesc is computed from the actual PK column direction
rather than hard-coded ASC; keep the existing matchedScanDesc/matchedScanDescSet
logic and return property.PropNotMatched as before when directions conflict.
In `@pkg/planner/core/planbuilder.go`:
- Around line 1347-1353: The servability check on the snapshot index
(index.IsServable() / index.UnservableErr(tblName.O)) is being run too early for
RC/forUpdateRead planning and must be performed after latest-index
reconciliation; update the logic in planbuilder.go so you first call the
reconciliation routine (e.g., invoke GetLatestIndexInfo() / the latest-index
reconciliation path used in forUpdateRead) to refresh or resolve the index, then
run index.IsServable() and return index.UnservableErr(...) only after that
reconciliation; make the same change for the other occurrence later in the file
(the block around the 1361–1370 equivalent).
In `@pkg/planner/util/path.go`:
- Around line 135-147: AccessPath.Clone currently omits the new MatchedScanDesc
field so cloned AccessPath instances lose the reverse-scan flag; update
AccessPath.Clone to copy MatchedScanDesc from the receiver into the new clone
(same as other metadata fields) so that GetOriginalPhysicalIndexScan sees the
correct value and preserves IndexScan.Desc across cloning (e.g., in index-merge
and plan rebuild paths); after the change add a regression test that plans the
same statement twice (or via plan cache/apply) for an index that requires a
reverse scan and asserts the resulting IndexScan.Desc remains true.
In `@pkg/table/tables/mutation_checker.go`:
- Around line 275-285: The code unconditionally bitwise-complements v for DESC
columns even when v was reconstructed by DecodeIndexKV (restored data),
corrupting restored DESC values; change the logic so the ^0xFF undo only runs
when the bytes actually came from the key (i.e., not when NeedRestoredData(...)
is true). Concretely, around the existing block that checks
indexInfo.Columns[i].Desc and modifies v, guard the complement with a check
using NeedRestoredData(indexInfo, i) (or propagate a "fromKey" flag from
DecodeIndexKV) so that v is only complemented for key-resident bytes before
calling DecodeColumnValue.
In `@pkg/tablecodec/tablecodec_test.go`:
- Line 440: Remove the unused local constant prefixLen in tablecodec_test.go
(the declaration "const prefixLen = 11") or alternatively use it where intended;
specifically delete the prefixLen declaration so the Go test package compiles
(or reference prefixLen in the surrounding test logic such as in functions/Test
helpers that compute the prefix length) and ensure no other code expects this
identifier.
In `@pkg/util/codec/codec.go`:
- Around line 1623-1650: The DESC decode branch currently allocates a new tmp
slice for every variable-length column (see the cases for ^bytesFlag,
^compactBytesFlag, ^decimalFlag) and Decoder.DecodeOne clones descending columns
into inv and peek() clones the remaining key, causing per-row heap churn; change
this to reuse a scratch buffer (e.g., a []byte scratch on Decoder or a small
sync.Pool) and/or add flag-specific fast paths that avoid full copying: modify
Decoder.DecodeOne to reuse the decoder's scratch when preparing descending
complements instead of allocating inv/tmp, and add overloads or helper variants
for peekBytes/peekCompactBytes/types.DecimalPeak that accept a scratch buffer to
write the complemented bytes in-place (or operate on complemented views) so the
complementing work does not allocate per call.
---
Outside diff comments:
In `@pkg/executor/admin.go`:
- Around line 821-829: The resume sentinel advancement using PrefixNext() can
skip DESC-encoded index rows; update the code that sets
keyRanges.FirstPartitionRange()[0].StartKey (currently using
kv.Key(e.lastIdxKey).PrefixNext()) to not call PrefixNext and instead use the
raw sentinel key so it sorts at-or-before real index entries (i.e., set StartKey
= kv.Key(e.lastIdxKey) or otherwise compute a resume start key that is
encoding-aware for DESC columns). Modify the assignment in admin.go (the block
using distsql.IndexRangesToKVRangesWithDesc, keyRanges.SetToNonPartitioned, and
keyRanges.FirstPartitionRange()) to remove PrefixNext and ensure the resume key
logic respects DESC index encoding.
---
Nitpick comments:
In `@pkg/ddl/index.go`:
- Around line 444-449: The current code unconditionally sets idxInfo.Version = 1
when idxInfo.HasDescColumn() is true; change this to only raise the version if
it's lower than 1 to avoid silently downgrading future higher versions — i.e.,
replace the assignment with a conditional check such as if idxInfo.Version < 1 {
idxInfo.Version = 1 } within the same HasDescColumn() branch (refer to
idxInfo.Version, idxInfo.HasDescColumn(), and buildIndexColumns to locate the
logic).
In `@pkg/executor/executor_pkg_test.go`:
- Line 67: Current tests only exercise the ascending path because all calls to
buildKvRangesForIndexJoin pass desc=nil; add at least one unit test invocation
that supplies a non-nil desc slice with at least one true to exercise the
descending-index path. Locate the test cases calling buildKvRangesForIndexJoin
in executor_pkg_test.go (the calls shown around the existing lines) and add a
parallel case that reuses the same ctx, joinKeyRows, indexRanges, keyOff2IdxOff
but passes desc like []bool{false, true} (or matching index column count) and
asserts the returned kvRanges and err behave as expected for DESC. Ensure you
update all similar call sites in that file so one covers DESC semantics while
keeping existing ASC cases intact.
🪄 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: e0598d5b-06f5-452d-91a4-9f39acc9e790
📒 Files selected for processing (25)
pkg/ddl/db_integration_test.gopkg/ddl/desc_index_version_check_test.gopkg/ddl/executor.gopkg/ddl/index.gopkg/distsql/request_builder.gopkg/executor/admin.gopkg/executor/builder.gopkg/executor/distsql.gopkg/executor/executor_pkg_test.gopkg/executor/index_merge_reader.gopkg/executor/infoschema_reader.gopkg/executor/show.gopkg/meta/model/index.gopkg/meta/model/index_test.gopkg/planner/core/find_best_task.gopkg/planner/core/operator/physicalop/physical_index_scan.gopkg/planner/core/planbuilder.gopkg/planner/util/path.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/sysvar.gopkg/table/tables/mutation_checker.gopkg/tablecodec/tablecodec.gopkg/tablecodec/tablecodec_test.gopkg/util/codec/codec.gopkg/util/codec/codec_test.go
…cap#2519) Add IndexColumn.Desc, IndexInfo.Version, and a tidb_enable_descending_index sysvar (default OFF) so the DESC keyword in CREATE INDEX round-trips through metadata, SHOW CREATE TABLE, and INFORMATION_SCHEMA.STATISTICS. Encoding and planner behavior are unchanged in this phase; queries continue to use reverse coprocessor scans for ORDER BY DESC, so results stay correct regardless of the gate. IndexInfo.Version bumps to 1 whenever any column is DESC. An explicit IsServable() check lets future TiDB versions refuse to serve queries against indexes whose metadata format they do not understand, once physical encoding lands in a later phase. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
pingcap#2519) matchProperty now records the uniform stored direction of the matched index columns on path.MatchedIdxDesc and rejects paths whose matched columns mix directions (a single forward/reverse cop scan cannot satisfy them). The scan-direction setters for PhysicalIndexScan and the index variant of BatchPointGet XOR the required order with that direction, so a DESC-stored index satisfies ORDER BY DESC with a forward scan and satisfies ORDER BY ASC with a reverse scan. No behavior change when tidb_enable_descending_index is off (the default) because no column ever has Desc=true in that state. Correctness of the new forward-scan path depends on the physical-encoding work in a later phase; until then the feature gate should remain off in production. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
…pingcap#2519) Add per-column descending-order encoding helpers that bitwise-complement the memcomparable bytes of DESC columns so a forward byte-order scan yields the user-declared direction. The helpers reuse the existing encode()/DecodeOne() dispatch by complementing around them, keeping the new surface area small and supporting every type the ascending path supports (int, uint, float, bytes/string, null, decimal, duration, time). This commit adds only the codec primitives — no callers are wired yet, so behaviour is unchanged. Subsequent phases thread per-column Desc flags through tablecodec (index key writes) and distsql (range bounds). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
…3b of pingcap#2519) GenIndexKey now takes the IndexColumn.Desc flag into account when writing index keys: columns with Desc=true have their memcomparable bytes bitwise-complemented so a forward RocksDB iterator yields the declared direction. All-ascending indexes (the overwhelmingly common case) stay on the original EncodeKey fast path, producing byte-identical output to before — crucial for reading legacy data unchanged. Read-side range-bound encoding in distsql.EncodeIndexKey still produces ascending-encoded bounds, so enabling tidb_enable_descending_index and writing to a DESC index with this commit alone will hide the rows from subsequent queries. The read-side bound construction, including the low↔high swap needed for DESC-differentiating columns, lands in phase 3c. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
… 3c of pingcap#2519) Thread per-column descending-order flags from IndexInfo into the byte-range builders used by IndexReader, IndexLookUp, IndexMerge, admin check, and the index-join inner builder. The encoder complements the stored bytes and, if the resulting byte low/high pair is inverted (which happens when a differentiating column is DESC), swaps them and their exclude flags so the range handed to TiKV stays well-ordered. Supporting pieces: * codec.peek() learns the descending flag bytes so codec.CutOne and every walker layered on it (DecodeIndexHandle, CutIndexKeyNew, ...) advances past DESC-encoded columns without schema context. * mutation_checker's consistency check un-complements a column's bytes before handing them to DecodeColumnValue, and uses DecodeKeyHead instead of DecodeIndexKey where only the index id is needed. * indexColDescFlags() materialises an IndexInfo.Columns[i].Desc slice on demand, returning nil for all-ascending indexes so callers stay on the original allocation-free fast path. INSERT against a DESC index now passes through end-to-end; SELECT served through the unistore/TiKV coprocessor emulation still fails because that layer has yet to learn DESC decoding. The coordinated TiKV work is phase 3d — at which point the integration test can be extended to cover SELECT. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
…ngcap#2519) Teach Decoder.DecodeOne to recognise the bitwise-complemented leading flag of a descending-order column and transparently invert the column's bytes before dispatching through the ascending switch. The unistore coprocessor emulator calls into this decoder when streaming index columns back to TiDB, so this change is the last piece needed to exercise INSERT + SELECT against a DESC index end-to-end through MockStore. Auto-detection (rather than a threaded per-column desc flag) avoids touching the tipb protobuf for now: the flag byte itself disambiguates ascending from descending, and the sole theoretical collision (^floatFlag == maxFlag == 0xFA) never occurs on stored rows because maxFlag is exclusively a range-sentinel marker that this decoder never receives. The real TiKV coprocessor (Rust) still needs the same treatment for production use; that lands in a follow-up commit to pingcap/tikv. The DDL integration test now covers point lookups (both present and missing keys), a forward-ordered DESC scan that satisfies ORDER BY DESC without a reverse flag, and the reverse-scan path used to satisfy ORDER BY ASC on a DESC-stored index. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
The fence was added to IndexInfo's metadata in phase 1 but no caller
invoked it, leaving an old TiDB binary that read schema written by a
newer one free to silently drop the unknown Version field and serve
queries against an index it doesn't understand. Wire the check in:
* getPossibleAccessPaths returns an error when an enumerated index
has Version > maxKnownIndexVersion. SELECT/UPDATE/DELETE planning
runs through here, so they fail fast with a clear message.
* buildInsert calls a new checkAllIndicesServable helper. INSERT
VALUES never enumerates access paths so it needs an explicit guard
before the executor touches any index-maintenance path.
The error message names the index, the table, the version mismatch,
and the remediation (upgrade TiDB or DROP INDEX) so an operator can
act without grepping source.
Tests cover both the model-level helper and an end-to-end forge: an
in-memory IndexInfo with Version=99 must be rejected by SELECT and
INSERT alike.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
When CREATE INDEX or CREATE TABLE would persist a column with Desc=true, walk the cluster's PD-reported store list and refuse to proceed if any non-TiFlash, non-tombstone TiKV store is below MinTiKVVersionForDescIndex. This is the third defence in the rolling-upgrade plan from phase 1: the sysvar gate prevents accidental enablement, IndexInfo.Version protects an older TiDB from serving an unfamiliar index, and now this version gate prevents writing a DESC-encoded index against TiKV that cannot decode it. The DDL paths covered: * CreateIndex / createIndex: secondary index DDL. * CreatePrimaryKey: standalone PK DDL. * CreateTable: KEY (col DESC) inside CREATE TABLE statements. Mock stores (which never satisfy tikv.Storage) skip the gate so the existing MockStore-based integration tests continue to exercise DESC indexes via the unistore auto-detection path. The pure version-comparison core (checkStoresMeetDescIndexMinVersion) is factored into its own helper and unit-tested across seven scenarios: old/new TiKV mix, TiFlash exclusion, tombstone handling, missing or malformed version strings, and a malformed minVersion constant. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
Drop the AllSameOrder gate at the start of matchProperty and replace the
per-column "is the index direction uniform?" check with a per-column
"does forward or reverse scan satisfy this column?" check. Concretely,
for each (sortItem, idxCol) pair we compute
thisScanDesc = sortItem.Desc != idxCol.Desc
and require all matched pairs to agree. matchProperty now records the
unified decision on path.MatchedScanDesc (renamed from MatchedIdxDesc),
so the consumer can set IndexScan.Desc directly without the previous
XOR-with-the-query-direction step.
This unlocks the MySQL 8.0 flagship case: INDEX(a ASC, b DESC) now
satisfies "ORDER BY a ASC, b DESC" with a forward cop scan, and
"ORDER BY a DESC, b ASC" with a reverse cop scan. Uniform requests
against an all-ascending index continue to work because the per-column
check collapses to a single boolean (all matched pairs share the same
sortItem.Desc, all share idxCol.Desc=false, so thisScanDesc is uniform).
Tests:
* TestDescendingIndexScanDirection: explicitly asserts that
INDEX(a, b DESC) ORDER BY a, b DESC keeps order without a Sort
and is NOT marked as a reverse scan; the inverted ORDER BY uses
the reverse-scan path; "ORDER BY a, b" still falls back to Sort.
* TestMixedDirectionCompositeIndex: end-to-end INSERT + SELECT,
asserting actual row order for both forward and reverse cases.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
The parser has accepted INDEX((expr) DESC) for years and the BuildHiddenColumnInfo rewrite that turns expression parts into synthesized hidden-column references preserves the IndexPartSpecification.Desc flag (it only mutates Column and Expr). With the metadata-plumbing commits in place, this means DESC on expression-index parts already works end-to-end — but there was no test guarding the path. Add one so a future refactor that drops Desc somewhere in the rewrite chain fails loudly rather than silently producing an ascending-stored expression index. The test asserts: the IndexColumn carries Desc=true, IndexInfo.Version=1, SHOW CREATE TABLE round-trips the DESC keyword, and information_schema.statistics returns collation 'D' for the synthesized hidden column. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
…ngcap#2519) Add a regression test that nails down what tidb_enable_descending_index does — and just as importantly, what it does NOT do. The gate controls whether *new* CREATE INDEX statements persist DESC into the schema. Flipping it off later must: * Leave existing DESC indexes intact (IndexColumn.Desc and IndexInfo.Version preserved). * Continue serving SELECT queries against those indexes. * Continue maintaining them on INSERT (GenIndexKey routes through the DESC path because the per-column flag is on the persisted IndexColumn, not derived from the live sysvar). * Drop DESC silently on any *new* index built while the gate is off, preserving MySQL 5.7 compatibility for migration scripts that may contain `INDEX (col DESC)` syntax for forward-compat. This codifies the contract so a future refactor that ties index behaviour to the live sysvar value (rather than the persisted Desc flag) fails loudly here. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
…pingcap#2519) Make the placeholder version more visible so a future maintainer doesn't accidentally treat it as a real release gate. The constant must be bumped in lockstep with the TiKV release that contains the matching Rust auto-detect logic; until that PR ships and is tagged, the placeholder 9.0.0 is intentionally above stable so the gate fails closed in any pre-release cluster. No behavioural change. Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
) Addresses CodeRabbit comments [4]/[5] on pingcap#68049: the path that builds key ranges for a CommonHandle index-join (and the planner's matchProperty when walking the appended PK suffix) both assume the clustered PK columns are ascending. Allowing DESC on a clustered PK would silently miscompare row keys. Add a DDL-time guard at two layers so the assumption is always true: * BuildTableInfo's first PRIMARY-KEY pass: when the constraint is going to produce a clustered index (PKIsHandle for single-int PKs or IsCommonHandle for composite), reject DESC on any of its key columns. This covers the PKIsHandle path, where the PK column becomes the row's int handle directly and BuildIndexInfo is never invoked. * BuildIndexInfo: same check after building IndexColumns, defending in depth and covering ALTER TABLE flows that may construct an IndexInfo without going through buildTableInfo's PK loop. NONCLUSTERED primary keys are unaffected — they're encoded just like unique secondary indexes and DESC works through the regular index path. Update the comment in find_best_task.go's CommonHandle suffix matching to point at this guard, since the planner relies on it to assume the appended PK columns are always ascending. Tests cover all three rejection paths (multi-column CLUSTERED, single- int CLUSTERED, alter-add NONCLUSTERED with DESC succeeds) plus a regular DESC index on a clustered table to confirm only PK columns are restricted. Signed-off-by: takaidohigasi <takaidohigasi@gmail.com>
takaidohigasi
force-pushed
the
feature/desc-index
branch
from
b1504e5 to
61a14ea
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/executor/admin.go (1)
858-870:⚠️ Potential issue | 🟠 MajorADMIN CLEANUP INDEX silently fails on DESC-leading secondary indexes due to incorrect range initialization.
When the index has a DESC leading column,
init()seedse.lastIdxKeywithGenIndexKey([NULL], MinInt64), which applies DESC-aware encoding: NULL complements to0xFFand sorts at the end of the index keyspace, not the start. The cleanup loop then overwritesStartKeywithlastIdxKey.PrefixNext()(line 829), stepping past the entire index range on the first iteration. The scan returns no results, and no dangling entries are found or deleted—making the operation silently ineffective.
IndexRangesToKVRangesWithDesccorrectly computes the full range given the DESC flags, but line 829 defeats it by overwriting the start boundary. Either seedlastIdxKeywith the actual lower-bound key (e.g., the index prefix itself soPrefixNextlands on the first physical key), or on the first iteration use theStartKeyfrom the range and only overwrite withlastIdxKey.PrefixNext()on subsequent iterations.Existing tests do not cover
ADMIN CLEANUP INDEXon DESC indexes, which is why this went undetected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/admin.go` around lines 858 - 870, The cleanup loop seeds e.lastIdxKey in CleanupIndexExec.init using GenIndexKey with a NULL and MinInt64 which yields a DESC-aware upper-most key; change the initialization so the scanner starts at the true lower bound: either (preferred) set e.lastIdxKey to the index prefix lower-bound (i.e., the raw index prefix key for the index columns) so that calling PrefixNext advances to the first physical key, or modify the cleanup loop that uses StartKey/lastIdxKey (the code that sets StartKey = lastIdxKey.PrefixNext()) to use the computed range.StartKey from IndexRangesToKVRangesWithDesc on the first iteration and only switch to lastIdxKey.PrefixNext() on subsequent iterations; update CleanupIndexExec.init (and the loop that reads lastIdxKey/StartKey) accordingly to ensure DESC-leading columns do not cause the start to skip the entire range.
🧹 Nitpick comments (6)
pkg/ddl/create_table.go (1)
1340-1351: Use a structureddbterrorcode instead oferrors.Errorffor the DESC-on-clustered-PK rejection.Other DDL rejections in this file (e.g.,
dbterror.ErrPKIndexCantBeInvisible,dbterror.ErrUnsupportedClusteredSecondaryKey,dbterror.ErrGeneralUnsupportedDDL.GenWithStackByArgs(...)) flow throughdbterror, which carries a MySQL-compatible error code and terror class. A bareerrors.Errorfhere means clients thatterror.IsCode-match (or look at MySQL error code) won't recognize this case, and the warning surface in tests/SHOW WARNINGS will look inconsistent.♻️ Suggested refactor
- for _, key := range constr.Keys { - if key.Desc { - return nil, errors.Errorf( - "DESC is not supported on the columns of a clustered PRIMARY KEY; either drop the DESC keyword or declare the primary key as NONCLUSTERED") - } - } + for _, key := range constr.Keys { + if key.Desc { + return nil, dbterror.ErrGeneralUnsupportedDDL.GenWithStackByArgs( + "DESC on the columns of a clustered PRIMARY KEY; either drop the DESC keyword or declare the primary key as NONCLUSTERED") + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ddl/create_table.go` around lines 1340 - 1351, The rejection for DESC on clustered PRIMARY KEY currently returns a plain errors.Errorf which lacks a MySQL-compatible dbterror code; replace that call inside the loop over constr.Keys (the block checking key.Desc) with a dbterror-based error—e.g. use dbterror.ErrGeneralUnsupportedDDL.GenWithStackByArgs("DESC is not supported on the columns of a clustered PRIMARY KEY; either drop the DESC keyword or declare the primary key as NONCLUSTERED") or, if a more specific dbterror constant exists, use that constant's GenWithStackByArgs variant so the error carries the proper code/class and integrates with terror.IsCode and SHOW WARNINGS.pkg/executor/builder.go (1)
4137-4138: Make the ASC-only common-handle invariant point to the DDL guard.These
desc=nilcall sites are correct, but the comment would be easier to trust if it named the upstream contract explicitly: clustered PRIMARY KEY columns are rejected withDESCat DDL time, so common-handle range building stays ASC-only.✏️ Suggested comment tweak
- // Common handle PK is always ascending (no per-column DESC), so desc=nil. + // Clustered PRIMARY KEY columns cannot be DESC (rejected at DDL time), + // so common-handle lookups always use ascending encoding and pass desc=nil.Based on learnings, clustered PRIMARY KEY
DESCis explicitly rejected inpkg/ddl/create_table.goandpkg/ddl/index.go.Also applies to: 4146-4146, 5115-5116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/executor/builder.go` around lines 4137 - 4138, Update the inline comment that currently states "Common handle PK is always ascending (no per-column DESC), so desc=nil." to explicitly reference the upstream DDL contract that enforces this: mention that clustered PRIMARY KEY columns with DESC are rejected at DDL time (see pkg/ddl/create_table.go and pkg/ddl/index.go), and indicate why the call to buildKvRangesForIndexJoin(..., desc=nil, ...) is therefore safe; update the other similar sites (around the buildKvRangesForIndexJoin call at the noted locations and lines ~4146 and ~5115) to the same explicit DDL-guard wording so readers can trust the ASC-only invariant.pkg/distsql/request_builder.go (1)
859-928: DESC encoding + swap logic looks correct, with one assumption worth restating in code.Walked through the swap/exclude/PrefixNext interaction for single-column DESC,
(asc, desc), and(desc, asc)composite cases — the half-open byte ranges produced match the semantic ranges in every case I tried. The doc comment calls out the right limitation:This one-shot swap is correct for single-column DESC ranges and for composite ranges whose differentiating column is DESC.
This is correct under the ranger invariant that within a single
*ranger.Range, all prefix columns are equality (LowVal[i] == HighVal[i]) and at most the trailing column has a non-equality bound. If two columns ever differ inLowVal/HighValof the sameRange, the byte-vs-semantic ordering can diverge in mixed-direction indexes and the one-shot swap would be insufficient. That invariant holds in current TiDB ranger output, but it's a quiet contract — consider adding a one-line note ("relies on ranger producing one-differing-column ranges") so a future planner change doesn't silently break this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/distsql/request_builder.go` around lines 859 - 928, The doc comment for EncodeIndexKeyWithDesc should explicitly state it relies on the ranger.Range invariant that all prefix columns are equal (LowVal[i] == HighVal[i]) and at most one trailing column can differ; update the comment above EncodeIndexKeyWithDesc to add a single sentence referencing this contract (e.g., "Relies on ranger producing ranges where only one column may differ between LowVal and HighVal") and mention ranger.Range/LowVal/HighVal so future readers know the swap logic assumes that invariant.pkg/meta/model/index_test.go (1)
94-100: Comment doesn't quite describe what's tested.The comment promises "unknown fields from a newer TiDB must decode cleanly on an older binary," but the test below just round-trips the known
Descfield via the same struct on the same binary — it doesn't simulate decoding JSON with truly unknown extra fields. Either retitle the comment to match what's tested (round-trip ofDescsurvives marshal/unmarshal, including viaClone), or extend the test to actually unmarshal a JSON blob containing an unrecognized key alongside"desc":trueand assert no error.📝 Comment-only clarification
- // Unknown fields from a newer TiDB must decode cleanly on an older binary. + // Round-trip: a marshalled DESC column unmarshals back with Desc=true, + // and Clone preserves the flag. var decoded IndexColumn require.NoError(t, json.Unmarshal(descJSON, &decoded)) require.True(t, decoded.Desc)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/meta/model/index_test.go` around lines 94 - 100, The current comment is misleading because the test only round-trips the known Desc field; change the test to actually simulate unknown fields by creating a JSON blob that contains "desc": true plus an extra unknown key (e.g., a new_field) and then json.Unmarshal that blob into the IndexColumn (using descJSON or a new variable like jsonWithExtra) and assert require.NoError(t, ...) and require.True(t, decoded.Desc); also keep the existing assertion that descCol.Clone().Desc is preserved, and update the comment to describe that the test verifies unmarshalling JSON with unknown fields into IndexColumn succeeds and that Clone preserves Desc.pkg/util/codec/codec.go (2)
345-369:EncodeKeyWithDescdefeatsencode's batchpreReallocby encoding one datum per call.
encode()opens withpreRealloc(b, vals, comparable1)to sizebfor the entire batch. Calling it per element (encode(loc, b, v[i:i+1], true)inside the loop) reduces that to a per-datum capacity check and may incur extrareallocBytescalls when encoding a wide composite key.A simple alternative: encode the full batch once, then walk it with
peekto find each column's byte range and complement only the DESC ones in place. This keeps a single capacity calculation and avoids per-element overhead. Not a correctness issue and not on the per-row decode hot path, so leaving as a recommended follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/codec/codec.go` around lines 345 - 369, EncodeKeyWithDesc currently calls encode per-datum which defeats encode's batch preRealloc; instead, call encode once for the full v slice (encode(loc, b, v, true)), then iterate through the encoded bytes using peek to locate each datum's byte range and apply the bytewise complement only for columns marked DESC in desc; keep the same comparable flag and behavior for types that disallow DESC (the existing check for types.KindMysqlJSON and types.KindVectorFloat32), and ensure you preserve the original start/stop boundaries discovered by peek so the in-place complement operates on the correct ranges.
406-425:CutOneWithDesccan callpeek(b)directly — no complement copy needed.Now that
peek()understands the DESC-complemented flags (lines 1631-1658) for both fixed- and variable-length types, cutting a DESC column doesn't need any scratch at all:peek(b)on the original DESC bytes already returns the encoded length. The current implementation allocateslen(b)bytes (the entire remaining key, not just this column) and complements them only to fall back into the ASC peek arm. The comment on lines 413–415 ("take the minimum slice needed") also doesn't match themake([]byte, len(b))that follows.This mirrors what
Decoder.DecodeOnealready does (peek first, then complement onlylengthbytes intodescBuf).♻️ Proposed simplification
func CutOneWithDesc(b []byte, desc bool) (data []byte, remain []byte, err error) { if !desc { return CutOne(b) } if len(b) < 1 { return nil, nil, errors.New("invalid encoded key") } - // peek() in cmpl-space gives the encoded length. We only need one byte of - // scratch (the flag) plus up to the encoded length for variable-length - // types, so take the minimum slice needed. - cp := make([]byte, len(b)) - for i := range b { - cp[i] = b[i] ^ 0xFF - } - l, err := peek(cp) + // peek understands DESC-complemented flags directly (see the ^*Flag arms + // in peek), so no scratch buffer is needed to cut past a DESC column. + l, err := peek(b) if err != nil { return nil, nil, errors.Trace(err) } return b[:l], b[l:], nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/util/codec/codec.go` around lines 406 - 425, CutOneWithDesc currently allocates and complements an entire copy of b before calling peek; instead call peek directly on the original DESC bytes (i.e. replace the cp allocation/complement loop with l, err := peek(b)) because peek already understands DESC-complemented flags for fixed and variable types, then keep the existing error handling and return b[:l], b[l:], nil; this mirrors Decoder.DecodeOne which only complements into a small descBuf when needed and avoids the unnecessary full-buffer allocation in CutOneWithDesc.
🤖 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/ddl/executor.go`:
- Around line 1110-1123: The TiKV-descending-index compatibility loop (iterating
tbInfo.Indices, calling idx.HasDescColumn() and checkTiKVSupportsDescIndex(ctx))
runs before the "IF NOT EXISTS" / OnExistIgnore fast-path, causing CREATE TABLE
IF NOT EXISTS to fail on old/unhealthy TiKV even when the table already exists;
move or delay this loop/check so it executes only after you determine the
OnExistIgnore short-circuit won't return (i.e., after the existing-table fast
path in the CreateTable/CreateTableWithInfo/BatchCreateTableWithInfo flow),
ensuring that when OnExistIgnore is true and the table is present you return
early without calling checkTiKVSupportsDescIndex.
- Around line 5009-5017: Replace the unbounded context used in
pdCli.GetAllStores (currently calling GetAllStores(context.Background())) with a
cancellable/timeout-bound context so PD lookups can't hang indefinitely; create
a context with a short timeout (or derive from the caller/session context) and
pass that to pdCli.GetAllStores, ensure you cancel the context after use, and
keep the subsequent call to checkStoresMeetDescIndexMinVersion(stores,
MinTiKVVersionForDescIndex, intest.InTest) unchanged.
In `@pkg/ddl/index.go`:
- Around line 136-140: The current check unconditionally rejects descending
order for columnar indexes (if ip.Desc && columnarIndexType !=
model.ColumnarIndexTypeNA) which breaks the compatibility path when descending
indexes are globally disabled; update the condition to also require the
descending-index feature flag (e.g., descEnabled) so that the rejection only
happens when DESC is enabled: change the guard to something like if descEnabled
&& ip.Desc && columnarIndexType != model.ColumnarIndexTypeNA and keep the
existing error construction via
dbterror.ErrUnsupportedAddColumnarIndex.FastGenByArgs(columnarIndexType.SQLName())
so VECTOR/INVERTED index clauses remain no-ops when descEnabled is false.
- Line 121: The code currently recomputes descEnabled using
vardef.EnableDescendingIndex.Load() inside buildIndexColumns (and the similar
block at lines ~184-191), causing the DESC/ASC decision to depend on global
state at execution time; instead use the persisted decision stored in the DDL
job/index metadata (e.g., the IndexPart or IndexInfo field like ip.Desc or the
flag set by checkAndBuildIndexInfo) so the create-time choice is honored.
Replace references to vardef.EnableDescendingIndex.Load() in buildIndexColumns
and the other block with the already-stored DESC boolean passed into or read
from the IndexPart/IndexInfo, and if necessary change buildIndexColumns'
signature or its call sites (including checkAndBuildIndexInfo) to accept that
persisted desc flag rather than re-reading the global gate.
In `@pkg/executor/index_merge_reader.go`:
- Around line 194-195: In Open(), don't ignore the error returned by
distsql.IndexRangesToKVRangesWithDesc: capture its error return, and if non-nil
return it (with contextual message referencing the table/index and the range)
instead of proceeding to assign e.partitionKeyRanges; update the call site
around IndexRangesToKVRangesWithDesc (and references to indexColDescFlags,
e.ranges, and e.partitionKeyRanges) to check the error and propagate it up from
Open() so range-encoding failures are surfaced.
In `@pkg/planner/core/planbuilder.go`:
- Around line 4103-4110: The current servability check (checkAllIndicesServable)
is only applied for INSERT/REPLACE but must also be enforced for LOAD DATA and
IMPORT INTO because buildLoadData and buildImportInto can mutate secondary
indexes without using getPossibleAccessPaths; add calls to
checkAllIndicesServable(tableInfo) at the start of buildLoadData and
buildImportInto (or factor it into the shared planner entry used by those flows)
and return the error if non-nil so older TiDB binaries will refuse operations
that would write indexes with unsupported metadata.
---
Outside diff comments:
In `@pkg/executor/admin.go`:
- Around line 858-870: The cleanup loop seeds e.lastIdxKey in
CleanupIndexExec.init using GenIndexKey with a NULL and MinInt64 which yields a
DESC-aware upper-most key; change the initialization so the scanner starts at
the true lower bound: either (preferred) set e.lastIdxKey to the index prefix
lower-bound (i.e., the raw index prefix key for the index columns) so that
calling PrefixNext advances to the first physical key, or modify the cleanup
loop that uses StartKey/lastIdxKey (the code that sets StartKey =
lastIdxKey.PrefixNext()) to use the computed range.StartKey from
IndexRangesToKVRangesWithDesc on the first iteration and only switch to
lastIdxKey.PrefixNext() on subsequent iterations; update CleanupIndexExec.init
(and the loop that reads lastIdxKey/StartKey) accordingly to ensure DESC-leading
columns do not cause the start to skip the entire range.
---
Nitpick comments:
In `@pkg/ddl/create_table.go`:
- Around line 1340-1351: The rejection for DESC on clustered PRIMARY KEY
currently returns a plain errors.Errorf which lacks a MySQL-compatible dbterror
code; replace that call inside the loop over constr.Keys (the block checking
key.Desc) with a dbterror-based error—e.g. use
dbterror.ErrGeneralUnsupportedDDL.GenWithStackByArgs("DESC is not supported on
the columns of a clustered PRIMARY KEY; either drop the DESC keyword or declare
the primary key as NONCLUSTERED") or, if a more specific dbterror constant
exists, use that constant's GenWithStackByArgs variant so the error carries the
proper code/class and integrates with terror.IsCode and SHOW WARNINGS.
In `@pkg/distsql/request_builder.go`:
- Around line 859-928: The doc comment for EncodeIndexKeyWithDesc should
explicitly state it relies on the ranger.Range invariant that all prefix columns
are equal (LowVal[i] == HighVal[i]) and at most one trailing column can differ;
update the comment above EncodeIndexKeyWithDesc to add a single sentence
referencing this contract (e.g., "Relies on ranger producing ranges where only
one column may differ between LowVal and HighVal") and mention
ranger.Range/LowVal/HighVal so future readers know the swap logic assumes that
invariant.
In `@pkg/executor/builder.go`:
- Around line 4137-4138: Update the inline comment that currently states "Common
handle PK is always ascending (no per-column DESC), so desc=nil." to explicitly
reference the upstream DDL contract that enforces this: mention that clustered
PRIMARY KEY columns with DESC are rejected at DDL time (see
pkg/ddl/create_table.go and pkg/ddl/index.go), and indicate why the call to
buildKvRangesForIndexJoin(..., desc=nil, ...) is therefore safe; update the
other similar sites (around the buildKvRangesForIndexJoin call at the noted
locations and lines ~4146 and ~5115) to the same explicit DDL-guard wording so
readers can trust the ASC-only invariant.
In `@pkg/meta/model/index_test.go`:
- Around line 94-100: The current comment is misleading because the test only
round-trips the known Desc field; change the test to actually simulate unknown
fields by creating a JSON blob that contains "desc": true plus an extra unknown
key (e.g., a new_field) and then json.Unmarshal that blob into the IndexColumn
(using descJSON or a new variable like jsonWithExtra) and assert
require.NoError(t, ...) and require.True(t, decoded.Desc); also keep the
existing assertion that descCol.Clone().Desc is preserved, and update the
comment to describe that the test verifies unmarshalling JSON with unknown
fields into IndexColumn succeeds and that Clone preserves Desc.
In `@pkg/util/codec/codec.go`:
- Around line 345-369: EncodeKeyWithDesc currently calls encode per-datum which
defeats encode's batch preRealloc; instead, call encode once for the full v
slice (encode(loc, b, v, true)), then iterate through the encoded bytes using
peek to locate each datum's byte range and apply the bytewise complement only
for columns marked DESC in desc; keep the same comparable flag and behavior for
types that disallow DESC (the existing check for types.KindMysqlJSON and
types.KindVectorFloat32), and ensure you preserve the original start/stop
boundaries discovered by peek so the in-place complement operates on the correct
ranges.
- Around line 406-425: CutOneWithDesc currently allocates and complements an
entire copy of b before calling peek; instead call peek directly on the original
DESC bytes (i.e. replace the cp allocation/complement loop with l, err :=
peek(b)) because peek already understands DESC-complemented flags for fixed and
variable types, then keep the existing error handling and return b[:l], b[l:],
nil; this mirrors Decoder.DecodeOne which only complements into a small descBuf
when needed and avoids the unnecessary full-buffer allocation in CutOneWithDesc.
🪄 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: 164a269f-29fd-41ea-b927-5543085e2bda
📒 Files selected for processing (27)
pkg/ddl/create_table.gopkg/ddl/db_integration_test.gopkg/ddl/desc_index_version_check_test.gopkg/ddl/executor.gopkg/ddl/index.gopkg/distsql/request_builder.gopkg/executor/admin.gopkg/executor/builder.gopkg/executor/distsql.gopkg/executor/executor_pkg_test.gopkg/executor/index_merge_reader.gopkg/executor/infoschema_reader.gopkg/executor/show.gopkg/meta/model/index.gopkg/meta/model/index_test.gopkg/planner/core/find_best_task.gopkg/planner/core/operator/physicalop/physical_index_scan.gopkg/planner/core/planbuilder.gopkg/planner/util/path.gopkg/planner/util/path_test.gopkg/sessionctx/vardef/tidb_vars.gopkg/sessionctx/variable/sysvar.gopkg/table/tables/mutation_checker.gopkg/tablecodec/tablecodec.gopkg/tablecodec/tablecodec_test.gopkg/util/codec/codec.gopkg/util/codec/codec_test.go
✅ Files skipped from review due to trivial changes (2)
- pkg/util/codec/codec_test.go
- pkg/ddl/db_integration_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/executor/show.go
- pkg/table/tables/mutation_checker.go
- pkg/ddl/desc_index_version_check_test.go
- pkg/executor/executor_pkg_test.go
Comment on lines
+1110
to
+1123
| // Run the descending-index TiKV-version gate at the shared chokepoint | ||
| // for CREATE TABLE so CreateTableWithInfo and BatchCreateTableWithInfo | ||
| // are covered alongside CreateTable. If any index in the about-to-be- | ||
| // persisted TableInfo carries Desc=true and the cluster contains a | ||
| // TiKV that cannot decode descending keys, fail early before the DDL | ||
| // job is queued. See pingcap/tidb#2519. | ||
| for _, idx := range tbInfo.Indices { | ||
| if idx.HasDescColumn() { | ||
| if err = checkTiKVSupportsDescIndex(ctx); err != nil { | ||
| return nil, errors.Trace(err) | ||
| } | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
Delay the DESC-index preflight until after the existing-table fast path.
Line 1110 runs the TiKV compatibility check before the OnExistIgnore short-circuit. That means CREATE TABLE IF NOT EXISTS ... can now fail on an old or temporarily unhealthy cluster even when the table already exists and no DDL job would be queued.
💡 Suggested fix
- // Run the descending-index TiKV-version gate at the shared chokepoint
- // for CREATE TABLE so CreateTableWithInfo and BatchCreateTableWithInfo
- // are covered alongside CreateTable. If any index in the about-to-be-
- // persisted TableInfo carries Desc=true and the cluster contains a
- // TiKV that cannot decode descending keys, fail early before the DDL
- // job is queued. See pingcap/tidb#2519.
- for _, idx := range tbInfo.Indices {
- if idx.HasDescColumn() {
- if err = checkTiKVSupportsDescIndex(ctx); err != nil {
- return nil, errors.Trace(err)
- }
- break
- }
- }
-
if err = handleTablePlacement(ctx, tbInfo); err != nil {
return nil, errors.Trace(err)
}
@@
if err := checkTableInfoValidExtra(ctx.GetSessionVars().StmtCtx.ErrCtx(), ctx.GetStore(), dbName, tbInfo); err != nil {
return nil, err
}
+
+ // Run the descending-index TiKV-version gate only once we know the DDL
+ // will actually be enqueued.
+ for _, idx := range tbInfo.Indices {
+ if idx.HasDescColumn() {
+ if err = checkTiKVSupportsDescIndex(ctx); err != nil {
+ return nil, errors.Trace(err)
+ }
+ break
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/ddl/executor.go` around lines 1110 - 1123, The TiKV-descending-index
compatibility loop (iterating tbInfo.Indices, calling idx.HasDescColumn() and
checkTiKVSupportsDescIndex(ctx)) runs before the "IF NOT EXISTS" / OnExistIgnore
fast-path, causing CREATE TABLE IF NOT EXISTS to fail on old/unhealthy TiKV even
when the table already exists; move or delay this loop/check so it executes only
after you determine the OnExistIgnore short-circuit won't return (i.e., after
the existing-table fast path in the
CreateTable/CreateTableWithInfo/BatchCreateTableWithInfo flow), ensuring that
when OnExistIgnore is true and the table is present you return early without
calling checkTiKVSupportsDescIndex.
Comment on lines
+5009
to
+5017
| stores, err := pdCli.GetAllStores(context.Background()) | ||
| if err != nil { | ||
| return errors.Annotate(err, "checking TiKV cluster version for descending index") | ||
| } | ||
| // In production we fail closed on missing versions. The mock store | ||
| // fixture used in `intest` builds reports empty versions for every | ||
| // replica; tolerate that case so integration tests can exercise | ||
| // descending-index DDL without spinning up a real PD. | ||
| return checkStoresMeetDescIndexMinVersion(stores, MinTiKVVersionForDescIndex, intest.InTest) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that the DESC-index TiKV gate currently uses an unbounded background context.
rg -n -C2 'GetAllStores\(context\.Background\(\)\)' --type=goRepository: pingcap/tidb
Length of output: 1488
🏁 Script executed:
#!/bin/bash
# Find the definition of checkTiKVSupportsDescIndex
rg -n 'func.*checkTiKVSupportsDescIndex' --type=go -A 10Repository: pingcap/tidb
Length of output: 626
🏁 Script executed:
#!/bin/bash
# Find all calls to checkTiKVSupportsDescIndex to see what context is passed
rg -n 'checkTiKVSupportsDescIndex' --type=go -B 3 -A 3Repository: pingcap/tidb
Length of output: 2342
🏁 Script executed:
#!/bin/bash
# Look at the CreateTableWithInfo function around where it calls checkTiKVSupportsDescIndex
rg -n 'CreateTableWithInfo' pkg/ddl/executor.go -A 20 | head -80Repository: pingcap/tidb
Length of output: 3436
🏁 Script executed:
#!/bin/bash
# Find sessionctx.Context interface definition to see available methods
rg -n 'type.*Context.*interface' sessionctx/ --type=go -A 30 | head -100Repository: pingcap/tidb
Length of output: 108
🏁 Script executed:
#!/bin/bash
# Look for how other DDL operations handle context when making external calls
rg -n 'context\.WithTimeout\|context\.WithDeadline' pkg/ddl/ --type=go -B 3 -A 3Repository: pingcap/tidb
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Search for other PD client calls in DDL executor to see patterns
rg -n 'pdCli\.|pdClient\.' pkg/ddl/executor.go -B 2 -A 2Repository: pingcap/tidb
Length of output: 255
🏁 Script executed:
#!/bin/bash
# Find sessionctx.Context interface definition
find . -type f -name "*.go" | xargs rg -l 'type Context interface' | head -5Repository: pingcap/tidb
Length of output: 140
🏁 Script executed:
#!/bin/bash
# Look for sessionctx package/context
rg -n 'package sessionctx' --type=go | head -3Repository: pingcap/tidb
Length of output: 207
🏁 Script executed:
#!/bin/bash
# Check imports in executor.go to find where sessionctx comes from
head -50 pkg/ddl/executor.go | grep -E 'import|sessionctx'Repository: pingcap/tidb
Length of output: 65
🏁 Script executed:
#!/bin/bash
# Get full imports from executor.go
sed -n '1,80p' pkg/ddl/executor.go | grep -A 80 'import'Repository: pingcap/tidb
Length of output: 2309
🏁 Script executed:
#!/bin/bash
# Read sessionctx.Context interface definition
cat -n pkg/sessionctx/context.go | head -150Repository: pingcap/tidb
Length of output: 8237
🏁 Script executed:
#!/bin/bash
# Look for context extraction or deadline methods in sessionctx
rg -n 'func.*Context.*\(.*context\.Context' pkg/sessionctx/ --type=goRepository: pingcap/tidb
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Continue reading sessionctx.Context interface
sed -n '80,300p' pkg/sessionctx/context.go | cat -nRepository: pingcap/tidb
Length of output: 7451
🏁 Script executed:
#!/bin/bash
# Search for GetCtx or similar context methods in sessionctx.Context
rg -n 'func.*Context.*Ctx|GetContext|GetCtx' pkg/sessionctx/context.goRepository: pingcap/tidb
Length of output: 38
🏁 Script executed:
#!/antml/bash
# Look for how other parts of code use sessionctx.Context with context.Context
rg -n 'ctx.*sessionctx\.Context' pkg/ddl/ --type=go -A 2 | head -50Repository: pingcap/tidb
Length of output: 3660
🏁 Script executed:
#!/antml:bash
# Look for GetTraceCtx usage to understand if it could be used
rg -n 'GetTraceCtx' pkg/ddl/ --type=go -A 2 -B 2Repository: pingcap/tidb
Length of output: 1270
🏁 Script executed:
#!/bin/bash
# Look for timeout patterns in DDL code to see what's typical
rg -n 'context\.WithTimeout|time\.' pkg/ddl/executor.go --type=go | grep -i 'timeout\|second' | head -20Repository: pingcap/tidb
Length of output: 583
🏁 Script executed:
#!/bin/bash
# Check how other external calls (like PD) are handled in DDL with context
rg -n 'pdCli\.|pdClient\.|GetRegionCache' pkg/ddl/ --type=go -B 3 -A 3 | head -60Repository: pingcap/tidb
Length of output: 4075
🏁 Script executed:
#!/bin/bash
# Check if sessionctx.Context or related provides statement timeout/deadline info
rg -n 'GetMaxExecutionTime|MaxExecutionTime|stmtCtx.*Timeout|deadline' pkg/sessionctx/ --type=go -A 2 -B 2Repository: pingcap/tidb
Length of output: 5497
🏁 Script executed:
#!/bin/bash
# Look for whether preflight checks elsewhere use timeouts or unbounded contexts
rg -n 'preflight|pre-flight|version.*check' pkg/ddl/executor.go --type=go -A 3 -B 3 | head -60Repository: pingcap/tidb
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check how other KV/store operations bound their contexts in executor
rg -n 'context\.WithTimeout.*time\.' pkg/ddl/ --type=go -B 2 -A 2 | head -40Repository: pingcap/tidb
Length of output: 1335
Bound the PD store lookup with a timeout or cancellable context.
GetAllStores(context.Background()) makes this preflight check uninterruptible from the SQL session. If PD stalls, CREATE INDEX / CREATE TABLE with descending columns can hang until the client library times out instead of failing promptly.
💡 Suggested fix
- stores, err := pdCli.GetAllStores(context.Background())
+ pdCtx := kv.WithInternalSourceType(context.Background(), kv.InternalTxnDDL)
+ pdCtx, cancel := context.WithTimeout(pdCtx, 5*time.Second)
+ defer cancel()
+ stores, err := pdCli.GetAllStores(pdCtx)
if err != nil {
return errors.Annotate(err, "checking TiKV cluster version for descending index")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/ddl/executor.go` around lines 5009 - 5017, Replace the unbounded context
used in pdCli.GetAllStores (currently calling
GetAllStores(context.Background())) with a cancellable/timeout-bound context so
PD lookups can't hang indefinitely; create a context with a short timeout (or
derive from the caller/session context) and pass that to pdCli.GetAllStores,
ensure you cancel the context after use, and keep the subsequent call to
checkStoresMeetDescIndexMinVersion(stores, MinTiKVVersionForDescIndex,
intest.InTest) unchanged.
| maxIndexLength := config.GetGlobalConfig().MaxIndexLength | ||
| // The sum of length of all index columns. | ||
| sumLength := 0 | ||
| descEnabled := vardef.EnableDescendingIndex.Load() |
There was a problem hiding this comment.
Persist the DESC decision in the DDL job instead of rereading the global gate here.
buildIndexColumns() is also reached from checkAndBuildIndexInfo() when the DDL owner replays CREATE INDEX / ADD PRIMARY KEY. Recomputing ip.Desc && vardef.EnableDescendingIndex.Load() at that point makes the final metadata depend on whatever the owner's global cache says when the job runs, not what was in effect when the statement was submitted. A SET GLOBAL tidb_enable_descending_index between submission and execution can therefore silently turn the same DDL into ASC or DESC, which breaks the intended create-time-only semantics.
Also applies to: 184-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/ddl/index.go` at line 121, The code currently recomputes descEnabled
using vardef.EnableDescendingIndex.Load() inside buildIndexColumns (and the
similar block at lines ~184-191), causing the DESC/ASC decision to depend on
global state at execution time; instead use the persisted decision stored in the
DDL job/index metadata (e.g., the IndexPart or IndexInfo field like ip.Desc or
the flag set by checkAndBuildIndexInfo) so the create-time choice is honored.
Replace references to vardef.EnableDescendingIndex.Load() in buildIndexColumns
and the other block with the already-stored DESC boolean passed into or read
from the IndexPart/IndexInfo, and if necessary change buildIndexColumns'
signature or its call sites (including checkAndBuildIndexInfo) to accept that
persisted desc flag rather than re-reading the global gate.
Comment on lines
+136
to
+140
| // Columnar indexes cannot support descending order; reject explicitly. | ||
| // Fulltext also has its own DESC check in buildFullTextInfoWithCheck. | ||
| if ip.Desc && columnarIndexType != model.ColumnarIndexTypeNA { | ||
| return nil, false, dbterror.ErrUnsupportedAddColumnarIndex.FastGenByArgs(fmt.Sprintf("%s does not support DESC order", columnarIndexType.SQLName())) | ||
| } |
There was a problem hiding this comment.
Keep the gate-off compatibility path for non-FULLTEXT columnar indexes.
This rejection is unconditional, so tidb_enable_descending_index = OFF still turns ... DESC into an error for VECTOR/INVERTED indexes. Before this change those clauses were effectively ignored, so the new gate no longer preserves existing behavior for those index types. If the contract is “OFF means no-op,” this check should be conditioned on descEnabled as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/ddl/index.go` around lines 136 - 140, The current check unconditionally
rejects descending order for columnar indexes (if ip.Desc && columnarIndexType
!= model.ColumnarIndexTypeNA) which breaks the compatibility path when
descending indexes are globally disabled; update the condition to also require
the descending-index feature flag (e.g., descEnabled) so that the rejection only
happens when DESC is enabled: change the guard to something like if descEnabled
&& ip.Desc && columnarIndexType != model.ColumnarIndexTypeNA and keep the
existing error construction via
dbterror.ErrUnsupportedAddColumnarIndex.FastGenByArgs(columnarIndexType.SQLName())
so VECTOR/INVERTED index clauses remain no-ops when descEnabled is false.
Comment on lines
194
to
195
| keyRange, _ := distsql.IndexRangesToKVRangesWithDesc(e.ctx.GetDistSQLCtx(), e.table.Meta().ID, idx.ID, indexColDescFlags(idx), e.ranges[i]) | ||
| e.partitionKeyRanges[i] = [][]kv.KeyRange{keyRange.FirstPartitionRange()} |
There was a problem hiding this comment.
Propagate key-range conversion errors in Open().
At Line 194, the error from IndexRangesToKVRangesWithDesc is dropped. This can hide range-encoding failures and continue with bad/empty key ranges.
🔧 Proposed fix
- keyRange, _ := distsql.IndexRangesToKVRangesWithDesc(e.ctx.GetDistSQLCtx(), e.table.Meta().ID, idx.ID, indexColDescFlags(idx), e.ranges[i])
+ keyRange, err := distsql.IndexRangesToKVRangesWithDesc(e.ctx.GetDistSQLCtx(), e.table.Meta().ID, idx.ID, indexColDescFlags(idx), e.ranges[i])
+ if err != nil {
+ return err
+ }
e.partitionKeyRanges[i] = [][]kv.KeyRange{keyRange.FirstPartitionRange()}📝 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
| keyRange, _ := distsql.IndexRangesToKVRangesWithDesc(e.ctx.GetDistSQLCtx(), e.table.Meta().ID, idx.ID, indexColDescFlags(idx), e.ranges[i]) | |
| e.partitionKeyRanges[i] = [][]kv.KeyRange{keyRange.FirstPartitionRange()} | |
| keyRange, err := distsql.IndexRangesToKVRangesWithDesc(e.ctx.GetDistSQLCtx(), e.table.Meta().ID, idx.ID, indexColDescFlags(idx), e.ranges[i]) | |
| if err != nil { | |
| return err | |
| } | |
| e.partitionKeyRanges[i] = [][]kv.KeyRange{keyRange.FirstPartitionRange()} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/executor/index_merge_reader.go` around lines 194 - 195, In Open(), don't
ignore the error returned by distsql.IndexRangesToKVRangesWithDesc: capture its
error return, and if non-nil return it (with contextual message referencing the
table/index and the range) instead of proceeding to assign e.partitionKeyRanges;
update the call site around IndexRangesToKVRangesWithDesc (and references to
indexColDescFlags, e.ranges, and e.partitionKeyRanges) to check the error and
propagate it up from Open() so range-encoding failures are surfaced.
Comment on lines
+4103
to
+4110
| // Refuse INSERT/REPLACE if any of the table's indexes uses a metadata | ||
| // version newer than this TiDB binary understands — maintaining such an | ||
| // index would risk wrong rows or mismatched encoding (pingcap/tidb#2519). | ||
| // SELECT/UPDATE/DELETE plans are guarded by getPossibleAccessPaths, but | ||
| // INSERT VALUES never enumerates access paths, so check explicitly here. | ||
| if err := checkAllIndicesServable(tableInfo); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Extend the servability fence to LOAD DATA and IMPORT INTO.
Line 4108 fixes INSERT/REPLACE, but buildLoadData and buildImportInto also write secondary indexes without going through getPossibleAccessPaths. On an older TiDB binary, those paths can still mutate a public DESC index with unsupported metadata and silently mis-maintain it.
Suggested follow-up
@@
func (b *PlanBuilder) buildLoadData(ctx context.Context, ld *ast.LoadDataStmt) (base.Plan, error) {
@@
tableInPlan, ok := b.is.TableByID(ctx, tableInfo.ID)
if !ok {
db := b.ctx.GetSessionVars().CurrentDB
return nil, infoschema.ErrTableNotExists.FastGenByArgs(db, tableInfo.Name.O)
}
+ if err := checkAllIndicesServable(tableInPlan.Meta()); err != nil {
+ return nil, err
+ }
schema, names, err := expression.TableInfo2SchemaAndNames(b.ctx.GetExprCtx(), ast.NewCIStr(""), tableInfo)
@@
func (b *PlanBuilder) buildImportInto(ctx context.Context, ld *ast.ImportIntoStmt) (base.Plan, error) {
@@
tableInPlan, ok := latestIS.TableByID(ctx, tableInfo.ID)
if !ok {
@@
return nil, infoschema.ErrTableNotExists.FastGenByArgs(db, tableInfo.Name.O)
}
+ if err := checkAllIndicesServable(tableInPlan.Meta()); err != nil {
+ return nil, err
+ }
schema, names, err := expression.TableInfo2SchemaAndNames(b.ctx.GetExprCtx(), ast.NewCIStr(""), tableInfo)📝 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
| // Refuse INSERT/REPLACE if any of the table's indexes uses a metadata | |
| // version newer than this TiDB binary understands — maintaining such an | |
| // index would risk wrong rows or mismatched encoding (pingcap/tidb#2519). | |
| // SELECT/UPDATE/DELETE plans are guarded by getPossibleAccessPaths, but | |
| // INSERT VALUES never enumerates access paths, so check explicitly here. | |
| if err := checkAllIndicesServable(tableInfo); err != nil { | |
| return nil, err | |
| } | |
| // In buildLoadData (around the tableInPlan resolution): | |
| tableInPlan, ok := b.is.TableByID(ctx, tableInfo.ID) | |
| if !ok { | |
| db := b.ctx.GetSessionVars().CurrentDB | |
| return nil, infoschema.ErrTableNotExists.FastGenByArgs(db, tableInfo.Name.O) | |
| } | |
| if err := checkAllIndicesServable(tableInPlan.Meta()); err != nil { | |
| return nil, err | |
| } | |
| schema, names, err := expression.TableInfo2SchemaAndNames(b.ctx.GetExprCtx(), ast.NewCIStr(""), tableInfo) | |
| // In buildImportInto (around the tableInPlan resolution): | |
| tableInPlan, ok := latestIS.TableByID(ctx, tableInfo.ID) | |
| if !ok { | |
| db := b.ctx.GetSessionVars().CurrentDB | |
| return nil, infoschema.ErrTableNotExists.FastGenByArgs(db, tableInfo.Name.O) | |
| } | |
| if err := checkAllIndicesServable(tableInPlan.Meta()); err != nil { | |
| return nil, err | |
| } | |
| schema, names, err := expression.TableInfo2SchemaAndNames(b.ctx.GetExprCtx(), ast.NewCIStr(""), tableInfo) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/planner/core/planbuilder.go` around lines 4103 - 4110, The current
servability check (checkAllIndicesServable) is only applied for INSERT/REPLACE
but must also be enforced for LOAD DATA and IMPORT INTO because buildLoadData
and buildImportInto can mutate secondary indexes without using
getPossibleAccessPaths; add calls to checkAllIndicesServable(tableInfo) at the
start of buildLoadData and buildImportInto (or factor it into the shared planner
entry used by those flows) and return the error if non-nil so older TiDB
binaries will refuse operations that would write indexes with unsupported
metadata.
…Reader (pingcap#2519) Addresses CodeRabbit round-2 feedback on the global-index branch in IndexMergeReaderExecutor.Open: the previous code discarded the error and could surface a downstream nil-keyrange panic. Return the error directly so callers see a clear failure. Signed-off-by: Takafumi Hojo <taka-h@mercari.com>
…p#2519) Addresses CodeRabbit round-2 feedback: bulk-load paths write every secondary index of the target table just like INSERT does, so they must run through the same servability fence. Without this, an unservable IndexInfo (e.g. version produced by a future TiDB and loaded back into an older one) would be silently mis-maintained at load time. Signed-off-by: Takafumi Hojo <taka-h@mercari.com>
…cap#2519) Addresses CodeRabbit round-2 feedback. Previously buildIndexColumns re-read the global atomic at job-execution time, so a `SET GLOBAL tidb_enable_descending_index = OFF` between statement submission and DDL owner replay could silently flip the persisted schema layout. Move the gate into a new ApplyDescGateToIndexParts helper that walks the AST IndexPartSpecification slice and zeroes out every Desc flag when the feature is disabled. Call it at every CREATE-time chokepoint (CreateTable / CreateIndex / CreatePrimaryKey / createColumnarIndex) before the DDL job is enqueued. The decision is now fully baked into the AST, independent of any later toggling. As a side-effect this also restores the historical "DESC silently dropped" behaviour for columnar (vector / inverted / fulltext) indexes when the gate is off — the columnar-DESC reject inside buildIndexColumns only fires when the user explicitly opted in. Signed-off-by: Takafumi Hojo <taka-h@mercari.com>
…ngcap#2519) Addresses CodeRabbit round-2 feedback in createTableWithInfoJob. The descending-index TiKV-version gate previously ran before the OnExistIgnore branch, so a no-op \`CREATE TABLE IF NOT EXISTS\` against an already-existing table would still hit PD. Move the preflight loop after the existence check so the no-op path stays purely local. Signed-off-by: Takafumi Hojo <taka-h@mercari.com>
Addresses CodeRabbit round-2 feedback. checkTiKVSupportsDescIndex previously called pdCli.GetAllStores(context.Background()), which could hang DDL indefinitely if PD is unreachable. Wrap the call with a 5-second timeout so an unhealthy cluster surfaces as a fast, clear error instead of a stuck CREATE TABLE. Signed-off-by: Takafumi Hojo <taka-h@mercari.com>
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the For example:
📖 For more info, you can check the "Contribute Code" section in the development guide. |
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
1 participant
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: close #2519
Problem Summary:
The
DESCkeyword inCREATE INDEX (col DESC)is parsed today butsilently ignored — a MySQL 5.7 compatibility behaviour TiDB has carried
forward since 2018 (see issue #2519). MySQL 8.0 supports descending
indexes natively, and users migrating from MySQL 8.0 or running queries
that need a descending sort over a composite key (e.g.
ORDER BY a ASC, b DESCagainst a covering index) currently must eitherrewrite their schema or accept a planner-inserted Sort operator.
This PR makes
DESCtake effect end-to-end: it persists through indexmetadata, the storage encoder, the planner, the read path, and round-
trips out via
SHOW CREATE TABLEandINFORMATION_SCHEMA.STATISTICS.The companion TiKV PR is tikv/tikv#19558, which teaches the
coprocessor's key decoder to recognise DESC-complemented flag bytes.
The companion documentation PR is pingcap/docs#22805.
What changed and how does it work?
Per-column
Descis added tomodel.IndexColumnand written into theindex key as the bitwise complement of the ascending memcomparable
bytes, so a forward RocksDB iterator yields the declared direction. The
planner satisfies
ORDER BYby computing whether a forward or reversecop scan matches per-column directions; mixed composites like
INDEX(a ASC, b DESC)satisfyORDER BY a ASC, b DESCwith a singleforward scan and no Sort.
Three-layer rolling-upgrade defence:
tidb_enable_descending_index(default OFF). Whenoff,
DESCis silently dropped at DDL time, preserving MySQL 5.7migration-script behaviour.
non-TiFlash store reports a TiKV version below
MinTiKVVersionForDescIndex, the DDL is rejected with a clear"upgrade TiKV" message. Prevents writing DESC-encoded keys to a
cluster that can't decode them.
IndexInfo.Versionfence: the field bumps to 1 whenever anycolumn is DESC.
IndexInfo.IsServableis checked at plan time andin INSERT planning, so an older TiDB binary reading a Version=1
index from schema JSON refuses to serve queries rather than
silently dropping the unknown field.
Commit structure (12 commits):
*: plumb per-column DESC flag through index metadataplanner: honor IndexColumn.Desc when matching sort propertycodec: add EncodeKeyWithDesc/DecodeOneWithDesc primitivestablecodec: encode DESC index columns with complemented bytesdistsql,executor,tables: wire DESC encoding through read pathscodec: auto-detect DESC flag bytes in chunk Decoderplanner,model: enforce IndexInfo.IsServable at plan timeddl: gate CREATE INDEX on TiKV cluster versionplanner: support mixed-direction composite indexesddl: regression test for DESC on expression-index partsddl: lock down sysvar create-time-only semanticsddl: clarify MinTiKVVersionForDescIndex is a release-coordinated TODOCheck List
Tests
Coverage:
pkg/util/codec— primitive round-trips, range-sentinel ordering, JSON/Vector rejectionpkg/meta/model— IndexColumn JSON round-trip, IsServable, HasDescColumnpkg/ddl— 8 dedicated tests including end-to-end INSERT+SELECT through MockStorepkg/planner/core/casetest/ruleandpkg/planner/core/issuetest— no regressionsbazel build --config=ci //... --nogo— green at tipSide effects
When
tidb_enable_descending_indexis OFF (the default), behaviour isbyte-for-byte identical to current TiDB. The
IsServablefence does notadd a regression for any existing index because
Version=0indexes areunconditionally serviceable.
Documentation
Adds
tidb_enable_descending_indexsysvar; enables previously-ignoredDESCsyntax inCREATE INDEX; brings TiDB closer to MySQL 8.0 fordescending-index behaviour. Documentation updates covering the new
sysvar entry, the
CREATE INDEXsyntax page, and the MySQLcompatibility list are tracked in pingcap/docs#22805.
Release note
Summary by CodeRabbit
Release Notes
New Features
tidb_enable_descending_indexsystem variable.SHOW INDEX/SHOW CREATE TABLEoutput.Bug Fixes
Tests