Marko Mäkelä added a comment - 2020-05-07 08:46 - edited The ‘extra’ call to ibuf_insert_low() is explained by the following: err = ibuf_insert_low(BTR_MODIFY_PREV, op, no_counter, entry, entry_size, index, page_id, zip_size, thr); if (err == DB_FAIL) { err = ibuf_insert_low(BTR_MODIFY_TREE | BTR_LATCH_FOR_INSERT, op, no_counter, entry, entry_size, index, page_id, zip_size, thr); } That is, no duplicate record was inserted into the change buffer for either the DELETE (@r, 36) or the DELETE_MARK (NULL, 521) . Only 14 records were present when the DELETE (NULL, 520) was wrongly buffered. In ibuf_get_volume_buffered() we check also the preceding change buffer page. This worries me a little, because normally, a backward B-tree scan in InnoDB could lead to a deadlock of threads, and we normally save the cursor position and search for the preceding record to move to the preceding page. Only if the caller ibuf_insert_low() was invoked with mode = BTR_MODIFY_TREE | BTR_LATCH_FOR_INSERT would there likely be no concern, because we would be holding ibuf_mutex and ibuf_pessimistic_insert_mutex . However, that call has been there since the inclusion of InnoDB in MySQL . If I remember correctly the discussions between 2005 and 2007, going back one more page could lead to deadlocks. So, I guess that we are fine with regard to deadlocks here. (Also, I am not aware of any actual deadlocks in this code.) The following hash function ought to work. False positives are not a correctness problem. For example, ('fu','bar'), (NULL,'fubar'),('fub','ar') would be treated as equal, and we would increment min_n_rec by at most 1 for those 3 distinct keys. /** Determine if a change buffer record has been encountered already. @param rec change buffer record in the MySQL 5.5 format @param hash hash table of encountered records @param size number of elements in hash @retval true if a distinct record @retval false if this may be duplicating an earlier record */ static bool ibuf_get_volume_buffered_hash( const rec_t *rec, ulint *hash, ulint size) { ut_ad(rec_get_n_fields_old(rec) > IBUF_REC_FIELD_USER); const ulint start= rec_get_field_start_offs(rec, IBUF_REC_FIELD_USER); const ulint len= rec_get_data_size_old(rec) - start; const uint32_t fold= ut_crc32(rec + start, len); hash+= (fold / (CHAR_BIT * sizeof *hash)) % size; ulint bitmask= static_cast <ulint>(1) << (fold % (CHAR_BIT * sizeof (*hash)));   if (*hash & bitmask) return false ;   /* We have not seen this record yet. Remember it. */ *hash|= bitmask; return true ; } The comparison min_n_recs < 2 in ibuf_insert_low() actually does check for underflow, because the variable is a signed type ( lint , not ulint ). With the corrected hash function, the estimate of min_n_recs looks conservative to me, even if there were multiple change buffer pages of buffered changes and the estimate did not consider them all. In the worst case, we missed many preceding pages filled with IBUF_DELETE_OP records that had been buffered for the user page earlier. But, it would have been an error to buffer those operations if the records did not exist in the first place. So, it is OK to ignore any preceding pages of buffered operations and start with a conservative estimate that there exist at least min_n_recs=0 records in the page. Update: I tried 10.2 with the following patch: diff --git a/storage/innobase/include/page0page.ic b/storage/innobase/include/page0page.ic index 4efb92f7182..276f85df69a 100644 --- a/storage/innobase/include/page0page.ic +++ b/storage/innobase/include/page0page.ic @@ -173,9 +173,7 @@ page_header_set_field( { ut_ad(page); ut_ad(field <= PAGE_N_RECS); -#if 0 /* FIXME: MDEV-19344 hits this */ ut_ad(field != PAGE_N_RECS || val); -#endif ut_ad(field == PAGE_N_HEAP || val < srv_page_size); ut_ad(field != PAGE_N_HEAP || (val & 0x7fff) < srv_page_size); but the test innodb.innodb-change-buffer-recovery did not fail in 70 runs even without my fix. I will try harder, possibly with an older version, so that I can test my fix.

Marko Mäkelä added a comment - 2020-05-07 10:08 So far, I have been unable to repeat the assertion failure on 10.2. It looks like the result difference was a caused by the test not removing the file my_restart.err between runs, causing a failure on ./mtr --repeat . That test problem was fixed in 10.2.28, 10.3.19, 10.4.10, 10.5.0 .

Matthias Leich added a comment - 2020-05-07 10:58 The rr trace mentioned above belongs to some test run on a 10.5 develoment tree where the server aborts with (snip from server error log slightly edited) [ERROR] InnoDB: Unable to purge a record mysqld: /home/mleich/bb-10.5-marko/storage/innobase/ibuf/ibuf0ibuf.cc:3994: void ibuf_delete(const dtuple_t*, buf_block_t*, dict_index_t*, mtr_t*): Assertion `0' failed. [ERROR] mysqld got signal 6 ;

Marko Mäkelä added a comment - 2020-05-07 16:02 mleich reported the assertion on crash recovery. It turns out that the assertion was missing || recv_recovery_is_on() at the end. The reason for this is the following condition, which has been removed in 10.5 ( MDEV-12353 ): if (page_get_n_recs(page) == 1 && !recv_recovery_is_on()) { /* Empty the page, unless we are applying the redo log during crash recovery. During normal operation, the page_create_empty() gets logged as one of MLOG_PAGE_CREATE, MLOG_COMP_PAGE_CREATE, MLOG_ZIP_PAGE_COMPRESS. */ ut_ad(page_is_leaf(page)); /* Usually, this should be the root page, and the whole index tree should become empty. However, this could also be a call in btr_cur_pessimistic_update() to delete the only record in the page and to insert another one. */ page_cur_move_to_next(cursor); ut_ad(page_cur_is_after_last(cursor)); page_create_empty(page_cur_get_block(cursor), const_cast <dict_index_t*>(index), mtr); return ; } Only during crash recovery, we can decrement PAGE_N_RECS from 1 to 0. In all other cases, we would invoke page_create_empty() . The assertion failed on the change buffer tree, because the change buffer was emptied during crash recovery.

Marko Mäkelä added a comment - 2020-05-07 18:10 Summary: The test was not tolerant of ./mtr --repeat=2 and the assertion that was added in a hope to catch the bug that I now filed and fixed as MDEV-22497 was bogus. I removed the assertion that was already bracketed inside #if 0 .