Optimistic btr_pcur_t restore accepts a stale position after the buffer pool shrinks and grows again

This issue was discovered while working at MDEV-32286 and addressing a review comment in #5625, which attributed at the PR itself what actually inherited from a pre-existing issue with modify_clock usage.
Please find below the AI-aided description and repro test case: note that the window for this to occur is rather small and requires at least a buffer pool shrink and grow occurring during the "usually short" lifetime of a stored cursor.
I think this may affect as well some reasoning about modify_clock handling that is ongoing in MDEV-33966's PR #5763.

Description

btr_pcur_t::restore_position() tries an optimistic restore first, for BTR_SEARCH_LEAF and BTR_SEARCH_PREV (and before MDEV-34458 also for BTR_MODIFY_LEAF). It accepts the stored position when these checks pass:

• buf_page_optimistic_fix(): the block descriptor that the cursor remembers (page_cur.block) holds the stored page id (old_page_id).
• buf_page_optimistic_get(): after the page latch is acquired, block->modify_clock is equal to the stored modify_clock.

These checks assume that modify_clock only increases during the life of a descriptor. Eviction, record removal, page creation and page reorganization increment it. A shrink followed by a grow breaks this assumption:

1. buf_pool_t::shrink() withdraws the descriptors at the end of the pool. It moves their pages to other blocks or evicts them, and shrunk() decommits the memory.
2. buf_pool_t::resize() grows the pool again. It zero-fills the new descriptors (memset in the grow path), which are at the same addresses as the withdrawn ones. So modify_clock restarts at 0.
3. A read puts the same page into the same descriptor. The page id matches, and modify_clock can be equal to the stored value again. The easiest case is 0: a cursor that was stored on a page that had been read into the descriptor right after an earlier grow.

The page content can change while the page is out of the descriptor. The restore then accepts a record pointer that points to other data.

In a debug build, this assertion in btr_pcur_t::restore_position() fails:

btr0pcur.cc:374: btr_pcur_t::restore_status btr_pcur_t::restore_position(btr_latch_mode, mtr_t*): Assertion `!cmp_rec_rec(old_rec, rec, offsets1, offsets2, index)' failed.

(The line is 388 on 10.11 and 372 on 11.4.)

How to repeat

Attached: buf_pool_resize_pcur_restore_debug.test and buf_pool_resize_pcur_restore_debug.opt. The test needs a debug build (debug_sync) and innodb_page_size=16k. Put the files in mysql-test/suite/innodb/t/ and run:

./mtr --mem innodb.buf_pool_resize_pcur_restore_debug

--innodb-buffer-pool-size=8M

--innodb-buffer-pool-size-max=16M

--innodb-lru-scan-depth=100

The test does these steps:

1. It grows the pool from 8M to 12M. Then it reads single pages (one partition of a 256-partition table for each lookup) until only the new blocks are free. A grow appends the new blocks to the end of the free list, so the next read uses the first new descriptor. The test reads leaf page P of table t into that descriptor, and it checks the placement with INFORMATION_SCHEMA.INNODB_BUFFER_PAGE.BLOCK_ID.
2. A READ UNCOMMITTED reader scans t and stops in a stored function on pk=3, which is an uncommitted insert of another connection. The cursor stores block B, page P and modify_clock 0.
3. The insert of pk=3 is rolled back, which removes the record without purge. An insert of pk=103 with the same record size reuses the freed record. The stored offset now holds pk=103.
4. The pool shrinks to 8M, which moves P out of B. P is flushed and evicted by a scan of a table that is three times the size of the pool.
5. The pool grows to 12M again, so B is zero-filled. The same counting step reads P into B, with modify_clock 0.
6. The reader resumes. The optimistic restore succeeds on the record at the stale offset (pk=103), and the assertion fails.

In 20 of 20 runs on each of 10.11, 11.4 and 11.8, the assertion failed.

A correct restore makes the reader return 1,2,3,4,...,20,103. Without the debug assertion, the reader continues after pk=103, which is the last record, so it is expected to return 1,2,3. This was not observed, because the test cannot run on a release build.

Effect in release builds

The check of the restored record against old_rec is debug-only. The effect depends on the caller and on the data at the stale offset:

These effects come from reading the code. Only the wrong position itself was reproduced, through the debug assertion.

• Reads: row_search_mvcc() restores with BTR_SEARCH_LEAF, for consistent reads and for locking reads. When the offset is at the start of another record, the scan continues from there. Rows are skipped or returned twice, with no error. The scan phase of UPDATE and DELETE uses the same restore, so these statements can miss rows or process a row twice.
• UPDATE and DELETE on 10.11: row_upd_clust_step() restores with BTR_MODIFY_LEAF, which tries the optimistic restore before MDEV-34458. When the result is SAME_ALL, it changes the record at the restored position, and when has_clust_rec_x_lock is set, it does not check the lock again. So another row can be updated or delete-marked, and the change is written to the redo log. From 11.4.3 (2f6df937480), BTR_MODIFY_LEAF always does a search, so this path is not affected.
• Offset not at the start of a record (the page was reorganized or split while it was away): page_rec_get_next_low() has bounds checks on the next-record offset, but the record header at the stale offset is not validated before rec_get_offsets() and the row conversion use it. The result can be an error, wrong data or a crash.
• Purge: row_purge_poss_sec() restores with BTR_SEARCH_LEAF, and row_purge_is_unsafe() does not check that the record belongs to the row of the undo record. With a wrong record, purge can remove a delete-marked secondary index entry that a version visible to an active read view still needs, so that read view can miss the row in a secondary index scan. The removal of the clustered index record is protected: row_purge_remove_clust_if_poss_low() compares DB_ROLL_PTR first.

The window is the life of one stored cursor, for example a long scan or a slow client between two fetches. In that window, the pool must shrink and grow again, and the page must come back into the same descriptor with an equal modify_clock.

History

Possible fixes

1. Keep modify_clock monotonic for each descriptor address. Keep a global high-water mark in buf_pool_t. When shrink() withdraws a descriptor, and before shrunk() decommits the memory, set the mark to the maximum of the mark and modify_clock + 1. When the grow path initializes a new descriptor, set its modify_clock to the mark. A stored value is not more than the clock of the descriptor at the time that it was withdrawn, so it is less than any value after the grow. Only shrink() and the grow path access the mark, and both hold buf_pool.mutex, so it is a plain variable with no atomic operations. The increments of modify_clock and the restore path do not change.
2. Add a resize generation, as buf_withdraw_clock did before MDEV-23693. btr_pcur_t stores it, and buf_page_optimistic_fix() compares it. This adds one load and one comparison to each optimistic restore.

Option 1 also fixes every other user of the "same descriptor, same page id, same modify_clock" check, for example the clustered leaf hint that MDEV-32286 proposes (not merged).