MDEV-40431 heap: move the changed keys back on every heap_update() failure - #5468
Open
arcivanov wants to merge 2 commits into
Open
MDEV-40431 heap: move the changed keys back on every heap_update() failure#5468arcivanov wants to merge 2 commits into
heap_update() failure#5468arcivanov wants to merge 2 commits into
Conversation
…ap_update()` `heap_update()` moves all changed key entries to the new key values **before** writing the new blob chains. When a blob chain write then failed (e.g. with `HA_ERR_RECORD_FILE_FULL`), the rollback restored the record bytes and blob chain pointers, but the `err:` label only undid key changes for `HA_ERR_FOUND_DUPP_KEY` -- historically the only possible failure once the key loop had run. The hash/btree entries were left keyed on the new values while pointing at a record holding the old values, corrupting the index: 1. index lookups by the old key value missed the row 2. `CHECK TABLE` reported the table corrupt 3. on debug builds the heap consistency check in `ha_heap::external_lock()` raised a second error into an already-set diagnostics area, firing a `Diagnostics_area` assertion on the next statement Fix: widen the `err:` recovery to run for `HA_ERR_RECORD_FILE_FULL`, `HA_ERR_OUT_OF_MEM` and `ENOMEM` as well, so a failure raised after the key loop also moves every changed key back to its old value. One recovery path now serves every failure that leaves keys moved to their new values, including any future error source in the key loop itself. The `err:` block assumed the failure happened **inside** the key loop, so that `keydef` addresses the partially processed keydef. A blob-chain write fails after that loop has run to completion, and therefore arrives with `keydef == end`. Reading `info->errkey` and `keydef->algorithm` from there addresses `share->keydef[share->keys]`; as `sizeof(HP_KEYDEF)` (888) far exceeds the key segments and blob descriptors that follow the keydef array, that read runs past the end of the `HP_SHARE` allocation, and the rollback sweep then dereferences a garbage `keydef->seg` in `hp_rec_key_cmp()`. So the recovery distinguishes the two failure sites: with `keydef == end` there is no partly updated key to repair and none to name in `info->errkey`, and the sweep starts at the last keydef instead. The original errno is captured before the recovery and restored after it, so that a rollback `write_key` failure (which `hp_rb_write_key()` reports as `HA_ERR_FOUND_DUPP_KEY` with a stale `errkey`) cannot mask it. The new test `heap.blob_update_key_rollback` exercises hash, BTREE, two changed indexes, an index on an unchanged column (which the rollback must leave untouched), and a partial multi-row UPDATE; each asserts the index stays consistent after the failure via `CHECK TABLE` and index lookups.
…failure `heap_update()` moves every changed key entry to its new key value before it updates the record, so a failure raised afterwards leaves the index describing a record image that is not in the table. The recovery at the `err:` label only ran for an enumerated set of error codes: originally just `HA_ERR_FOUND_DUPP_KEY`, widened by MDEV-40378 to also cover `HA_ERR_RECORD_FILE_FULL`, `HA_ERR_OUT_OF_MEM` and `ENOMEM`. Any error outside that list, such as the `HA_ERR_CRASHED` of `hp_delete_key()`, skipped the recovery entirely: lookups by the real column value miss the row, `CHECK TABLE` reports the table corrupt, and on debug builds the consistency check in `ha_heap::external_lock()` raises an error into an already set diagnostics area, firing a `Diagnostics_area` assertion on a following statement. Interlocked with that, `hp_rb_write_key()` reported **every** rejected `tree_insert()` as `HA_ERR_FOUND_DUPP_KEY`, although `tree_insert()` also returns NULL when the allocation of the tree node fails. That made the `HA_ERR_OUT_OF_MEM` and `ENOMEM` arms of the widened list unreachable on the rb-tree path, and it makes the server act on an allocation failure as if it were a duplicate: `handler::is_fatal_error()` reports `HA_ERR_FOUND_DUPP_KEY` as not fatal to callers asking for `HA_CHECK_DUP_KEY`, and `handler::print_error()` names a key through `info->errkey` that need not be unique at all. Correcting one without the other trades a wrong error for a silently corrupt index, so both change here. 1. `hp_rb_write_key()` tells the two failures apart. `tree_insert()` accounts the node in `rb_tree.allocated` before it allocates it and leaves the counter alone when it rejects a duplicate, so a grown counter identifies the allocation attempt. The accounting for the node that was never allocated is taken back and `HA_ERR_OUT_OF_MEM` is returned. 2. The recovery runs for every failure instead of for a list of error codes, so that no error source can silently skip it. The key loop now tests `delete_key()` and `write_key()` separately, because which of them failed decides what the failing key needs: a failed `delete_key()` leaves the index untouched, a failed `write_key()` needs the old value written back, and a hash duplicate additionally needs the new entry removed, which `hp_write_key()` deliberately leaves in place. The `keydef == end` case of MDEV-40378, where the blob chain writes fail after every changed key was moved, keeps its behaviour. 3. A failing recovery step no longer returns early. The keys are independent of each other, so the ones that can still be moved back are. 4. `info->errkey` is only set for a duplicate key, the single error it describes. The condition Monty asked for in the report, `HA_ERR_FOUND_DUPP_KEY || HA_ERR_RECORD_FILE_FULL || ENOMEM`, landed with MDEV-40378; every error code it names still recovers here. It is generalized rather than extended once more because test 4 below shows an error code outside any such list corrupting the index, and because its `ENOMEM` arm cannot be reached at all while the rb-tree misreports allocation failures. The failures are reached through debug keywords: `hp_rb_write_key_oom_always` and `hp_rb_write_key_oom_once` make the tree node allocation fail, `hp_delete_key_not_found` makes a hash entry look missing from its chain. None of them may have another one as a prefix, because `DBUG_SET()` matches an already listed keyword by prefix and would silently leave the hook unarmed. The new unit test `hp_test_update` drives them through the engine API and validates the indexes with `heap_check_heap()`: the classification and its duplicate key counterpart, an already moved key being moved back, an error code outside the old list, and the case where the recovery itself cannot restore one key and has to repair the others anyway. `heap.update_key_rollback` covers the same recovery from SQL; without this fix it kills a debug server on the following `CHECK TABLE`.
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.
Problem
heap_update()moves every changed key entry to its new key value before it updates the record, so a failure raised afterwards leaves the index describing a record image that is not in the table. The recovery at theerr:label only ran for an enumerated set of error codes: originally justHA_ERR_FOUND_DUPP_KEY, widened by MDEV-40378 to also coverHA_ERR_RECORD_FILE_FULL,HA_ERR_OUT_OF_MEMandENOMEM.Any error outside that list skips the recovery entirely — for example the
HA_ERR_CRASHEDofhp_delete_key(). Lookups by the real column value then miss the row,CHECK TABLEreports the table corrupt, and on debug builds the consistency check inha_heap::external_lock()raises an error into an already set diagnostics area, firing aDiagnostics_areaassertion on a following statement.Interlocked with that,
hp_rb_write_key()reported every rejectedtree_insert()asHA_ERR_FOUND_DUPP_KEY, althoughtree_insert()also returns NULL when the allocation of the tree node fails. That made theHA_ERR_OUT_OF_MEMandENOMEMarms of the widened list unreachable on the rb-tree path, and it makes the server act on an allocation failure as if it were a duplicate:handler::is_fatal_error()reportsHA_ERR_FOUND_DUPP_KEYas not fatal to callers asking forHA_CHECK_DUP_KEY, andhandler::print_error()names a key throughinfo->errkeythat need not be unique at all.Correcting one without the other trades a wrong error for a silently corrupt index, so both change here.
Fix
hp_rb_write_key()tells the two failures apart.tree_insert()accounts the node inrb_tree.allocatedbefore it allocates it and leaves the counter alone when it rejects a duplicate, so a grown counter identifies the allocation attempt. The accounting for the node that was never allocated is taken back andHA_ERR_OUT_OF_MEMis returned.delete_key()andwrite_key()separately, because which of them failed decides what the failing key needs: a faileddelete_key()leaves the index untouched, a failedwrite_key()needs the old value written back, and a hash duplicate additionally needs the new entry removed, whichhp_write_key()deliberately leaves in place. Thekeydef == endcase of MDEV-40378, where the blob chain writes fail after every changed key was moved, keeps its behaviour.info->errkeyis only set for a duplicate key, the single error it describes.The condition asked for in the report,
HA_ERR_FOUND_DUPP_KEY || HA_ERR_RECORD_FILE_FULL || ENOMEM, landed with MDEV-40378; every error code it names still recovers here. It is generalized rather than extended once more because test 4 below shows an error code outside any such list corrupting the index, and because itsENOMEMarm cannot be reached at all while the rb-tree misreports allocation failures.Test
The failures are reached through debug keywords:
hp_rb_write_key_oom_alwaysandhp_rb_write_key_oom_oncemake the tree node allocation fail,hp_delete_key_not_foundmakes a hash entry look missing from its chain.New unit test
hp_test_updatedrives them through the engine API and validates the indexes withheap_check_heap():New
heap.update_key_rollbackcovers the same recovery from SQL; without this fix it kills a debug server on the followingCHECK TABLE.Verified by reintroducing the defects with the final test files in place: 8 of the 20 unit assertions fail and the MTR test kills the server. With the fix,
heapsuite 29/29,storage/heapunit tests 6/6,mainsuite 1429/1429.Base
Based on
preview-13.1-preview, so it currently also shows the MDEV-40378 commit from #5407. Once that is merged this PR contains a single commit.