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 == 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 == 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 same branch also covers a table with no keys at all (`share->keys == 0`), where the sweep has nothing to do. `info->errkey` is initialized to `-1` on entry to `err:`, so a failure that is not a key error can never expose a stale key number from an earlier operation. 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), a partial multi-row UPDATE, and a table with no indexes at all; each asserts the table stays consistent after the failure via `CHECK TABLE` and index lookups.
…shed when key recovery fails
`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. An out of memory during a
BTREE-indexed UPDATE was therefore reported as a duplicate: the user got
`ER_DUP_ENTRY` with a fabricated value, possibly naming a key that is
not unique at all, `handler::is_fatal_error()` treated the allocation
failure as not fatal for callers asking for `HA_CHECK_DUP_KEY`, and the
`HA_ERR_OUT_OF_MEM` / `ENOMEM` arms of the `heap_update()` recovery list
were unreachable on the rb-tree path.
`tree_insert()` now records why it returned NULL in the new
`TREE::error` (`TREE_ERROR_OOM` or `TREE_ERROR_DUP_KEY`), and
`hp_rb_write_key()` maps that to `HA_ERR_OUT_OF_MEM` or
`HA_ERR_FOUND_DUPP_KEY` (`HA_ERR_INTERNAL_ERROR` defensively, should a
new NULL source appear). With the classification corrected, the
allocation failure lands in the `HA_ERR_OUT_OF_MEM` arm of the
`heap_update()` recovery, which moves the already changed keys back, so
correcting the error report does not trade the wrong message for a
silently corrupt index.
The recovery at `err:` keeps its explicit list of error codes: those are
the errors we know how to recover from, and recovering from errors whose
meaning we do not know is worse than stopping. What changes around it:
1. `info->errkey` is only set for `HA_ERR_FOUND_DUPP_KEY`, the single
error it describes; every other failure leaves the `-1` that `err:`
starts with.
2. When the recovery itself cannot restore a key -- the re-insert of an
old key value fails -- or the failure is outside the list, the index
no longer describes the data and the table is marked **crashed** in
the new `HP_SHARE::state_changed` (bits and macros modeled on the
`state.changed` of Maria, see `storage/maria/maria_def.h`). A crashed
table refuses every read and write with `HA_ERR_CRASHED` ("Index for
table is corrupt"), `heap_check_heap()` reports it as damaged, and
`hp_clear()` -- reached through TRUNCATE or DELETE without WHERE --
clears the state along with the data, because it rebuilds the indexes
from nothing. Refusing a table known to be corrupt beats continuing
and delivering wrong results.
3. `delete_key()` is assumed to succeed: it allocates no memory, so it
cannot fail unless the table is already inconsistent, and building
recovery logic and injected failures for that case would complicate
the code for a scenario that cannot happen on a healthy table. If it
ever does fail, the error falls outside the recovery list and the
table is marked crashed, preserving the damaged state for analysis.
The debug-build consistency check in `ha_heap::external_lock(F_UNLCK)`
skips tables already marked crashed: their inconsistency is known and
deliberate, and re-detecting it would raise a second error into a
diagnostics area that can already hold OK, firing the
`Diagnostics_area` assertion the check exists to prevent.
The allocation failure is injected inside `tree_insert()` itself:
`simulate_tree_insert_oom` fails every node allocation until the caller
disarms it, `once_simulate_tree_insert_oom` fails only the next one and
disarms itself. The two names must not be a prefix of one another,
because `DBUG_SET()` matches an existing keyword by prefix and would
silently merge instead of adding. Callers arm the keywords themselves
and keep an inert guard keyword in the list, because a keyword list
that becomes empty while debugging is on matches every keyword.
The unit test `hp_test_update` drives the failures through the engine
API: the classification and its duplicate-key counterpart, an already
moved key being moved back, and the crashed lifecycle -- marking,
refusal of reads and writes, and the reset on emptying. With the
defects reintroduced, 8 of its 24 assertions fail.
`heap.update_key_rollback` covers the same from SQL; without the fix its
first UPDATE reports `ER_DUP_ENTRY 'Duplicate entry 101 for key k2'` on
a key that is not unique.
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.