fix: false duplicate entry error on concurrent INSERT with NULL uniqu…#24046
fix: false duplicate entry error on concurrent INSERT with NULL uniqu…#24046jiangxinmeng1 wants to merge 8 commits intomatrixorigin:mainfrom
Conversation
…e keys (matrixorigin#24030) Concurrent multi-row INSERT into tables with UNIQUE KEY + AUTO_INCREMENT falsely reports 'Duplicate entry '' for key' when ~50% of unique key values are NULL. ~30-40% of concurrent INSERTs fail. Root causes and fixes: 1. fuzzyCheck treats NULL as empty string (fuzzyCheck.go) - firstlyCheck() and genCollsionKeys() did not skip NULL rows - Multiple NULLs in unique key column were detected as duplicate empty strings - Fix: skip NULL rows in both functions 2. PKPersistedBetween.InplaceSort corrupts bitmap alignment (txn_table.go) - InplaceSort reorders Varlena data entries but does NOT reorder the null bitmap - After sorting, NULL entries (zero Varlena) move to array start while bitmap still marks original positions, causing bitmap/data misalignment - Fix: Clone vector before InplaceSort in PKPersistedBetween 3. NULL Varlena slots contain garbage from pSpool buffer reuse (txn_table.go) - GetUnionAllFunction skips writing to NULL Varlena slots during vector copy - Reused cache buffers may contain stale data from different column types - EncodePrimaryKeyVector iterates all rows without checking nulls, reading garbage Varlena with svlen>23 but nil area causes slice bounds panic - Fix: cloneNonNullKeys() filters out NULL rows before passing to primaryKeysMayBeChanged Additional defensive fixes: - bitmap.go: add RecalculateCount() to fix count/data inconsistency - nulls.go: Contains() removes EmptyByFlag shortcut; Range() snapshots via Clone() - vector.go: TryExpand in Copy/UnionBatch/AppendFixedList ensures bitmap length - value_scan.go: prepared flag prevents duplicate evalRowsetData; reset in Reset() - scope.go: TP queries use sequential MergeRun execution - stats.go: disable right join optimization for DEDUP joins - txn.go: checkPKDup skips NULL for varchar/array types - preinsert.go: type handling adjustment in constructColBuf Fixes matrixorigin#24030 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jiangxinmeng1
requested review from
XuPeng-SH,
aunjgr,
gouhongshen and
ouyuanning
as code owners
Review Summary by QodoFix false duplicate entry error on concurrent INSERT with NULL unique keys
WalkthroughsDescription• Fix false duplicate entry errors on concurrent INSERT with NULL unique keys • Prevent NULL bitmap corruption from concurrent vector modifications • Skip NULL values in duplicate detection and primary key checks • Ensure bitmap length consistency across vector operations • Disable right join optimization for DEDUP joins to preserve NULL handling Diagramflowchart LR
A["Concurrent INSERT<br/>with NULL unique keys"] -->|fuzzyCheck| B["Skip NULL rows<br/>in duplicate detection"]
A -->|Vector operations| C["Ensure bitmap<br/>length consistency"]
A -->|PKPersistedBetween| D["Clone vector<br/>before sorting"]
A -->|Value scan| E["Prevent duplicate<br/>evalRowsetData calls"]
A -->|DEDUP joins| F["Disable right join<br/>optimization"]
B --> G["Fix: No false<br/>duplicates"]
C --> G
D --> G
E --> G
F --> G
File Changes1. pkg/common/bitmap/bitmap.go
|
Code Review by Qodo
1. Bitmap Contains race panic
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| // Contains returns true if the integer is contained in the Nulls | ||
| func (nsp *Nulls) Contains(row uint64) bool { | ||
| return nsp != nil && !nsp.np.EmptyByFlag() && nsp.np.Contains(row) | ||
| return nsp != nil && nsp.np.Contains(row) |
There was a problem hiding this comment.
1. Bitmap contains race panic 🐞 Bug ☼ Reliability
Nulls.Contains now always calls bitmap.Bitmap.Contains, and some union paths rely on IsEmpty() instead of EmptyByFlag(), increasing read access to bitmaps during concurrent expansion. Since Bitmap.TryExpandWithSize sets len before resizing data, a concurrent Contains() can index past data and panic.
Agent Prompt
### Issue description
`Bitmap.TryExpandWithSize` can temporarily expose `len` that is larger than `data` length. With the PR’s increased reliance on `Contains()` in concurrent contexts, this can crash with an index-out-of-range panic.
### Issue Context
This PR is explicitly addressing concurrent bitmap corruption; hardening the core bitmap read path avoids turning “wrong null detection” into process crashes.
### Fix Focus Areas
- Add a defensive bound check in `Bitmap.Contains` (and any other direct `n.data[idx]` reads) to return false if `idx >= uint64(len(n.data))`.
- Optionally reorder `TryExpandWithSize` to grow `data` before publishing `len`.
- pkg/common/bitmap/bitmap.go[231-238]
- pkg/common/bitmap/bitmap.go[348-363]
- pkg/container/nulls/nulls.go[136-139]
- pkg/container/vector/vector.go[1340-1364]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| } | ||
|
|
||
| b.np.InitWithSize(int64(end + 1 - bias)) | ||
|
|
||
| // Take a snapshot of the source bitmap to prevent reading inconsistent | ||
| // state when the source is being concurrently modified by a parallel | ||
| // Prepare call on the same operator chain. | ||
| snap := nsp.Clone() | ||
| for ; start < end; start++ { | ||
| if nsp.np.Contains(start) { | ||
| if snap.np.Contains(start) { | ||
| b.np.Add(start - bias) | ||
| } | ||
| } |
There was a problem hiding this comment.
2. Range skips corrupted nulls 🐞 Bug ≡ Correctness
nulls.Range still returns early based on EmptyByFlag() (which is count==0), so if count is corrupted to 0 while data has set bits, Range will drop NULLs entirely. This can make Vector.Window/CloneWindowTo silently lose NULL markers and produce incorrect results.
Agent Prompt
### Issue description
`nulls.Range` still gates on `EmptyByFlag()` which depends on `count`. In the exact corruption scenario this PR is targeting (stale `count`), `Range` can incorrectly return early and drop NULLs in windowed/sliced vectors.
### Issue Context
The PR adds a snapshot clone inside `Range`, but the early-return happens before the snapshot and can mask real nulls.
### Fix Focus Areas
- Avoid using `EmptyByFlag()` as the early-exit condition.
- Consider: clone first, then call `snap.np.RecalculateCount()` and check `snap.np.IsEmpty()` before iterating.
- pkg/container/nulls/nulls.go[223-239]
- pkg/common/bitmap/bitmap.go[171-178]
- pkg/common/bitmap/bitmap.go[193-197]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
scope.go explicitly sets PreScopes elements to nil to prevent double-free during MergeRun errors. However, many functions that recursively traverse PreScopes did not check for nil, causing panics during bootstrap (e.g. genReceiverMap in debugTools.go). Add nil guards at function entry for all affected functions: - debugTools.go: genReceiverMap, showSingleScope - analyze_module.go: ConvertScopeToPhyScope, UpdatePreparePhyScope, explainSingleScope - compile.go: UpdateScopeTxnOffset - remoterun.go: generatePipeline, fillInstructionsForPipeline - remoterunClient.go: checkPipelineStandaloneExecutableAtRemote (both BFS loops, plus Proc nil check) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
In MergeRun's TP path, non-connector scopes are run sequentially. After execution, perform the full cleanup sequence matching the normal Compile.FreeOperator → Compile.clear order: 1. FreeOperator(c) - calls op.Free() to release expression executors (e.g. FixedVectorExpressionExecutor) 2. release() - calls op.Release() and reuse.Free[Scope] to return operator and scope objects to the reuse pool 3. Set PreScopes[i] = nil - prevents double-free when later cleanup walks the tree (nil checks exist throughout) The previous approach only called release() without FreeOperator(), causing 'missing free' panics for expression executors. Removing release() entirely caused 'missing free' panics for Scope objects. Also use concurrentPreScopes counter instead of len(s.PreScopes) to avoid deadlock when receiving from the preScopeResultReceiveChan. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Review Summary
The NULL-skipping fixes (fuzzyCheck, checkPKDup, cloneNonNullKeys, PKPersistedBetween clone-before-sort) are correct and address real bugs. However, several other changes are treating symptoms of a deeper concurrency issue rather than fixing the root cause, and introduce performance regressions.
See inline comments for details on each concern.
| @@ -135,7 +135,7 @@ func TryExpand(nsp *Nulls, size int) { | |||
|
|
|||
| // Contains returns true if the integer is contained in the Nulls | |||
| func (nsp *Nulls) Contains(row uint64) bool { | |||
There was a problem hiding this comment.
🔴 Removing EmptyByFlag() short-circuit masks the root cause
EmptyByFlag() checks n.count == 0 — a fast O(1) short-circuit on one of the hottest paths in the engine. Removing it means you've discovered that count and data are inconsistent (count=0 but bits are set). The correct fix is to find what's corrupting the count/data invariant, not to stop trusting count.
Also note the contradiction with other changes in this PR: the EmptyByFlag()→IsEmpty()replacements in vector.go still rely oncount being accurate (IsEmpty()returnsn.count == 0`). If count is untrustworthy here, it's untrustworthy there too.
This will cause a measurable performance regression on every Contains() call across the entire engine.
| // Take a snapshot of the source bitmap to prevent reading inconsistent | ||
| // state when the source is being concurrently modified by a parallel | ||
| // Prepare call on the same operator chain. | ||
| snap := nsp.Clone() |
There was a problem hiding this comment.
🔴 Clone() on every Range() call — severe hot-path performance regression
Range() is called frequently during null propagation. Cloning the entire bitmap on every call is very expensive. If the source bitmap is being concurrently modified by a parallel Prepare call, the correct fix is to add synchronization at the caller level (or ensure the operator chain is not shared), not to defensively clone on every Range() invocation.
This also only provides a snapshot — if the underlying concurrency bug remains, the snapshot may still be taken from an inconsistent state (torn read).
| func (n *Bitmap) RecalculateCount() { | ||
| n.count = 0 | ||
| for _, w := range n.data { | ||
| n.count += int64(bits.OnesCount64(w)) |
There was a problem hiding this comment.
🟡 RecalculateCount() treats the symptom, not the cause
This function exists to fix count/data inconsistency after "concurrent bitmap corruption" (per the comment). But if concurrent access can corrupt the count, it can also corrupt the data bits themselves. Recalculating count from (possibly corrupted) data only makes the count consistent with whatever garbage is in data.
The real fix should prevent the concurrent corruption in the first place. Consider this a temporary band-aid and track the root cause separately.
| @@ -349,7 +358,13 @@ func (n *Bitmap) TryExpandWithSize(size int) { | |||
| return | |||
There was a problem hiding this comment.
✅ TryExpandWithSize zeroing — correct fix
This is a legitimate bug fix. When len(n.data) < newCap <= cap(n.data), the n.data = n.data[:newCap] re-slice exposes backing-array slots that may contain stale data from a previous use. Zeroing them is correct.
Note: the newCap > cap(n.data) branch (line 348) allocates via make() which zero-initializes in Go, so it doesn't need this treatment.
| // release (op.Release + reuse.Free[Scope]). This mirrors the normal cleanup | ||
| // order in Compile.FreeOperator → Compile.clear. The entry is then nil'd so | ||
| // that later walks of PreScopes skip it (nil checks exist throughout). | ||
| if c.IsTpQuery() { |
There was a problem hiding this comment.
🟡 Removing !c.hasMergeOp condition — overly broad, fragile pattern
The old condition c.IsTpQuery() && !c.hasMergeOp limited sequential execution to simple TP queries. Removing !c.hasMergeOp forces ALL TP queries (including INSERT with UNIQUE KEY, which sets hasMergeOp=true) into sequential pre-scope execution. This is a performance regression for TP queries with multiple independent PreScopes.
The FreeOperator → release → nil pattern is also fragile — it requires nil guards everywhere PreScopes are traversed (20+ new nil checks across compile.go, compile2.go, debugTools.go, remoterun.go, remoterunClient.go, types.go, analyze_module.go). Missing a single nil check will cause a panic.
A more targeted fix would be to identify which specific scope is being shared unsafely and fix the sharing, rather than serializing everything.
| // correctly even when the last appended rows are non-null. | ||
| if vec.nsp.Count() > 0 { | ||
| nulls.TryExpand(&vec.nsp, vec.length) | ||
| } |
There was a problem hiding this comment.
🟡 TryExpand on every non-null appendOneFixed — hot path cost
if vec.nsp.Count() > 0 {
nulls.TryExpand(&vec.nsp, vec.length)
}This runs on every non-null appendOneFixed call when the vector has any previous nulls. For a batch of 8192 rows where only row 0 is null, this calls TryExpand 8191 times. The expand is a no-op when already big enough, but the function call overhead on this hot path is unnecessary.
Consider doing this once after the batch is fully built, or tracking whether expansion is needed with a flag.
| for _, k := range pkey { | ||
| for i, k := range pkey { | ||
| // NULL != NULL in SQL standard, skip NULL values from duplicate check | ||
| if toCheck.GetNulls().Contains(uint64(i)) { |
There was a problem hiding this comment.
✅ Correct — NULL rows must be skipped in duplicate detection
SQL standard: NULL != NULL. Multiple NULL values in a UNIQUE KEY column are not duplicates. This fix is correct and addresses one of the core root causes.
| sortedKeys, err := keys.Dup(tbl.proc.Load().Mp()) | ||
| if err != nil { | ||
| return false, err | ||
| } |
There was a problem hiding this comment.
✅ Correct — Clone before InplaceSort
InplaceSort() reorders the data slice via sort.Slice() but does NOT reorder the null bitmap (verified in source). Cloning before sort prevents bitmap/data misalignment. This is a legitimate bug fix.
Note: a more fundamental fix would be to make InplaceSort() itself handle nulls (either sort the bitmap alongside data, or filter nulls first), since this footgun will bite other callers too.
| } | ||
| return cloned, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
✅ Correct — cloneNonNullKeys prevents garbage Varlena panic
Filtering out NULL rows before primaryKeysMayBeChanged prevents EncodePrimaryKeyVector from reading garbage Varlena data in NULL slots. This is a correct defensive fix.
Minor: when !keysVector.HasNull(), this does a full Dup(). If memory is a concern, the no-null path could return the original vector with a flag indicating the caller should NOT free it (to avoid the unconditional defer clonedKeys.Free(mp)).
| start, count int) (bool, string) { | ||
| for _, v := range vals[start : start+count] { | ||
| nsp := pk.GetNulls() | ||
| for i := start; i < start+count; i++ { |
There was a problem hiding this comment.
✅ Correct — skip NULL in checkPKDupGeneric
NULL values should not participate in primary key duplicate detection. This fix is correct.
3 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #24030
What this PR does / why we need it:
Concurrent multi-row INSERT into tables with UNIQUE KEY + AUTO_INCREMENT falsely reports 'Duplicate entry '' for key' when ~50% of unique key values are NULL. ~30-40% of concurrent INSERTs fail.
Root causes and fixes:
fuzzyCheck treats NULL as empty string (fuzzyCheck.go)
PKPersistedBetween.InplaceSort corrupts bitmap alignment (txn_table.go)
NULL Varlena slots contain garbage from pSpool buffer reuse (txn_table.go)
Additional defensive fixes:
Fixes #24030