MDEV-32067 InnoDB linear read ahead had better be logical - #4600
MDEV-32067 InnoDB linear read ahead had better be logical#4600Thirunarayanan wants to merge 1 commit into
Conversation
|
|
1fc14cb to
7fa9c70
Compare
7fa9c70 to
93d5f7d
Compare
dr-m
left a comment
There was a problem hiding this comment.
I only reviewed a small part of this so far. Please debug this with undo tablespace truncation enabled.
93d5f7d to
6aea84a
Compare
iMineLink
left a comment
There was a problem hiding this comment.
The new logic is a step forward towards more precise and efficient read-ahead logic.
I think that it can be extended easily to non MRR queries, by always passing a context to row_search_mvcc() initialized with the default 64 pages when no explicit limit is present.
| /* On other BLOB pages except the first the BLOB header | ||
| always is at the page data start: */ | ||
|
|
||
| offset = FIL_PAGE_DATA; |
There was a problem hiding this comment.
Maybe this is not useful now that offset = FIL_PAGE_DATA is in the for loop?
| { | ||
| skip: | ||
| space->release(); | ||
| return; |
There was a problem hiding this comment.
Maybe do not stop at first cached page?
| if (space->is_stopping()) | ||
| break; | ||
| buf_pool_t::hash_chain &chain= buf_pool.page_hash.cell_get(new_low.fold()); | ||
| space->reacquire(); |
There was a problem hiding this comment.
Since space->acquire() as added earlier, I suppose extra space->release() calls are needed to keep old behavior.
I'm looking at the early exit paths goto func_exit;, I suppose space->release() should be called before jumping.
There was a problem hiding this comment.
Now I think this was a wrong comment, discarding. Ok sorry, still valid. The acquire on top is balanced on the func_exit path, the reacquire() is needed to have one reference to consume in buf_read_page_low(). So here, before jumping to func_exit, the re-acquired references should be released.
6aea84a to
249de0f
Compare
The traditional linear read-ahead, enabled by innodb_read_ahead_threshold, only helps when consecutively accessed pages have adjacent page numbers. That is rarely true for B-tree leaf pages: after splits, merges and page reuse, the logical order of leaves has nothing to do with their physical page numbers. So for a scan of scattered leaves the old read-ahead either did nothing or read the wrong pages. This replaces linear read-ahead of index pages with a logical read-ahead that prefetches the actual leaf pages a scan is going to visit, discovered from the B-tree during traversal. After this change, the exact nonzero value of innodb_read_ahead_threshold matters only for the read-ahead of undo log pages. records_in_range() reports the first and last leaf page of a range, and the optimizer forwards that extent to the engine through handler::advise_page_range() so read-ahead can be sized and bounded to the scan. buf_read_ahead_undo(): Renamed from buf_read_ahead_linear(). Now invoked only for undo log pages, whose page numbers are sequential. No longer called on BLOB pages or index pages. buf_read_ahead_one(): Read ahead a single leaf page. Cannot be disabled. buf_read_ahead_pages(): Read ahead a set of known B-tree leaf pages. buf_read_ahead_random(): Split into a decision part (which extent to read, in buf0buf.cc) and an action part (issue the reads). The pending-read throttle now compares os_aio_pending_reads_approx() with buf_pool.curr_size() (a page count) instead of curr_pool_size() (bytes), which had made the throttle a no-op. buf_read_ahead_pages() releases the tablespace reference on a failed/corrupted page. btr_read_ahead_t: A context which records the level-1 page and the child page number of the last node pointer collected, so read-ahead can be resumed later. btr_read_ahead_collect(): scan the node pointers of a PAGE_LEVEL=1 page from a given record in scan order, appending child page numbers. btr_cur_t::search_leaf(), btr_cur_t::open_leaf(): At PAGE_LEVEL=1, after the descent has located the child to follow, harvest leaf page numbers starting at that child in the scan direction. These are exactly the leaves the cursor will visit, so the prefetch is precise regardless of physical page numbers. btr_read_ahead_resume_rec(): Locate the record from which read-ahead should resume, by re-finding the last harvested child by value. It walks only genuine records, so a concurrent page reorganization cannot cause an invalid read (a stored byte offset could). btr_pcur_move_to_next_page(), btr_pcur_move_backward_from_page(): Invoke buf_read_ahead_one() on the following/preceding sibling. btr_copy_blob_prefix(): Simplified; no longer relies on FIL_PAGE_PREV/NEXT. advise_page_range(): Records the scan's estimated leaf extent. Used only to size and bound read-ahead; it never gates the pages the scan actually reads. mrr_readahead_from_scan_range() derives the read-ahead ceiling from the advised extent, falling back to a LIMIT-based estimate. The window ramps up from small number of pages, doubling on each refill toward the ceiling, so single-row probes such as index_first() for MIN()/MAX() prefetch only a few leaves while long scans reach full depth. start_readahead(): For a just-positioned scan, prefetch the collected batch and set up the rolling cursor. readahead_refill(): As the scan advances (general_fetch()), resume the level-1 harvest under an index S-latch, chain across level-1 siblings, and prefetch the next batch bounded by the advised last leaf, or to the end of the index when the extent is unknown. Read-ahead is started from index_read() (range and full index scans), rnd_init()/general_fetch() (full table scans), and, at one-page depth, from row_merge_read_clustered_index() (OPTIMIZE/ALTER rebuild). general_fetch() sets active_handler_stats before issuing read-ahead so prefetched pages are attributed to the query. A truncation race is closed with a new STOPPING_READS tablespace flag: mtr_t::commit_shrink() sets it before shrinking and clears it after, and buf_read_ahead_undo() re-checks space->is_stopping(). trx_undo_get_prev_rec(), trx_undo_get_prev_rec_from_prev_page(): take the trx_undo_t object instead of a long parameter list and always latch shared. handler::multi_range_read_info_const() and DsMrr_impl::dsmrr_info_const() Passed a page_range parameter; the default MRR implementation aggregates the per-range leaf extents. opt_range.cc carries it through the chosen QUICK_RANGE_SELECT, and QUICK_RANGE_SELECT::reset() calls advise_page_range() before multi_range_read_init(). main.analyze_stmt_prefetch_count: corrected for double counting now that pages_read_count includes waits on prefetched pages.
249de0f to
72e3242
Compare
iMineLink
left a comment
There was a problem hiding this comment.
Thanks for addressing almost all my previous review comments.
I re-checked the code and there are few new comments, mostly around buf_read_page_low() usage (and possible usage) and buf_LRU_get_free_block() usage in read ahead.
Performance testing is still required to ensure no major regressions will be introduced even by this first implementation.
Please check also this buildbot failure under MSAN: https://buildbot.mariadb.org/#/builders/867/builds/15152
|
|
||
| const size_t zip_size{space->zip_size()}; | ||
| buf_block_t *block= | ||
| zip_size ? nullptr : buf_LRU_get_free_block(have_no_mutex); |
There was a problem hiding this comment.
Maybe use buf_read_acquire() which will not block but return nullptr instead?
| id.set_page_no(page); | ||
| buf_pool_t::hash_chain &chain= buf_pool.page_hash.cell_get(id.fold()); | ||
| buf_block_t *block= | ||
| zip_size ? nullptr : buf_LRU_get_free_block(have_no_mutex); |
There was a problem hiding this comment.
Maybe use buf_read_acquire() which will not block but return nullptr instead?
| Cost_estimate *cost); | ||
|
|
||
| int dsmrr_explain_info(uint mrr_mode, char *str, size_t size); | ||
| void set_limit(ha_rows limit) { limit_hint= limit; } |
| read_ahead: | ||
| if (space->is_stopping()) | ||
| goto no_read_ahead; | ||
| if (!buf_read_page_low(i, zip_size, nullptr, chain, space, block)) |
There was a problem hiding this comment.
This may be ignoring -1 and -2 as return values?
@retval -1 if err==nullptr and an asynchronous read was submitted
@retval -2 if err==nullptr and the page exists in the buffer pool
| if (!block) | ||
| goto func_exit; | ||
|
|
||
| if (!buf_read_page_low(new_low, 0, nullptr, chain, space, block)) |
There was a problem hiding this comment.
This may be ignoring -1 and -2 as return values?
@retval -1 if err==nullptr and an asynchronous read was submitted
@retval -2 if err==nullptr and the page exists in the buffer pool
| goto func_exit; | ||
| } | ||
| const ulint len{zip_size ? zip_size : srv_page_size}; | ||
| if (UNIV_LIKELY(space->io(IORequest(IORequest::READ_ASYNC), |
There was a problem hiding this comment.
If this returns an error, it seems internally it will release the space already. So in this case, it should not be re-released here at function exit.
| else | ||
| { | ||
| buf_pool.corrupted_evict(bpage, buf_page_t::READ_FIX); | ||
| space->release(); |
There was a problem hiding this comment.
If space->io() above returns an error, it seems internally it will release the space already. So in this case, the space should not be re-released here. Maybe we should just use buf_read_page_low() instead? (same could apply to similar code above)
There was a problem hiding this comment.
Pull request overview
This PR improves InnoDB read-ahead by shifting from physical “linear page number adjacency” assumptions to logical read-ahead driven by actual B-tree leaf pages discovered during traversal (MRR-aware). It also extends the MRR interface to pass page-range hints from the optimizer to the storage engine so the engine can size/enable read-ahead more accurately, and hardens undo read-ahead against tablespace truncation races.
Changes:
- Extend
handler::multi_range_read_info_const()to return an optionalpage_rangehint and addhandler::advise_page_range(); plumb these through optimizer/MRR and multiple engines. - Add an MRR-aware logical leaf-page collector (
btr_read_ahead_t) and use it in InnoDB scans to prefetch upcoming leaf pages in batches. - Refactor buffer read-ahead into undo-specific linear read-ahead plus new “one page” and “many pages” helpers; add STOPPING_READS handling for truncation.
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| storage/videx/ha_videx.cc | MRR API signature updated to accept page_range*. |
| storage/spider/ha_spider.h | MRR API signature updated to accept page_range*. |
| storage/spider/ha_spider.cc | Plumb page_range* through Spider’s MRR info path. |
| storage/myisam/ha_myisam.h | MRR API signature updated to accept page_range*. |
| storage/myisam/ha_myisam.cc | Pass page_range* into DS-MRR. |
| storage/mroonga/ha_mroonga.hpp | MRR API signature updated to accept page_range* across wrapper/storage paths. |
| storage/mroonga/ha_mroonga.cpp | Plumb page_range* through Mroonga wrapper and storage MRR calls. |
| storage/maria/ha_maria.h | MRR API signature updated to accept page_range*. |
| storage/maria/ha_maria.cc | Pass page_range* into DS-MRR. |
| storage/connect/ha_connect.h | MRR API signature updated to accept page_range*. |
| storage/connect/ha_connect.cc | Pass page_range* into DS-MRR. |
| sql/handler.h | Add page_range* to MRR info API; add advise_page_range() hook. |
| sql/multi_range_read.h | DS-MRR API updated to accept page_range*; store LIMIT hint. |
| sql/multi_range_read.cc | Compute/return combined page-range hint across ranges when available. |
| sql/opt_range.h | Store page_range hint on QUICK_RANGE_SELECT. |
| sql/opt_range.cc | Collect and propagate page_range hint into engines via advise_page_range() + MRR. |
| sql/ha_partition.h | Partition MRR API signature updated to accept page_range*. |
| sql/ha_partition.cc | Plumb page_range* through partition MRR calls (currently with an issue). |
| storage/innobase/include/buf0rea.h | New undo/index read-ahead APIs: undo linear, one-page, multi-page. |
| storage/innobase/buf/buf0rea.cc | Implement refactored read-ahead functions (undo/one/pages/random). |
| storage/innobase/buf/buf0buf.cc | Move random read-ahead trigger logic to buf_pool page-fix path. |
| storage/innobase/include/fil0fil.h | Add STOPPING_READS flag setters/clearers to fil_space_t. |
| storage/innobase/fil/fil0fil.cc | Implement fil_space_t::set_stopping_reads() / clear_stopping_reads(). |
| storage/innobase/mtr/mtr0mtr.cc | Set/clear STOPPING_READS during tablespace shrink to avoid readahead races. |
| storage/innobase/include/btr0types.h | Add btr_read_ahead_t collector type. |
| storage/innobase/include/btr0cur.h | Declare read-ahead collection/resume helpers and thread context through cursor APIs. |
| storage/innobase/btr/btr0cur.cc | Implement logical leaf collection and resume helpers; thread context through leaf search (currently with an issue). |
| storage/innobase/include/btr0pcur.inl | Pass read-ahead context into search_leaf(). |
| storage/innobase/include/btr0pcur.h | Extend persistent cursor open calls with optional read-ahead context. |
| storage/innobase/btr/btr0pcur.cc | Switch leaf-page movement to buf_read_ahead_one(); add backward fast-path. |
| storage/innobase/include/row0sel.h | Extend row_search_mvcc() signature with optional read-ahead context. |
| storage/innobase/row/row0sel.cc | Pass read-ahead context into pcur open calls. |
| storage/innobase/handler/ha_innodb.h | Store scan extent hint + rolling window state; add advise_page_range() override. |
| storage/innobase/handler/ha_innodb.cc | Implement read-ahead window sizing/refill and start logic for scans; use advise_page_range() hint. |
| storage/innobase/trx/trx0undo.cc | Undo-page read-ahead renamed/updated; strengthen around truncation (acquire/release). |
| storage/innobase/include/trx0undo.h | Update undo traversal APIs to accept trx_undo_t& instead of discrete header args. |
| storage/innobase/trx/trx0trx.cc | Update undo traversal call sites to new signature. |
| storage/innobase/row/row0undo.cc | Update undo traversal call site to new signature. |
| storage/innobase/dict/dict0stats.cc | Prefetch sample dive child pages during stats analysis using multi-page read-ahead. |
| storage/innobase/row/row0merge.cc | Prefetch next clustered leaf during sequential clustered index scan. |
| mysql-test/main/analyze_stmt_prefetch_count.test | Adjust test logic to avoid double-counting prefetched reads. |
| mysql-test/main/analyze_stmt_prefetch_count.result | Update expected output for revised prefetch/read accounting. |
Comments suppressed due to low confidence (1)
storage/innobase/btr/btr0cur.cc:2166
- There is an empty
if (latch_mode != BTR_MODIFY_TREE);statement due to a stray semicolon. This is very easy to misread and makes the control flow brittle; it should be expressed as a single explicit condition for the BTR_MODIFY_TREE case.
if (latch_mode != BTR_MODIFY_TREE);
else if (btr_cur_need_opposite_intention(block->page, index->is_clust(),
lock_intention,
node_ptr_max_size, compress_limit,
page_cur.rec))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* Prevent race condition with undo tablespace truncation by checking | ||
| if the space is being truncated (STOPPING_READS flag set) */ | ||
| if (!space->acquire()) | ||
| return 0; |
| &m_mrr_buffer_size[i], | ||
| &tmp_mrr_mode, limit, &part_cost); | ||
| &tmp_mrr_mode, pr, limit, &part_cost); | ||
| *pr= {0, 0}; |
| /* Basically it should give the union of page extents of | ||
| every range in the scan. Initialize with unused_page_range | ||
| until the first range reports; If it is unused_page_range then | ||
| widened the search to whole tablespace .*/ | ||
| page_range range_covering_all= unused_page_range; |
MDEV-32067 InnoDB linear read ahead had better be logical