MDEV-40448 HEAP unique hash duplicate rejection re-hashes the full key value#5409
MDEV-40448 HEAP unique hash duplicate rejection re-hashes the full key value#5409arcivanov wants to merge 1 commit into
Conversation
…y value A rejected duplicate insert into a HEAP unique hash index paid the full-value key hash twice: `hp_write_key()` hashed the key to insert it (the documented contract was "the record was still added and the caller must call hp_delete_key for it"), and `hp_delete_key()` then hashed the same record **again** just to locate the bucket to unlink from. For long key values (BLOB keys, MDEV-38975) the second scan dominates duplicate-heavy workloads: `SELECT DISTINCT` over 20-50KB blob values (~80% duplicates) regressed 25-32% vs Aria tmp tables at low concurrency, with `my_uca_hash_sort_utf8mb4` doing 1.83x the hashing work for the identical query. The hash caching added by `de1765fb64d` (`HASH_INFO::hash_of_key`) already made all chain surgery hash-free; the two full-value hashes per rejected row were the entire cost. Fix: 1. **Probe before insert** (`hp_write_key()`): compute the hash once at the top; for `HA_NOSAME` keys without NULL key parts walk the key's chain under the pre-insert mask comparing cached `HASH_INFO::hash_of_key` values, comparing full key values only on a hash match. On a duplicate return `HA_ERR_FOUND_DUPP_KEY` with nothing modified; otherwise proceed with the linear-hash split and insertion reusing the already-computed hash. Records with equal full hashes share a bucket under any mask, so probing the pre-insert chain finds any duplicate. A successful insert costs exactly one full-value hash, as before; a rejected duplicate drops from two full hashes + insert + undo delete to one hash + compare, the same as a lookup. 2. **Error-path contract change**: `hp_write_key()` no longer inserts the key on a duplicate, so `heap_write()` rolls back only the preceding keys (unconditional `keydef--`, as for BTREE/ENOMEM), and `heap_update()`'s duplicate path now re-inserts the old key for the failing keydef for both algorithms (previously BTREE-only) before rolling back earlier keys. 3. **Delete-side hash reuse** (`hp_delete_key()`): when the row being deleted was positioned via the same hash index (`flag` set and `info->current_hash_ptr->ptr_to_rec == recpos`), take the hash from the index entry instead of re-scanning the key value; a `DBUG_ASSERT` cross-checks the cached hash in debug builds. This removes the remaining full-value hash from `DELETE`/`UPDATE` of rows located through the index (`heap_rkey()` already hashed the key). 4. **`heap_rfirst()`/`heap_rlast()`**: clear `info->current_hash_ptr` when rejecting a hash index with `HA_ERR_WRONG_COMMAND`. Both functions retarget `info->lastinx` before the algorithm check, so a stale `current_hash_ptr` from a previous search on a different key could otherwise satisfy the new cached-hash guard in `hp_delete_key()` and send the bucket lookup to the wrong chain (reachable through the heap API only; the SQL layer never issues ordered reads on hash indexes). New unit test `hp_test_write_dup-t` (105 assertions) wraps the key charset's `hash_sort` collation handler in a counting shim, asserting the exact number of full-value hashes for every operation: 1 per insert attempt (successful or rejected), 1 total for index-read + delete, 1 for an index-positioned re-key. Behavioral coverage: single- and multi-key rollback for INSERT and UPDATE duplicates, NULL key parts, non-unique keys, delete after rejected `heap_rfirst()`/`heap_rlast()`, and a 500-row duplicate-heavy stress across linear-hash splits (exactly 500 hashes; was 827 before the fix). Benchmarked (16 threads, 120s runs, 8c/16t AMD 7840HS; `base` = preview-13.1 without MDEV-38975 = Aria tmp tables, `new` = unfixed, `fix` = this patch): | test (QPS) | base | new | fix | |---------------|------|--------------|---------------------| | `blob_case_c` | 4.65 | 3.36 (-28%) | 5.61 (+21% vs base) | | `blob_mixed` | 5.87 | 4.13 (-30%) | 6.92 (+18% vs base) | `hp_delete_key()` (38% inclusive before) is absent from the fixed profile; the low-concurrency regression becomes a win on top of the existing 2x+ high-concurrency wins.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request optimizes duplicate key handling in the HEAP storage engine by probing for duplicates before modifying the hash index, which avoids expensive redundant hashing (especially for blob keys) and simplifies rollback logic. It also optimizes deletion by reusing cached hashes, clears stale hash pointers on rejected ordered reads, and introduces a comprehensive unit test suite. The review feedback highlights a critical bug in the new duplicate probe logic in hp_write_key where an empty bucket could trigger a segmentation fault when search_pos is 0, and suggests ensuring pos->ptr_to_rec is non-NULL before evaluating the mask.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
gkodinov
added
the
External Contribution
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
Thank you for your contribution! This is a preliminary review.
The only remark I have is that, as a new "feature", this should be based on the main branch. Can you please rebase?
Otherwise, I see nothing wrong in the diff.
|
Forgot to ask: are these somehow related to your previous work? If they are and need to go where the original work is pushed, then please confirm and ignore my rebase requests. |
|
These are BLOB in HEAP (MDEV-38975) performance fixes/regressions that were discovered during performance testing. The feature is in preview so the PR has to go into preview. |
gkodinov
left a comment
gkodinov
left a comment
There was a problem hiding this comment.
LGTM. Please stand by for the final review.
3 participants
JIRA: MDEV-40448
Problem
A rejected duplicate insert into a HEAP unique hash index paid the full-value key hash twice:
hp_write_key()hashed the key to insert it (the documented contract was "the record was still added and the caller must call hp_delete_key for it"), andhp_delete_key()then hashed the same record again just to locate the bucket to unlink from. For long key values (BLOB keys, MDEV-38975) the second scan dominates duplicate-heavy workloads:SELECT DISTINCTover 20-50KB blob values (~80% duplicates) regressed 25-32% vs Aria tmp tables at low concurrency, withmy_uca_hash_sort_utf8mb4doing 1.83x the hashing work for the identical query.The hash caching added by
de1765fb64d(HASH_INFO::hash_of_key) already made all chain surgery hash-free; the two full-value hashes per rejected row were the entire cost.Fix
hp_write_key()): compute the hash once at the top; forHA_NOSAMEkeys without NULL key parts walk the key's chain under the pre-insert mask comparing cachedHASH_INFO::hash_of_keyvalues, comparing full key values only on a hash match. On a duplicate returnHA_ERR_FOUND_DUPP_KEYwith nothing modified; otherwise proceed with the linear-hash split and insertion reusing the already-computed hash. Records with equal full hashes share a bucket under any mask, so probing the pre-insert chain finds any duplicate. A successful insert costs exactly one full-value hash, as before; a rejected duplicate drops from two full hashes + insert + undo delete to one hash + compare, the same as a lookup.hp_write_key()no longer inserts the key on a duplicate, soheap_write()rolls back only the preceding keys (unconditionalkeydef--, as for BTREE/ENOMEM), andheap_update()'s duplicate path now re-inserts the old key for the failing keydef for both algorithms (previously BTREE-only) before rolling back earlier keys.hp_delete_key()): when the row being deleted was positioned via the same hash index (flagset andinfo->current_hash_ptr->ptr_to_rec == recpos), take the hash from the index entry instead of re-scanning the key value; aDBUG_ASSERTcross-checks the cached hash in debug builds. This removes the remaining full-value hash fromDELETE/UPDATEof rows located through the index (heap_rkey()already hashed the key).heap_rfirst()/heap_rlast(): clearinfo->current_hash_ptrwhen rejecting a hash index withHA_ERR_WRONG_COMMAND. Both functions retargetinfo->lastinxbefore the algorithm check, so a stalecurrent_hash_ptrfrom a previous search on a different key could otherwise satisfy the new cached-hash guard inhp_delete_key()and send the bucket lookup to the wrong chain (reachable through the heap API only; the SQL layer never issues ordered reads on hash indexes).Testing
New unit test
hp_test_write_dup-t(105 assertions) wraps the key charset'shash_sortcollation handler in a counting shim, asserting the exact number of full-value hashes for every operation: 1 per insert attempt (successful or rejected), 1 total for index-read + delete, 1 for an index-positioned re-key. Behavioral coverage: single- and multi-key rollback for INSERT and UPDATE duplicates, NULL key parts, non-unique keys, delete after rejectedheap_rfirst()/heap_rlast(), and a 500-row duplicate-heavy stress across linear-hash splits (exactly 500 hashes; was 827 before the fix). All counting assertions were verified to fail before the fix and pass after, with the production code otherwise unchanged.Full runs green on RelWithDebInfo and Debug: heap unit tests,
hp_test1/hp_test2, MTRsuite/heap(27/27), and main-suite distinct/group_by/count_distinct/derived/type_blob/temp_table/replace/insert_update/update/show_check/information_schema (14/14). TheDBUG_ASSERTcross-check never fired.Benchmark
16 threads, 120s recorded runs, 8c/16t AMD 7840HS;
base= preview-13.1 without MDEV-38975 (Aria tmp tables),new= unfixed,fix= this patch:blob_case_cblob_mixedhp_delete_key()(38% inclusive before) is absent from the fixed profile; the low-concurrency regression becomes a win on top of the existing 2x+ high-concurrency wins.