Skip to content

MDEV-32067 InnoDB linear read ahead had better be logical - #4600

Open
Thirunarayanan wants to merge 1 commit into
mainfrom
main_MDEV-32067
Open

Thirunarayanan wants to merge 1 commit into
mainfrom
main_MDEV-32067

Conversation

@Thirunarayanan

@Thirunarayanan Thirunarayanan commented Jan 28, 2026 •

Copy link
Copy Markdown
Member

MDEV-32067 InnoDB linear read ahead had better be logical

The traditional linear read-ahead, enabled by innodb_read_ahead_threshold=56,
only works if pages are allocated on adjacent page numbers, which is not
always the case for B-tree leaf pages.

After this change, the exact nonzero values of
innodb_read_ahead_threshold matter only for the read-ahead of
undo log pages.

Introduced Multi-Range Read (MRR) aware read-ahead that collects
actual leaf page numbers during B-tree traversal

buf_read_ahead_undo(): Renamed from buf_read_ahead_linear().
This function will no longer be invoked on any BLOB pages
(for which FIL_PAGE_PREV and FIL_PAGE_NEXT were not initialized
consistently) nor on any index pages. For index leaf pages,
we will introduce buf_read_ahead_one() and buf_read_ahead_pages().

buf_read_ahead_one(): Read ahead one (sibling leaf) page.
This logic cannot be disabled.

buf_read_ahead_pages(): Read ahead B-tree index leaf pages.

buf_read_ahead_random(): Split the function into two parts: one
that determines which range of pages should be read, and another
that actually initiates a read of the pages.

btr_pcur_move_to_next_page(): Invoke buf_read_ahead_one()
instead of buf_read_ahead_linear().

btr_pcur_move_backward_from_page(): Implement a fast path of
trying to acquire a latch on the previous page without waiting,
and invoke buf_read_ahead_one() on the preceding page, with the
assumption that we may be accessing that page in the near future.

btr_copy_blob_prefix(): Simplify the logic. On other than
ROW_FORMAT=COMPRESSED BLOB pages, the FIL_PAGE_NEXT field is not
meaningfully initialized. The FIL_PAGE_PREV field is not pointing
to anything meaningful either. buf_read_ahead_linear() expects
these to be set meaningfully. Only the non-default setting
innodb_random_read_ahead=ON might be meaningful here.

btr_cur_t::search_leaf(): Add MRR read-ahead context to collect
leaf page numbers at PAGE_LEVEL=1 during B-tree traversal.
The collected page numbers represent actual leaf pages that
will be accessed, enabling more targeted
read-ahead than linear page number assumptions.

mrr_readahead_ctx_t: New structure for passing MRR context
through the call chain from ha_innobase -> row_search_mvcc()
-> btr_pcur_open() -> search_leaf() and it has
READ_AHEAD_PAGES=64 limit.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Thirunarayanan
Thirunarayanan force-pushed the main_MDEV-32067 branch 5 times, most recently from 1fc14cb to 7fa9c70 Compare February 2, 2026 07:21
@Thirunarayanan
Thirunarayanan requested a review from dr-m February 3, 2026 09:03
@Thirunarayanan
Thirunarayanan marked this pull request as ready for review February 3, 2026 09:03

@dr-m dr-m left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only reviewed a small part of this so far. Please debug this with undo tablespace truncation enabled.

Comment thread storage/innobase/buf/buf0rea.cc Resolved
Comment thread storage/innobase/buf/buf0rea.cc Resolved
Comment thread storage/innobase/buf/buf0rea.cc Resolved
Comment thread storage/innobase/buf/buf0rea.cc Resolved
Comment thread storage/innobase/buf/buf0rea.cc Resolved
Comment thread storage/innobase/buf/buf0rea.cc Resolved
Comment thread storage/innobase/buf/buf0rea.cc Resolved
Comment thread storage/innobase/buf/buf0rea.cc Resolved

@iMineLink iMineLink left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread sql/multi_range_read.cc Resolved
Comment thread sql/multi_range_read.cc Resolved
Comment thread sql/multi_range_read.cc Resolved
Comment thread sql/multi_range_read.cc Resolved
Comment thread sql/multi_range_read.cc Resolved
Comment thread storage/innobase/btr/btr0cur.cc Resolved
/* On other BLOB pages except the first the BLOB header
always is at the page data start: */

offset = FIL_PAGE_DATA;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is not useful now that offset = FIL_PAGE_DATA is in the for loop?

{
skip:
space->release();
return;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe do not stop at first cached page?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we must continue here, to allow covering the whole read ahead area?

if (space->is_stopping())
break;
buf_pool_t::hash_chain &chain= buf_pool.page_hash.cell_get(new_low.fold());
space->reacquire();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@iMineLink iMineLink Jul 27, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extra space->release() calls are needed before the two goto func_exit statements below. If we call buf_read_page_low(), it'll consume the re-acquired reference.

Comment thread storage/innobase/buf/buf0rea.cc Resolved

@iMineLink iMineLink left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread storage/innobase/buf/buf0rea.cc Resolved
Comment thread storage/innobase/buf/buf0rea.cc Resolved
Comment thread sql/multi_range_read.h Resolved
Comment thread storage/innobase/buf/buf0rea.cc Resolved
Comment thread storage/innobase/buf/buf0rea.cc Resolved
Comment thread storage/innobase/buf/buf0rea.cc Outdated
goto func_exit;
}
const ulint len{zip_size ? zip_size : srv_page_size};
if (UNIV_LIKELY(space->io(IORequest(IORequest::READ_ASYNC),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be fixed if we use buf_read_page_low() here instead.

Comment thread storage/innobase/buf/buf0rea.cc Outdated
else
{
buf_pool.corrupted_evict(bpage, buf_page_t::READ_FIX);
space->release();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, you can just avoid space->release() here, aligning to buf_read_page_low().
But yeah I think we better re-use buf_read_page_low() here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think it's better to reuse buf_read_page_low() here and in buf_read_ahead_one(), to keep "tricky" code in a single function.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 optional page_range hint and add handler::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.

Comment thread storage/innobase/buf/buf0rea.cc Outdated
Comment on lines 607 to 610
/* Prevent race condition with undo tablespace truncation by checking
if the space is being truncated (STOPPING_READS flag set) */
if (!space->acquire())
return 0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is relevant, documentation says @param space undo tablespace or fil_system.space, or nullptr. But is likely an issue of the documentation? Maybe add explicit ut_ad(space) as well?

Comment thread sql/ha_partition.cc Outdated
&m_mrr_buffer_size[i],
&tmp_mrr_mode, limit, &part_cost);
&tmp_mrr_mode, pr, limit, &part_cost);
*pr= {0, 0};
Comment thread sql/multi_range_read.cc
Comment on lines +196 to +200
/* 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;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the AI generated suggestion is valid and needs some attention.

This seems to be one of the entry points to the changed API. Some other entry points are in sql/opt_range.cc.

The page_range data type was introduced in f36ca14, which promised to implement a follow-up change to handler::multi_range_read_info_const(). I was wondering if sizeof(page_range) needs to be that large. InnoDB page numbers are limited to 32 bits. I see that for ENGINE=Aria and ENGINE=MyISAM, the "page numbers" are actually byte offsets to the data heap files. So, we have to live with such unnecessarily large "page numbers" in this API.

I was also wondering if the page_range could be added to one of the existing parameters that are being passed to handler::multi_range_read_info_const(), but I don’t see any obvious way. We are already well past the number of parameters that can be passed in registers in common AMD64 ABIs, so I don’t think it makes much difference to pass one more parameter via the stack.

Comment thread storage/connect/ha_connect.cc Outdated
void *seq_init_param,
uint n_ranges, uint *bufsz,
uint *flags, ha_rows limit,
uint *flags, page_range* pr,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backup test failing ,
Please check if this Assertion is related
origin/main_MDEV-32067 72e32423011943e5475be483f6805b833a6fd1c5

# 2026-08-03T09:19:38 [3486246] | mariadbd: /data/Server/main_MDEV-32067/storage/innobase/srv/srv0start.cc:1183: dberr_t srv_log_rebuild(): Assertion !os_aio_pending_reads()' failed.`

RR trace is present on SDP :-
/data/results/1785746415/TBR-2462

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed by this change:

diff --git a/storage/innobase/buf/buf0rea.cc b/storage/innobase/buf/buf0rea.cc
index 88a05bb9cdc..6244b1cf861 100644
--- a/storage/innobase/buf/buf0rea.cc
+++ b/storage/innobase/buf/buf0rea.cc
@@ -455,6 +455,8 @@ void buf_read_ahead_random(fil_space_t *space, page_id_t low,
 @param page     page to read ahead */
 void buf_read_ahead_one(fil_space_t *space, uint32_t page) noexcept
 {
+  if (srv_startup_is_before_trx_rollback_phase)
+    return;
   /* Unlike buf_read_ahead_undo(), we do not re-check space->is_stopping():
   this is used for user index/table tablespaces, which are not shrunk under a

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be solved now?

Comment thread sql/handler.h
void *seq_init_param,
uint n_ranges, uint *bufsz,
uint *mrr_mode, ha_rows limit,
uint *mrr_mode, page_range *pr,

@mariadb-SaahilAlam mariadb-SaahilAlam Aug 4, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check if this Assertion is related
# 2026-08-04T04:39:05 [1983730] | mariadbd: /data/Server/main_MDEV-32067/sql/handler.h:3713: int handler::ha_index_end(): Assertion inited==INDEX' failed.
RR trace is present on SDP:-/data/results/1785843487/000002`

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CREATE TABLE t1 (pk INT PRIMARY KEY) ENGINE=InnoDB;
INSERT INTO t1 VALUES (200),(300);
connect  con1,localhost,root,,test;
START TRANSACTION WITH CONSISTENT SNAPSHOT;
connection default;
TRUNCATE TABLE t1;
connection con1;
SELECT MAX(pk) FROM t1 WHERE pk BETWEEN 142 AND 642;
innodb.f1                                [ fail ]
        Test ended at 2026-08-31 13:28:43

CURRENT_TEST: innodb.f1
mysqltest: At line 16: query 'SELECT MAX(pk) FROM t1 WHERE pk BETWEEN 142 AND 642' failed with wrong errno <Unknown> (2013): 'Lost connection to server during query', instead of ER_TABLE_DEF_CHANGED (1412)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happens on 10.11 as well. I don't think it is related to this patch.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an MDEV for this issue?

Comment thread sql/ha_partition.cc
void *seq_init_param,
uint n_ranges, uint *bufsz,
uint *mrr_mode, ha_rows limit,
uint *mrr_mode, page_range *pr,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check if this Assertion is related:-

# 2026-08-04T04:40:25 [2014037] | mariadbd: /data/Server/main_MDEV-32067/sql/ha_partition.cc:7461: bool ha_partition::can_skip_merging_scans(): Assertion 0' failed.`

RR trace is on SDP :-
/data/results/1785843487/MDEV-32067_02

@Thirunarayanan Thirunarayanan Aug 31, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filesort() calls rnd_init(scan=true), which resets m_index_scan_type to partition_no_index_scan but
leaves m_mrr_mode and m_multi_range_read_first stale from an earlier MRR scan. The next BKA
refill hits multi_range_key_create_key() returning HA_ERR_END_OF_FILE (no keys), which multi_range_read_init() swallows as success at without setting any state.
So multi_range_read_next() runs on stale flags and makes assertion failure in can_skip_merging_scans() 

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to this patch

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was a bug filed for this test failure?

@iMineLink iMineLink left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a couple of places in which space->io() was added: I suggest to refactor them to re-use buf_read_page_low() instead, which has got all the details correct already. New buf_LRU_get_free_block() usages might better pass via buf_read_acquire() instead. There is also one comment about the termination of random read-ahead loop, unsure about that one. Then, one Copilot comment about a potentially null pointer seems valid, but solution might just be documentation adjustment.

Comment thread storage/innobase/buf/buf0rea.cc Outdated
else
{
buf_pool.corrupted_evict(bpage, buf_page_t::READ_FIX);
space->release();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise, you can just avoid space->release() here, aligning to buf_read_page_low().
But yeah I think we better re-use buf_read_page_low() here.

Comment thread storage/innobase/buf/buf0rea.cc Resolved
if (space->is_stopping())
break;
buf_pool_t::hash_chain &chain= buf_pool.page_hash.cell_get(new_low.fold());
space->reacquire();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extra space->release() calls are needed before the two goto func_exit statements below. If we call buf_read_page_low(), it'll consume the re-acquired reference.

Comment thread storage/innobase/buf/buf0rea.cc Resolved
Comment thread storage/innobase/buf/buf0rea.cc Outdated
goto func_exit;
}
const ulint len{zip_size ? zip_size : srv_page_size};
if (UNIV_LIKELY(space->io(IORequest(IORequest::READ_ASYNC),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be fixed if we use buf_read_page_low() here instead.

{
skip:
space->release();
return;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we must continue here, to allow covering the whole read ahead area?

Comment thread storage/innobase/buf/buf0rea.cc Outdated
Comment on lines 607 to 610
/* Prevent race condition with undo tablespace truncation by checking
if the space is being truncated (STOPPING_READS flag set) */
if (!space->acquire())
return 0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is relevant, documentation says @param space undo tablespace or fil_system.space, or nullptr. But is likely an issue of the documentation? Maybe add explicit ut_ad(space) as well?

@dr-m dr-m left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to review the InnoDB changes before this is merged. Here just a quick comment on naming.

Comment thread storage/innobase/include/btr0types.h Resolved
Comment thread storage/innobase/include/btr0types.h Resolved
Comment thread storage/innobase/include/btr0types.h Resolved
Comment thread sql/ha_partition.cc
void *seq_init_param,
uint n_ranges, uint *bufsz,
uint *mrr_mode, ha_rows limit,
uint *mrr_mode, page_range *pr,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was a bug filed for this test failure?

Comment on lines +126 to +128
--let $ra_query= SELECT * FROM t1 ORDER BY id LIMIT 1000
--source suite/innodb/include/ra_stats.inc
SELECT @pages_prefetch < 64 AS limit1000_bounded;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test file is failing in many environment. There is at least one failure for this particular test step:

CURRENT_TEST: innodb.logical_read_ahead
--- /home/buildbot/amd64-openeuler-2403/build/mysql-test/suite/innodb/r/logical_read_ahead.result	2026-10-08 08:48:45.000000000 +0000
+++ /home/buildbot/amd64-openeuler-2403/build/mysql-test/suite/innodb/r/logical_read_ahead.reject	2026-10-08 21:09:40.732866519 +0000
@@ -123,7 +123,7 @@
 1
 SELECT @pages_prefetch < 64 AS limit1000_bounded;
 limit1000_bounded
-1
+0
 # Multi-range read: ten single-key ranges
 SET optimizer_switch='mrr=on,mrr_cost_based=off';
 SELECT @pages_prefetch <= 10 AS mrr_unique_lookups_no_batch;

Result content mismatch

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On 1bb1738 (with the bound now <= 64) this still fails at 4k and 8k on every run, while 16k, 32k and 64k pass. ORDER BY id LIMIT 1000 is a full index scan, which does not go through multi_range_read_info_const(), so the LIMIT does not bound the read-ahead (index_read() calls init_readahead_window(buf_pool_t::READ_AHEAD_PAGES) when key_len == 0): when the scan stops, the pages already requested beyond that point are unused (243 prefetched / 119 accessed at 4k, 121 / 63 at 8k, 57 / 33 at 16k). The overshoot seems limited to about two windows, so I think it is minor; could the LIMIT reach this path too, or else should the test accept this overshoot?

@iMineLink iMineLink left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please find a partial review: could the branch be rebased on latest main as there are some conflicts? FWIW I suggested how to use new MDEV-33966 API in couple places.
I'm checking some other potential findings locally, I'll post them as soon as that is completed.

Comment thread storage/innobase/btr/btr0cur.cc Resolved
Comment thread storage/innobase/buf/buf0buf.cc Resolved
Comment thread sql/handler.h
void *seq_init_param,
uint n_ranges, uint *bufsz,
uint *mrr_mode, ha_rows limit,
uint *mrr_mode, page_range *pr,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an MDEV for this issue?

Comment thread storage/innobase/buf/buf0rea.cc Outdated
else
{
buf_pool.corrupted_evict(bpage, buf_page_t::READ_FIX);
space->release();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think it's better to reuse buf_read_page_low() here and in buf_read_ahead_one(), to keep "tricky" code in a single function.

@iMineLink iMineLink left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please find below the last part of my review. I see there are still some comments not closed from earlier iterations. Of these, I'd love to see buf_read_page_low() used in both places where it could. Thank you.

Comment thread storage/innobase/handler/ha_innodb.cc Resolved
Comment on lines +126 to +128
--let $ra_query= SELECT * FROM t1 ORDER BY id LIMIT 1000
--source suite/innodb/include/ra_stats.inc
SELECT @pages_prefetch < 64 AS limit1000_bounded;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On 1bb1738 (with the bound now <= 64) this still fails at 4k and 8k on every run, while 16k, 32k and 64k pass. ORDER BY id LIMIT 1000 is a full index scan, which does not go through multi_range_read_info_const(), so the LIMIT does not bound the read-ahead (index_read() calls init_readahead_window(buf_pool_t::READ_AHEAD_PAGES) when key_len == 0): when the scan stops, the pages already requested beyond that point are unused (243 prefetched / 119 accessed at 4k, 121 / 63 at 8k, 57 / 33 at 16k). The overshoot seems limited to about two windows, so I think it is minor; could the LIMIT reach this path too, or else should the test accept this overshoot?

Comment thread storage/connect/ha_connect.cc Outdated
void *seq_init_param,
uint n_ranges, uint *bufsz,
uint *flags, ha_rows limit,
uint *flags, page_range* pr,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be solved now?

@dr-m dr-m left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase this on the latest main branch (after the 365-commti merge 5d26b3b) and improve the statistics code that currently triggers -Wconversion on 32-bit targets.

Comment thread storage/innobase/dict/dict0stats.cc Outdated
Comment on lines 2024 to 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these really have to be 64-bit and does the i have to be shared among multiple for loops? The i is causing a compilation error on 32-bit systems:

/home/buildbot/x86-debian-12-fulltest-debug/build/storage/innobase/dict/dict0stats.cc: In member function ‘void {anonymous}::IndexLevelStats::sample_leaf_pages(trx_t*, uint16_t, {anonymous}::NDiffData*) const’:
/home/buildbot/x86-debian-12-fulltest-debug/build/storage/innobase/dict/dict0stats.cc:2090:59: error: conversion from ‘uint64_t’ {aka ‘long long unsigned int’} to ‘std::__debug::vector<long long unsigned int>::size_type’ {aka ‘unsigned int’} may change value [-Werror=conversion]
 2090 |                 const uint64_t  dive_below_idx = dive_idx[i];

Comment thread storage/innobase/dict/dict0stats.cc Resolved
Comment on lines 2105 to 2121
/* if the level has finished before the record we are
searching for, this means that the B-tree has changed in
the meantime, quit our sampling and use whatever stats
we have collected so far */
if (rec_idx < dive_below_idx) {

ut_ad(!btr_pcur_is_on_user_rec(&pcur));
break;
}

/* it could be that the tree has changed in such a way that
the record under dive_below_idx is the supremum record, in
this case rec_idx == dive_below_idx and pcur is positioned
on the supremum, we do not want to dive below it */
if (!btr_pcur_is_on_user_rec(&pcur)) {
break;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

page_rec_get_nth_const() would return nullptr if the argument is out of bounds. So, we’d only have to check the return value: !rec || page_rec_is_supremum(rec). Note that the argument must start from 1; 0 refers to the infimum record.

Comment thread storage/innobase/btr/btr0cur.cc Resolved
Comment thread storage/innobase/btr/btr0cur.cc Resolved
Comment thread storage/innobase/btr/btr0cur.cc Resolved
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.

buf_read_ahead_undo(): Renamed from buf_read_ahead_linear(). Now
invoked only for undo log pages, whose page numbers are
sequential. It is no longer called on BLOB pages or index pages.

buf_read_ahead_one(): Read ahead a single leaf page.

buf_read_ahead_pages(): Read ahead a set of known B-tree leaf pages.
Stops at the first page that is already resident or fails.

buf_read_ahead_random(): Split into a decision part(which extent
to read) and action part (issue the reads)

The pending read throttle now compares the
os_aio_pending_reads_approx() with buf_pool.curr_size() instead of
current pool bytes, which had made the throttle a no-op.

buf_read_ahead_pages() releases the tablespace reference
on a failed/corrupted page.

btr_ra_latch: A caller-owned buffer of leaf page numbers, which also
records the level-1 page and the child page number of the last node
pointer collected, so read-ahead can be resumed later.

btr_ra_collect(): Scan the node pointers of PAGE_LEVEL=1 page from
the 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 prefetch is precise
regardless of physical page numbers.

btr_ra_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

btr_pcur_move_to_next_page(): Prefetch the sibling after the
one being entered, with buf_read_ahead_one().

btr_pcur_move_backward_from_page(): Add a no-wait fast path that
latches the previous page if it is resident
(validating it against the current page), and prefetch
the one before it.

btr_copy_blob_prefix(): Simplified; no longer relies on
FIL_PAGE_PREV/NEXT, which are not initialized on BLOB 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().

multi_range_read_info_const() takes a page_range parameter;
the default MRR implementation aggregates the
per-range extents, and opt_range.cc carries the result through the
chosen QUICK_RANGE_SELECT, whose reset() calls advise_page_range()
before multi_range_read_init(). The extent only sizes and bounds
read-ahead; it never gates the pages the scan actually reads.
The LIMIT is converted in multi_range_read_info_const() to a
leaf count (m_ra_limit_pages).

mrr_readahead_from_scan_range() derives the read-ahead ceiling
from the advised extent, falling back to that LIMIT-based estimate.
The ceiling is capped by init_readahead_window() at 1/256 of
the buffer pool (but at least 4 pages), because every prefetched
page occupies a buffer pool frame as soon as its read is issued.

The window ramps up: a positioning read harvests at most 4 leaves
and resets the window to 4, and readahead_refill() doubles it
toward the ceiling. Single-row probes such as index_first()
for MIN()/MAX(), and every range of a multi-range read,
therefore prefetch only a few leaves while long scans reach
full depth.

search_with_readahead(): Position the cursor while collecting the leaf
pages, then call start_readahead(). It is used by
index_read() and by the first general_fetch() only when
read-ahead is armed, so the other paths carry no read-ahead locals.

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. The reads are issued after the latches are released.

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.

dict_stats_analyze_index_for_n_prefix(): Choose the records to dive
below first, prefetch their child pages in one batch with
buf_read_ahead_pages(), and then dive below the same records.

Undo/system tablespace shrinking holds fil_space_t::latch exclusively
(x_lock_space()), and buf_read_ahead_undo() holds it shared,
via the new fil_space_t::s_lock_try() that never waits,
while it checks the size and issues the reads.
If the latch is not available, the read-ahead is skipped.

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.

@dr-m dr-m left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is another incomplete review. I did not review the algorithm yet.

Comment on lines +642 to +653
/** Estimate how many leaf pages to prefetch for a
positioning read.
@param limit estimated rows to fetch,
@param keyno index to be scanned
@return leaf pages to read ahead */
uint mrr_readahead_pages(ha_rows limit, uint keyno) const;

/** Estimate the read-ahead page count from the scan's
advised leaf-page extent (m_ra_first_page, m_ra_last_page),
falling back to the LIMIT (m_ra_limit_pages).
@return leaf pages to read ahead */
uint mrr_readahead_from_scan_range() const;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are missing noexcept.

mtr.start();
mtr_s_lock_index(index, &mtr);

while (!ra.full() && l1 != FIL_NULL && !done)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we evaluate the m_ra_desc condition outside this loop? That is, duplicate the loop twice.

We do not seem to really need the variable done.

Can we use l1==0 as the special sentinel value?

Comment on lines +21401 to +21405
switch (ra_result)
{
case BTR_RA_STOP:
done= true;
break;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be no line break before the { that starts a switch body. This is the only place where the break would have to interrupt the loop as well. The break could be replaced with a goto, or the while body could be made a separate member function that could return a value indicating whether we should keep iterating.

Comment on lines +21390 to +21393
l1= m_ra_desc ? btr_page_get_prev(block->page.frame)
: btr_page_get_next(block->page.frame);
resume_child= 0;
continue;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be the only place where l1 is being assigned to. We could add the following:

if (l1 == FIL_NULL)
{
  l1= 0;
  break;
}

This would allow the surrounding loop condition to be simplified to the following:

while (!ra.full())


/* The sibling link of the last level-1 page is FIL_NULL;
the cursor uses 0 for "none" */
m_ra_l1_page= done || l1 == FIL_NULL ? 0 : l1;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the preceding code were adjusted, this could be simplified to just the following, or omitted altogether (the preceding code could modify the data member directly):

m_ra_l1_page= l1;

Comment on lines +113 to +118
/* Already read full capacity */
BTR_RA_FULL,
/* Current level page was completed; sibling may continue it */
BTR_RA_EDGE,
/* Reached stop_page */
BTR_RA_STOP

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These comments should start with /** for them to be recognised by Doxygen.

Comment thread storage/maria/ha_maria.cc
Comment on lines +4275 to +4276
uint *flags, page_range *pr,
ha_rows limit,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a double space after flags,

const rec_t *rec= btr_pcur_get_rec(&pcur);
rec_offs *offsets= rec_get_offsets(
rec, index, offsets_, 0, ULINT_UNDEFINED, &heap);
pages[n_pages++]= btr_node_ptr_get_child_page_no(rec, offsets);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please rename rec_fold() to rec_data_size(), swap its two first arguments (for a simpler call in dict_index_t::get_child_offset() below), and change its last statement from

  return my_crc32c(uint32_t(ut_fold_ull(index.id)), rec, n);

to

  return n;

The child page number will be delivered simply by the following, without any rec_get_offsets() calls:

size_t dict_index_t::get_child_offset(const rec_t *rec, uint16_t n_fields)
  const noexcept
{
  ut_ad(!(type & (DICT_FTS | DICT_CORRUPT));
  ut_ad(n_fields == dict_index_get_n_unique_in_tree_nonleaf(this));
  return table->not_redundant()
    ? rec_data_size<true>(this, rec, n_fields)
    : rec_data_size<false>(this, rec, n_fields);
}

uint32_t dict_index_t::get_child(const rec_t *rec) const noexcept
{
  ut_ad(is_btree());
  return mach_read_from_4
    (rec + get_child_offset(rec, dict_index_get_n_unique_in_tree(this)));
}

The function template rec_data_size() is best defined in the same compilation unit with both these functions. It looks like it would have to be instantiated explicitly, so that rec_fold can call it from btr0sea.cc:

template<bool comp>
static uint32_t rec_fold(const dict_index_t &index, const rec_t *rec,
                         uint32_t n_bytes_fields) noexcept
{
  return my_crc32c<comp>(uint32_t(ut_fold_ull(index.id)), rec,
                         rec_data_size<comp>(index, rec, n_bytes_fields));
}

The function dict_index_t::get_child_offset() would be needed in a revised btr_node_ptr_set_child_page_no().


uint32_t btr_ra_batch::child_page(const dict_index_t *index,
const rec_t *rec) noexcept
{

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must be dict_index_t::get_child(const rec_t* rec) const noexcept and avoid duplicating code from rec_fold(), as I noted in my other comment. We just need one way of determining a child page number.

While we are at it, all occurrences of btr_node_ptr_get_child_page_no() should be replaced and the calculation of offsets removed from btr_page_get_parent() and elsewhere.

Comment on lines +1195 to +1200
template btr_ra_result
btr_ra_batch::collect<true>(const dict_index_t *, const rec_t *,
uint32_t) noexcept;
template btr_ra_result
btr_ra_batch::collect<false>(const dict_index_t *, const rec_t *,
uint32_t) noexcept;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move all read-ahead code to a new compilation unit? This would also include the caller of btr_ra_batch. In that way, the function template may be inline to the only caller.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

7 participants