Skip to content

unionstore/art: avoid checkptr panic after final prefix chunk (tidb-8.5) - #2150

Merged
ti-chi-bot[bot] merged 1 commit into
tikv:tidb-8.5from
YangKeao:codex/art-checkptr-tidb85
Oct 27, 2026
Merged

ti-chi-bot[bot] merged 1 commit into
tikv:tidb-8.5from
YangKeao:codex/art-checkptr-tidb85

Conversation

@YangKeao

@YangKeao YangKeao commented Oct 27, 2026 •

Copy link
Copy Markdown
Member

Backport of #2119 to tidb-8.5, on top of the merged Go 1.27 upgrade #2122.

Go 1.27 race/checkptr testing aborts when ART prefix comparison advances a pointer past the final matching 8-byte chunk. This currently blocks pingcap/tidb#72435 in both unit-test variants and the real-TiKV import tests. Return before advancing beyond the allocation; retain the original heap-allocation regression tests for chunk boundaries, prefixes, nonzero depths and trailing bytes. Public APIs are unchanged.

Validation on Linux/amd64 with Go 1.27.3:

  • go test -race -tags=intest ./internal/unionstore/art -count=1 passed.
  • git diff --check HEAD^ HEAD passed.

The branch and dependencies remain based on tidb-8.5; this does not pull the master dependency graph into the release branch.

Summary by CodeRabbit

  • Bug Fixes
    • Improved key-prefix comparisons for empty keys, unaligned offsets, and inputs that match or differ across chunk boundaries. This helps ensure comparisons return the correct matching prefix length.

Return before advancing pointers past the allocation after the final matching chunk. Add heap-allocated regression coverage for chunk boundaries, prefixes, depths, and tails.

Fixes tikv#2116

Signed-off-by: zyguan <zhongyangguan@gmail.com>
(cherry picked from commit ac35602)
Signed-off-by: YangKeao <yangkeao@chunibyo.icu>
@ti-chi-bot ti-chi-bot Bot added dco-signoff: yes Indicates the PR's author has signed the dco. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 27, 2026
@coderabbitai

coderabbitai Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 3296e360-4640-4429-acb3-af8f385cf520
Commits

Reviewing files that changed from the base of the PR and between 8ddbad4 and d406eac.

Files selected for processing: 2
  • internal/unionstore/art/art_node.go
  • internal/unionstore/art/art_node_test.go

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.

Walkthrough

The chunk-based longest common prefix comparison now returns when it consumes the final eight-byte chunk, avoiding a pointer advance beyond the compared region. New tests cover heap-allocated keys across matching and mismatching chunks and tails.

Changes: ART longest common prefix boundary

Layer / File(s) Summary
Chunk comparison and boundary tests
internal/unionstore/art/art_node.go, internal/unionstore/art/art_node_test.go
The chunk comparison returns the matched length when the final eight-byte chunk is consumed. Table-driven tests cover empty keys, exhausted and nonzero depths, aligned and unaligned lengths, matching and mismatching data, and both argument orders.

Priority: Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Possibly related PRs

  • tikv/client-go#2119: This PR applies the same fix and adds the same heap-allocation regression test on the master branch.

Suggested reviewers: you06, ekexium

Merge Risk: Minimal · up to d406e

No merge-blocking risk was identified.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surv… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly identifies the affected component and the main fix: preventing a checkptr panic after the final prefix chunk. It is concise and specific.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 2 functions across 2 files.

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ti-chi-bot ti-chi-bot Bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Oct 27, 2026
@ekexium
ekexium self-requested a review October 27, 2026 09:43
@ti-chi-bot ti-chi-bot Bot added the lgtm label Oct 27, 2026
@ti-chi-bot

ti-chi-bot Bot commented Oct 27, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfzjywxk, ekexium

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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 ti-chi-bot Bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Oct 27, 2026
@ti-chi-bot

ti-chi-bot Bot commented Oct 27, 2026

Copy link
Copy Markdown

[LGTM Timeline notifier]

Timeline:

  • 2026-10-27 09:42:27.493624627 +0000 UTC m=+454484.985336885: ☑️ agreed by cfzjywxk.
  • 2026-10-27 10:43:53.046553745 +0000 UTC m=+458170.538266054: ☑️ agreed by ekexium.

@ti-chi-bot
ti-chi-bot Bot merged commit 348a939 into tikv:tidb-8.5 Oct 27, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved dco-signoff: yes Indicates the PR's author has signed the dco. lgtm size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants