fix: handle MaxUint64 boundary case in AUTO_INCREMENT to avoid returning 0 after restart#67244
fix: handle MaxUint64 boundary case in AUTO_INCREMENT to avoid returning 0 after restart#67244tiancaiamao wants to merge 2 commits intomasterfrom
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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
the
do-not-merge/release-note-label-needed
|
Review Complete Findings: 0 issues ℹ️ Learn more details on Pantheon AI. |
📝 WalkthroughWalkthroughThe changes add boundary condition handling for unsigned AUTO_INCREMENT values in TiDB's auto-ID allocation system. When allocating IDs for unsigned columns that reach the maximum value ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.3)Command failed Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ti-chi-bot
bot
added
the
size/XXL
|
Hi @tiancaiamao. 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
component/dumpling
|
/ok-to-test |
…ing 0 after restart Fixes #63455 Problem: - When UPDATE id = -1 triggers rebase to MaxUint64 (18446744073709551615) - After TiDB restart, allocator reads MaxUint64 from KV as -1 (int64) - NextGlobalAutoID() incorrectly returns maxv + 1 = -1 + 1 = 0 - Subsequent INSERT operations fail with duplicate key errors Solution: - Add MaxUint64 boundary check in NextGlobalAutoID() - Return ErrAutoincReadFailed instead of 0 when maxv is -1 for unsigned types - Prevents allocator from returning invalid ID = 0 Test Case: - Added TestMaxUint64Boundary to verify fix - Tests scenario where lastAllocated = -1 (MaxUint64 as int64) - Ensures error is returned instead of 0
tiancaiamao
force-pushed
the
fix-autoid-maxuint64
branch
from
9f9d7ca to
5344be5
Compare
ti-chi-bot
bot
removed
the
size/XXL
|
[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
the
size/L
|
@tiancaiamao: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
@tiancaiamao: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
[FORMAT CHECKER NOTIFICATION] Notice: To remove the 📖 For more info, you can check the "Contribute Code" section in the development guide. Notice: To remove the For example:
📖 For more info, you can check the "Contribute Code" section in the development guide. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pkg/meta/autoid/autoid_maxuint64_test.go (2)
52-64: Clarify comment for signed case.The comment "For signed type, maxv=-1 + 1 = 0 is valid" could be misleading. The key point is that for signed AUTO_INCREMENT, the value
-1stored in KV represents an actual allocation of-1(not a boundary case), whereas for unsigned types, the int64 value-1representsMaxUint64(the boundary). Consider refining the comment to make this distinction clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/meta/autoid/autoid_maxuint64_test.go` around lines 52 - 64, Update the test comment in the t.Run "signed_maxv_minus_one_returns_zero" to clarify that for signed AUTO_INCREMENT the stored KV value -1 represents an actual allocation of -1 (so -1 + 1 = 0 is valid), whereas for unsigned types an int64 value of -1 represents MaxUint64 (a boundary case) and should be handled differently; reference the variables maxv, isUnsigned, shouldReturnError and nextID in the comment so readers understand the test verifies only the signed-path behavior.
34-95: Test validates logic but doesn't exercise actual code path.The test re-implements the boundary check logic (
isUnsigned && maxv == -1) rather than callingsinglePointAlloc.NextGlobalAutoID(). This means:
- It won't catch regressions if the actual implementation changes
- It doesn't verify that
ErrAutoincReadFailedis returned with the correct messageConsider adding an integration test that exercises the actual
NextGlobalAutoID()method, even if it requires mocking the gRPC client to returnmaxv = -1. Alternatively, extract the boundary check into a testable helper function.♻️ Example: Extract boundary check for direct testing
In
autoid_service.go:// checkUnsignedBoundary returns an error if maxv represents MaxUint64 for unsigned types. func checkUnsignedBoundary(isUnsigned bool, maxv int64) error { if isUnsigned && maxv == -1 { return ErrAutoincReadFailed.GenWithStack("AUTO_INCREMENT has reached the maximum value for unsigned type") } return nil }Then in
NextGlobalAutoID():if err := checkUnsignedBoundary(sp.isUnsigned, maxv); err != nil { return 0, err }And test the helper directly:
func TestCheckUnsignedBoundary(t *testing.T) { err := checkUnsignedBoundary(true, -1) require.ErrorIs(t, err, ErrAutoincReadFailed) err = checkUnsignedBoundary(false, -1) require.NoError(t, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/meta/autoid/autoid_maxuint64_test.go` around lines 34 - 95, The test re-implements the boundary logic instead of exercising the real code path; update tests to either (A) call NextGlobalAutoID and assert it returns ErrAutoincReadFailed (with the expected message) by mocking the gRPC client to return maxv == -1 for an unsigned column, or (B) extract the boundary check into a helper (e.g., checkUnsignedBoundary(isUnsigned bool, maxv int64)) used by NextGlobalAutoID and add a unit test that calls checkUnsignedBoundary(true, -1) expecting ErrAutoincReadFailed and checkUnsignedBoundary(false, -1) expecting no error; ensure NextGlobalAutoID continues to call that helper.pkg/meta/autoid/BUILD.bazel (1)
51-51:embedis unnecessary — the test file only uses public APIs (requirefrom testify and arithmetic operations onint64/uint64). It doesn't access any unexported symbols.Consider either:
- Removing
embed = [":autoid"]from line 51 and keeping:autoidindeps, or- Converting the test to use
package autoid_test(black-box testing) and removingembedentirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/meta/autoid/BUILD.bazel` at line 51, The BUILD rule currently uses embed = [":autoid"] even though the test only exercises public APIs; remove the unnecessary embed attribute (delete the line embed = [":autoid"]) and keep ":autoid" in deps, or alternatively change the test's package declaration from package autoid to package autoid_test (black‑box test) and then remove the embed line; update either the BUILD rule (remove embed) or the test package (to autoid_test) so embed is no longer required.
🤖 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/meta/autoid/autoid_maxuint64_test.go`:
- Line 1: Update the license header in pkg/meta/autoid/autoid_maxuint64_test.go
by changing the copyright year from 2025 to 2026; locate the top-of-file comment
(the file header) and replace "2025 PingCAP, Inc." with "2026 PingCAP, Inc." so
the file reflects the current year.
In `@pkg/meta/autoid/autoid_service.go`:
- Around line 344-350: Add the same MaxUint64 boundary guard you added in
singlePointAlloc.NextGlobalAutoID to the other two implementations: in
allocator.NextGlobalAutoID() (autoid.go) and in
inMemoryAllocator.NextGlobalAutoID() (memid.go). Specifically, detect the case
where the input value equals -1 (MaxUint64 when cast to int64) for unsigned
allocators (use the same isUnsigned check or equivalent context) and return a
proper error (ErrAutoincReadFailed.GenWithStack with a message like
"AUTO_INCREMENT has reached the maximum value for unsigned type") instead of
returning 0; otherwise continue to return the incremented value as before.
---
Nitpick comments:
In `@pkg/meta/autoid/autoid_maxuint64_test.go`:
- Around line 52-64: Update the test comment in the t.Run
"signed_maxv_minus_one_returns_zero" to clarify that for signed AUTO_INCREMENT
the stored KV value -1 represents an actual allocation of -1 (so -1 + 1 = 0 is
valid), whereas for unsigned types an int64 value of -1 represents MaxUint64 (a
boundary case) and should be handled differently; reference the variables maxv,
isUnsigned, shouldReturnError and nextID in the comment so readers understand
the test verifies only the signed-path behavior.
- Around line 34-95: The test re-implements the boundary logic instead of
exercising the real code path; update tests to either (A) call NextGlobalAutoID
and assert it returns ErrAutoincReadFailed (with the expected message) by
mocking the gRPC client to return maxv == -1 for an unsigned column, or (B)
extract the boundary check into a helper (e.g., checkUnsignedBoundary(isUnsigned
bool, maxv int64)) used by NextGlobalAutoID and add a unit test that calls
checkUnsignedBoundary(true, -1) expecting ErrAutoincReadFailed and
checkUnsignedBoundary(false, -1) expecting no error; ensure NextGlobalAutoID
continues to call that helper.
In `@pkg/meta/autoid/BUILD.bazel`:
- Line 51: The BUILD rule currently uses embed = [":autoid"] even though the
test only exercises public APIs; remove the unnecessary embed attribute (delete
the line embed = [":autoid"]) and keep ":autoid" in deps, or alternatively
change the test's package declaration from package autoid to package autoid_test
(black‑box test) and then remove the embed line; update either the BUILD rule
(remove embed) or the test package (to autoid_test) so embed is no longer
required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f7a4ccf3-99f4-4afb-931a-a0989715e223
📒 Files selected for processing (3)
pkg/meta/autoid/BUILD.bazelpkg/meta/autoid/autoid_maxuint64_test.gopkg/meta/autoid/autoid_service.go
| @@ -0,0 +1,96 @@ | |||
| // Copyright 2025 PingCAP, Inc. | |||
There was a problem hiding this comment.
Update copyright year to 2026.
The current year is 2026, but the license header shows 2025. As per coding guidelines, update the year when creating new source files.
📝 Proposed fix
-// Copyright 2025 PingCAP, Inc.
+// Copyright 2026 PingCAP, Inc.📝 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.
| // Copyright 2025 PingCAP, Inc. | |
| // Copyright 2026 PingCAP, Inc. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/meta/autoid/autoid_maxuint64_test.go` at line 1, Update the license
header in pkg/meta/autoid/autoid_maxuint64_test.go by changing the copyright
year from 2025 to 2026; locate the top-of-file comment (the file header) and
replace "2025 PingCAP, Inc." with "2026 PingCAP, Inc." so the file reflects the
current year.
| // Fix: handle MaxUint64 boundary case | ||
| // When maxv is MaxUint64 (-1 in int64), adding 1 results in 0. | ||
| // We should return an error instead of allowing 0 for unsigned types. | ||
| if sp.isUnsigned && maxv == -1 { | ||
| return 0, ErrAutoincReadFailed.GenWithStack("AUTO_INCREMENT has reached the maximum value for unsigned type") | ||
| } | ||
| return maxv + 1, err |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check allocator.NextGlobalAutoID() implementation in autoid.go
rg -n -A 20 'func \(alloc \*allocator\) NextGlobalAutoID' pkg/meta/autoid/autoid.goRepository: pingcap/tidb
Length of output: 899
🏁 Script executed:
#!/bin/bash
# Check for all autoid allocator types and their Next/Get patterns
rg -n "type.*allocator" pkg/meta/autoid/ | head -20Repository: pingcap/tidb
Length of output: 607
🏁 Script executed:
#!/bin/bash
# Search for all return patterns with unsigned conversion in autoid package
rg -n "int64\(uint64" pkg/meta/autoid/Repository: pingcap/tidb
Length of output: 578
🏁 Script executed:
#!/bin/bash
# Check context around autoid_service.go lines 344-350 to see what changed
git -C /tmp/repo log --oneline -20 -- pkg/meta/autoid/autoid_service.go 2>/dev/null || echo "Git not available, checking file directly"
cat -n pkg/meta/autoid/autoid_service.go | sed -n '340,355p'Repository: pingcap/tidb
Length of output: 879
🏁 Script executed:
#!/bin/bash
# Check memid allocator implementation for unsigned handling
rg -n -B 5 "return int64\(uint64\(alloc.base\) \+ 1\)" pkg/meta/autoid/memid.goRepository: pingcap/tidb
Length of output: 287
🏁 Script executed:
#!/bin/bash
# Check if memid allocator has isUnsigned field and where Alloc checks it
rg -n -A 30 "func \(alloc \*memAutoIDAllocator\) Alloc\(" pkg/meta/autoid/memid.go | head -50Repository: pingcap/tidb
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Search for test coverage to understand if this boundary case is tested
rg -n "MaxUint64\|== -1\|0xffffffff" pkg/meta/autoid/ | grep -i testRepository: pingcap/tidb
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Get more context on inMemoryAllocator implementation
cat -n pkg/meta/autoid/memid.go | sed -n '60,90p'Repository: pingcap/tidb
Length of output: 1194
🏁 Script executed:
#!/bin/bash
# Check all NextGlobalAutoID implementations across allocator types
rg -n "func.*NextGlobalAutoID" pkg/meta/autoid/Repository: pingcap/tidb
Length of output: 343
🏁 Script executed:
#!/bin/bash
# Verify if allocator struct and inMemoryAllocator both have isUnsigned field
rg -n "isUnsigned" pkg/meta/autoid/autoid.go pkg/meta/autoid/memid.go | head -20Repository: pingcap/tidb
Length of output: 1467
The fix is correct for singlePointAlloc, but the same vulnerability exists in TWO other allocator implementations.
The boundary guard added to singlePointAlloc.NextGlobalAutoID() (lines 347–348) correctly prevents wrapping to 0 when maxv == -1 for unsigned types. However, two other Allocator interface implementations have the identical vulnerability:
allocator.NextGlobalAutoID()inautoid.go:327:return int64(uint64(autoID) + 1), errinMemoryAllocator.NextGlobalAutoID()inmemid.go:77:return int64(uint64(alloc.base) + 1), nil
Both will return 0 when the input equals -1 (MaxUint64 as int64) for unsigned types, without error handling.
Apply the same boundary guard to both allocator.NextGlobalAutoID() and inMemoryAllocator.NextGlobalAutoID() to fix this inconsistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/meta/autoid/autoid_service.go` around lines 344 - 350, Add the same
MaxUint64 boundary guard you added in singlePointAlloc.NextGlobalAutoID to the
other two implementations: in allocator.NextGlobalAutoID() (autoid.go) and in
inMemoryAllocator.NextGlobalAutoID() (memid.go). Specifically, detect the case
where the input value equals -1 (MaxUint64 when cast to int64) for unsigned
allocators (use the same isUnsigned check or equivalent context) and return a
proper error (ErrAutoincReadFailed.GenWithStack with a message like
"AUTO_INCREMENT has reached the maximum value for unsigned type") instead of
returning 0; otherwise continue to return the incremented value as before.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #67244 +/- ##
================================================
- Coverage 77.7966% 77.2321% -0.5645%
================================================
Files 2022 1941 -81
Lines 554843 541628 -13215
================================================
- Hits 431649 418311 -13338
- Misses 121449 123315 +1866
+ Partials 1745 2 -1743
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
2 participants
Problem
Ref #63455
When
UPDATE id = -1triggers AUTO_INCREMENT rebase to MaxUint64 (18446744073709551615), after TiDB restart, the allocator incorrectly returns 0 as the next ID, causing subsequent INSERT operations to fail with duplicate key errors.Root Cause
endvalue in KV storage, NOT thebasevalueUPDATE id = -1triggersForceRebase(), it rebases toMaxUint64MaxUint64is stored in KV as uint64 value (18446744073709551615)baseandendreset to 0)currentEnd = MaxUint64(read as int64 = -1)base = -1,end = -1NextGlobalAutoID()returns:maxv + 1 = -1 + 1 = 0id = 0→ Duplicate key errorThe bug was in the
NextGlobalAutoID()function inpkg/meta/autoid/autoid_service.goat linereturn maxv + 1, err. Whenmaxv = -1(MaxUint64 as int64),-1 + 1 = 0is invalid for unsigned AUTO_INCREMENT (first valid ID is 1).Solution
Add a MaxUint64 boundary check in
NextGlobalAutoID()to detect whenmaxv == -1for unsigned types and returnErrAutoincReadFailederror instead of returning 0.Changes Made
Modified
pkg/meta/autoid/autoid_service.go:if sp.isUnsigned && maxv == -1 { return 0, ErrAutoincReadFailed.GenWithStack(...) }sp.isUnsigned == true)Added test
pkg/meta/autoid/autoid_maxuint64_test.go:TestMaxUint64Boundarywith 4 test casesTesting
go test -v -run TestMaxUint64Boundary ./pkg/meta/autoid/Verification
Worktree is based on clean
origin/mastercommit: dffbd3eSummary by CodeRabbit
Bug Fixes
Tests