Why do we need full buffer pool flush during crash recovery?

[inaam-rana]

I want to understand the reason behind why we do full buffer pool flush (synchronously) during crash recovery (https://github.com/mysql/mysql-server/blob/845d525d49c8027a4d0cdcc43372c96ba295c857/storage/innobase/srv/srv0start.cc#L1809). There are few things to note here:

1. In canonical ARIES based recovery the process has to be idempotent. IIUC InnoDB's recovery is also idempotent
2. I tried digging into git history and this synchronous flushing seems to be included sometime in 5.7. It was not there before.
3. If we broadly divide recovery in (a) reading redo (b) parsing redo (c) applying redo (d) full buffer pool flush, on a busy system (d) can be second heaviest task after (b). Imagine a million dirty pages which means you have to flush 16G of data. On slow IO system this can be a significant hit.

The way InnoDB works, if our last checkpoint is at LSN X and then we crash at LSN Y, then during recovery once we have applied the redo records all the way upto LSN Y, we should be able to open the database for business with still LSN X as our last checkpoint LSN. True, we can have some corner cases like resizing the log files and stuff but typically i.e.: in 99.99% of the cases we should be able to avoid this full flushing penalty.

[jlopusza-oracle]

I am afraid the historical reason for that might be lost - to me it looks like some accidental remnant from much older design, probably dating back to:

commit 2208898dd92998f0b7ef5c921372ea4389594e90
Author: Marko Mäkelä <marko.makela@oracle.com>
Date:   Wed Apr 16 14:50:23 2014 +0300

    WL#7806 InnoDB: Log-based discovery of built-in tablespaces

in which this fragment seemed to be moved from a place which had this code comment:

-       /* Flush the changes made to TRX_SYS_PAGE by trx_sys_create_rsegs()*/

I can't really speak on behalf of the author, and there's not much details about the reasoning, behind it.

At any rate the current code looks way different than it looked back then.

I think I can speak more about the current situation in 26.7.0, which is that we not only write it back to disc, but also evict the pages from buffer pool.
This was done as part of WL#15372 "InnoDB: Disable IBUF merges during recovery" to avoid serious correctness problems around ibuf (aka change buffer). Before opening for public, all the pages in BP have to have buffered changes already applied. This can be achieved in two ways: apply the changes, or get rid of the pages from BP. The old approach was to apply the changes to the pages which were loaded to BP as part of the last redo recovery batch. This however had a serious issue: applying such changes generates redo logs, and redo log capacity has a hard limit. This could easily lead to deadlocks, irrecoverable state or data loss. Thus we've decided to simplify the design: simply write back and evict all the pages mentioned in the last batch, same way we always did for the N-1 previous batches.
One could imagine a design where we only do so if we detect ibuf being non-empty, or do it somehow selectively etc.
But we went for simplicity, with the main goal being correctness.
Let me add some more context for this decision:

1. There is a separate, dedicated feature for dumping and loading buffer pool, which properly configured saves the ids of the most hot pages, and then brings them back. These are taken from LRU, so in theory are closer to optimum, than "pages which were modified in the last batch", as the later ignores reads.
2. When redo log to apply is huge enough compared to BP, as to require more than one batch to apply, the size of last batch can be thought of as a random variable, which has equal chance to take up x% of BP as 100-x% of BP, and hence the amount of space left for pages is also random, and can't be relied on
3. The way InnoDB uses BP memory during recovery is that it tries to fill it up with redo log records, and reserves just a very small number of pages for real data pages - proportional to the number of io threads.
4. It is true, however, that if redo log is small enough to not fill up whole BP with deltas, the remaining part of BP can be used for page images. Thus it is true, that there are some combinations of parameters like redo log size, BP size, and access pattern, in which the new solution is indeed worse than the old performance-wise.

At any rate: if you have a real world data about the

• impact of buf_flush_sync_all_buf_pools() on Return To Operation time
• impact of evicting the pages in 26.0.7 on Return To Operation time
  Please share, as they could motivate work on a more complicated solution (such as evicting just the pages which had buffered changes).

And to answer you question directly: I am not aware of any good reason to force write back of dirty pages to disc, or to make a checkpoint, before opening for public, per se. AFAICT the InnoDB should work just fine if we start in a dirtied state (although care would have to be taken to properly sort the flush lists etc.). It seems that it is just a side effect of some other things we want to achieve (in the latest version of the code: avoiding issues with ibuf). As for the original reason for doing these flushes in ~2014, I can only guess these were done to mask some other trickery - older codebase used to modify some pages or files directly, without redo-logging, and to hide this dirty secret it had to write pages back to disc. Was that the reason in this particular case IDK.

[inaam-rana]

Thanks for the detailed reply. That clarified quite a few things. Though now I have a new set of questions/observations :)

1. A full buffer pool invalidation of buffer pool seems like adding insult to injury but I guess it is not much worse then what we currently have given that dump thread will asynchronously try to read in the right pages in the bp.

2. I totally agree that ibuf is totally scary and full of unheard of edge cases. Simpler the design the better it is. In terms of redo size limitation and redo generation the question is that when do we allow redo generation and when do we do we allow checkpointing to happen during recovery. If the two things happen at same point in code then we should, at least conceptually, not run into 'out of redo space' issue. I am just worried that ibuf recovery used to work for so many years, why do we thought it right to not apply ibuf during recovery only now.

3. Typically, BP size is much larger than redo capacity. Not uncommon to have, say, 128 or 256G BP while redo capacity of 16G. When we parse 16G, it can bloat up to, say, 32G max. In other words, a single batch redo apply is quite common. We can mimic it with sysbench using uniform distribution.

4. I am not an expert in ibuf but your suggestion to only flush (and evict) pages which have potential ibuf entries to apply is definitely worth exploring. If there is an authentic way of telling whether a page is likely subject to pending ibuf changes (even with slight false positives), then we can possibly avoid significant downtime during recovery.

[jlopusza-oracle]

Ad 1. Yes, if we look just at theoretical performance impact, then write-back + eviction in 26.7.0 is strictly worse than write-back 9.7 alone, but also not that much worse: the additional cost comes mostly from having to re-read the same pages again once they are accessed next time. But what we care more is correctness and empirical performance impact. We are also aware that the current implementation is not necessarily on the pareto-optimal frontier of all the trade-offs here, mostly because we have also traded against complexity. So, if we will see this to cause significant slowdown in real world scenarios, we might revisit this part, and try to evict more surgically

Ad 2. Some time ago InnoDB gained "Persistent Table Metadata" (PTM) which is used (infrequently) for marking indexes as corrupt, and (very frequently) for storing autoincrement value. The architecture of PTM in 9.7 is such, that it queues high-level operations (basically "UPSERT INTO ptm SET val=6 WHERE table_id=7") into the redo log which otherwise talks in very low-level language of changes to individual pages. To redo such ptm operation one has to have first reconstructed a fully operational ptm table, i.e. apply all the physical changes to its pages first, and only then try to perform all the collected UPSERTS logically. This means 9.7 can not really start checkpointing/reclaiming/deleting of old redo logs, until it finishes reading them all. In 26.7.0 we WL#15550 "InnoDB: Decouple Persistent Table Metadata from checkpointing" will get rid of the write-path of this mechanism, but still has to support the read part for seamless upgrade. Perhaps in 27.x we will be able to enable checkpointing during recovery. Note that InnoDB in 9.7 is not applying ibuf changes for first N-1 batches of redo log recovery, instead doing write-back + eviction, then in the last batch it applies ibuf changes, and doesn't evict, but (as you've found) does a write-back anyway. This behaviour in 9.7 is almost correct, except for the risk of running out of redo log space, which we fix in 26.7.0

Ad 3. Yes, I am afraid you a right that this case might be frequent. Once you have a real world scenario, we will be interested in comparison of three things: (A) the way 9.7 does it, which is only write-back, (B) the way 26.7.0 does it, which is write-back + eviction, (C) a hypothetical optimal way of doing it, which is to limit write-back and eviction only to the pages which have have ibuf changes buffered for them

Ad 4. Thanks. Yes, the thing is it is not straightforward to implement - and we wanted to start with something correct first. There are some chicken and egg problems like to know if a page has any changes buffered you need to first recover the page which stores this information etc. This isn't rocket science, but it isn't trivial neither. We've opted for safe approach in first release. There's still room for simple-yet-useful heuristics like "just check if ibuf is empty" which probably solve most of cases. We might explore this. The chance of us exploring it, depends on seeing real world data showing it to be the problem

[inaam-rana]

Thank you for detailed reply.

a) You mentioned about specific changes in 9.7 around PTM which necessitates deferring redo generation and checkpointing to a later point in recovery. My understanding (likely with some gaps) of PTM is (up to 8.4) that it is an in-memory structure backed by redo logs. Or is it backed by an internal btree of sort where btree pages need to be updated before logical recovery begins? Which specific part of PTM has changed in 9.7?

b) You said that in 9.7 ibuf changes are not applied to N - 1 batches. Has this not been the case since day one or is it something new? I thought no-ibuf-for-n-1 has been there since day one and for very valid reasons, buf-pool-invalidate-for-n-1 has been there as well from day one for the same reason and buf-pool-flush-for-n has been introduced somewhere around ten years ago and is perhaps redundant (or at least can be optimized)

c) We have added some instrumentation to measure the time for redo parsing (this necessarily includes the time to read redo as well), redo apply and buffer pool flush. I can dig into it and see what kind of time we are taking for this synchronous flushing activity.

d) ibuf_is_empty() can be a very good start. A next step can be, for example, if a page is not a secondary index leaf page then no need to evict it (ibuf is for secondary index leaf pages only...right?). Beyond that, we can even thinking of quickly scanning ibuf tree if needed at exactly the same location where we are doing sync flushing now. At that point, all redo has been applied and it feels like it should be safe to read through the ibuf btree. But, yes, with each heuristic with more accuracy it seems to be getting more non-trivial.

[jlopusza-oracle]

Hi Inaam!

Ad a) The PTM up to 9.7 including, is implemented in a rather complicated way, which consists of

• The mysql.innodb_dynamic_metadata table (which you can inspect if SET debug='+d,skip_dd_table_access_check';) which has table_id, version, metadata columns. This is almost like a regular table, except that for performance we don't bother with undo logging, trx_id field, locking etc. Crucially, though, it is stored on disc, and modified via redo-logged mtrs
• MLOG_TABLE_METADATA redo log records, which describe what UPSERTS should be made to that table (perhaps some of them were already made, some maybe not yet). Note that this is expressed in a high level language of things like "we should remember to update the autoinc value for $table_id to $value", which means that to "apply" such record, you first need to somehow locate (or create) the row/record of mysql.innodb_dynamic_metadata it talks about, which in turn implies you have to first recover the B-tree pages physically using the regular redo logs (such as MLOG_REC_INSERT etc.), up to the point where it forms a physically consistent shape of a table, which is simply missing a few UPSERTs
• and yes, there are in-memory data structures which represent the most current values of auto-increment etc.

The idea was roughly that we update the in-memory counters, and emit MLOG_TABLE_METADATA record as a memo to also do it to the mysql.innodb_dynamic_metadata table later in case we forget. From time to time (for example before doing a checkpoint) MySQL writes the state from memory to the mysql.innodb_dynamic_metadata, after which it considers that all the MLOG_TABLE_METADATA redo logs emitted "before" are no longer needed.

What will change in 26.x is that we will no longer emit MLOG_TABLE_METADATA records, because they were difficult to handle during recovery.
Instead we will proactively update info in the mysql.innodb_dynamic_metadata ahead of time (reserving some range of ids for autoinc upfront).
For compatibility with 9.7, the 26.x will still understand MLOG_TABLE_METADATA redo log records, collect them during redo log recovery, and apply them (logically) after the redo log recovery is done. Only then we can remove the old redo logs (enable checkpointing).

In future versions (say 28.x) we might get rid of this part, too.

Ad b) Yes, skipping application of ibuf changes during first N-1 batches was there since early days. I am not sure which of my sentences made you think I consider it a new behaviour. Note that any new behaviour I describe will come in 26.x, not in 9.7. The new thing will be that even in the Nth batch 26.x will not apply ibuf changes. 9.7 was applying them, which can cause a deadlock. The 26.x prefers to not deadlock during recovery, at the expense of evicting the pages.

Ad c) great, thank you

Ad d) good ideas, thank you

[inaam-rana]

Some more thoughts:

1. For flushing/eviction
   Option (b) does sound like least invasive. Though we can also have an option (d) where we leave the whole recovery code as is during apply and only work on this during the invalidation code. The idea is that during redo apply there are quite a few moving parts - io helper threads, recovery thread, recv_writer thread etc. while it is much easier to reason about the state once all redo is applied minus the ibuf changes. All we have to do is to write a version of buf_pool_invalidate() which only flushes and evicts pages with potential ibuf changes. Yes, it will involve a full bp scan but I feel like, to start with, a full scan is not too much of the cost if we end up making things simpler.

2. For flush_list sort
   Yeah, (b) to do one final sort after all processing should be the most straight forward way.

3. Other corner cases
   Do we have to worry about other corner cases like redo log resizing etc. Anything else that might get broken by us not cleaning up all redos? PTM, ddl_log etc.?

Maybe I have started thinking too much about avoiding sidestepping intrusive changes but to me it seems like the option(d) to write a newer version of buf_pool_invalidate() might be the cleanest choice. Think of it, if for any reason we want to do the full invalidation (redo size is changing or some other such reason) all we have to do is to flip to full invalidation and the rest should just work as is.

[inaam-rana]

Hi Jakub,

Is it fair to say:

• The function recv_recover_page_func is only called by IO helper threads i.e.: the main recovery thread will not end up in this function while doing standard InnoDB recovery.
• The call to buf_flush_sync_all_buf_pools is effectively a no-op given that we invalidate the buffer pool.

[jlopusza-oracle]

In mysql-trunk you can see two calls to recv_recover_page(..) the other being in

static void recv_apply_log_rec(recv_addr_t *recv_addr) {
  ut_ad(mutex_own(&recv_sys->mutex));
  ut_a(recv_addr->state != RECV_DISCARDED);

  bool found;
  const page_id_t page_id(recv_addr->space, recv_addr->page_no);

  const page_size_t page_size =
      fil_space_get_page_size(recv_addr->space, &found);
  ut_a(found);
  ut_a(!recv_sys->missing_ids.contains(recv_addr->space));
  ut_a(!recv_sys->deleted.contains(recv_addr->space));
  if (recv_addr->state == RECV_NOT_PROCESSED) {
    mutex_exit(&recv_sys->mutex);
    if (buf_page_peek(page_id)) {
      mtr_t mtr;

      mtr_start(&mtr);

      buf_block_t *block;

      block =
          buf_page_get(page_id, page_size, RW_X_LATCH, UT_LOCATION_HERE, &mtr);

      buf_block_dbg_add_level(block, SYNC_NO_ORDER_CHECK);
      /* TODO: when we start parsing a batch there's no page in BP, and we only
      add pages to BP once all deltas for a given batch are already in the
      hashmap. This can happen either during dict_boot() which reads a few pages
      before applying the last batch, or as part of applying a batch (which in
      case of last batch includes reading and applying changes from IBUF, too).
      In any case, if a page is in BP it must have been read after deltas meant
      for it were already added to the hashmap. This means io completer had to
      apply them. Therefore it makes no sense for us to try to apply anything,
      because it is guaranteed to be applied before we could get RW_X_LATCH on
      the block.

      We should simplify the code around here if below assert holds. */
#ifdef UNIV_DEBUG
      mutex_enter(&recv_sys->mutex);
      ut_a(recv_addr->state == RECV_PROCESSED);
      mutex_exit(&recv_sys->mutex);
#endif

      recv_recover_page(false, block);

      mtr_commit(&mtr);

    } else {

Our test coverage report shows 0 hits for this call, and as you can see there's some TODO comment from me about this part being probably a no-op, followed by a debug assert validating there's nothing to do here, followed by the just-in-i-am-wrong call to the function.
This will probably be removed soonish.

As for the call happening in the buf_page_io_complete(..) it could in theory happen in a synchronous read, instead of async read, and then it would be handled by the thread requesting the read. In practice, I am not aware of such synchronous call path. There could be some, but I don't recall any.

As for buf_flush_sync_all_buf_pools() - yes, it does nothing, and we no longer call it in mysql-trunk from srv_start(..). The call was moved into another place and has now this code comment:

    /* this is not really needed, as the buffer pool should be empty now */
    buf_flush_sync_all_buf_pools();

In 9.7 it is also not needed as the BP is also empty.
In 8.4 I think it is not empty, it might have some dirty pages, but still, I'd say the call isn't strictly needed for correctness.

[jlopusza-oracle]

I've psuhed "Bug#37296558 InnoDB: Evict only pages with ibuf changes after recovery after WL#15372" to origin/mysql-trunk today.

Thank you for help

The patch for "Bug#35398087 recv_writer thread is not really needed anymore" is in review.

[inaam-rana]

Kuba,

Thank you very much for this work. I believe this level of collaboration (though admittedly I didn't contribute to the patch itself) is already a big win. Looking forward to many more collaborative efforts to make InnoDB the fastest engine around!

Thanks again!
--inaam

[inaam-rana]

I am going to talk about this in contributor summit. I was told to upload the slides here.
innodb_recv_improvements.pdf

[jlopusza-oracle]

In the presentation you suggest checking if there were any redo log records applied to the page, with the reasoning which I would paraphrase (let me know if I misunderstood/misrepresent it) like this:
If there were any deltas applied to the page during recovery, the argument goes, then this page was in the BP at the moment of the crash, which we know, as clearly it was not written back to disc after the changes described in these redo logs were done to it (otherwise recovery logic would notice FIL_PAGE_LSN > delta.lsn and skip application), yet (1) InnoDB never evicts a dirty page without writing it back, and (2) never modifies pages on disc directly without having them in BP. And because it was in BP at the moment of the crash, it couldn't have ibuffered changes, because (3) innodb only buffers changes for pages which are not in BP.

Claims (1) and (2) are 99% true, and the edge-cases where they are not true (such as direct modification of system tablespace header at shutdown bypassing BP, or evictions of truncated undo space pages, or truncated temporary tables) are beyond the scope of the ibuf issue, so I grant you that :)

Claim (3) is also true.

Yet the whole reasoning seems to have one small flaw when taken together, which is the following edge case.
Consider a secondary index page which is not in the BP at t0.
Because it is missing from BP, InnoDB can ibuffer changes for it. Suppose there are at least two such changes in the ibuf for this page, and it so happens that the two buffered changes are not in the same page of the ibuf (which itself is a B-tree).
Now, suppose at t1 we try to read-in this secondary index page into BP.
The io completer logic, will notice there are ibuffered changes to it, and will apply them.
However, it does so in a loop, which you can see in ibuf_merge_or_delete_for_page(..) and that loop, takes a pause after each ibuf page boundary, to commit the current mtr, and start a fresh one, presumably in an attempt to limit the amount of latching, and the size of individual mtr. That means, that if the two buffered changes sit on two different ibuf pages, then there will be a wall-clock moment and corresponding lsn number at which the first change is already applied to the secondary index page and removed from ibuf, but the second one is not. The secondary index page is dirty, it has some changes described in the redo log, and also at least one pending change in the ibuf. If you crash at this very moment, then the recovery logic will apply the redo log changes to the secondary index page which correspond to the first ibuffered change (and also to removing this first change from ibuf) but there will be still a second one waiting for it to be applied.

If (1),(2) and (3) are true, how come the overall reasoning fails, imo? Well, I think the problem was hidden in smuggling-in a binary dihotomy "a page is in BP" vs "a page is missing BP" - turns out there's this transitional state where the io completer is still working on making it available in BP, while it is in a BP in one sense (io has read the bytes to a buffer, and io completer uses mtrs to modify it directly in BP) but it is not yet available to any other threads, so in this sense it is not yet in the BP.

I think whatever solution we will end up implementing has to take this case into account.

There's also another case, which is interesting on its own, even though it is not a counter-example for your proposed condition. It is interesting for another reason: it is possibly a more frequent scenario in which we have to apply ibuf changes to a page mentioned in a redo-log. In this 2nd scenario we have a secondary index page being modified, and written back to disc, and then while it is absent from BP, some changes for it get ibuffered, then we crash. So far so good, pretty natural. But there's one detail: the checkpoint lsn is not advanced yet past the last redo-logged modification to this page for whatever reason (checkpointer thread is lagging, page cleaners are lagging, etc.). The recovery starts reading deltas from the checkpoint_lsn, so will collect the deltas meant for the secondary page. As explained earlier it will skip them seeing the FIL_PAGE_LSN is already higher than that (due to write back of the dirty page happening before the crash) so your heuristic will work. Still, the page is being "processed" by redo log recovery logic AND has outstanding ibuf changes.
This scenario also needs to be handled somehow, and your heuristic seems to actually shine for cases like this one.

I found our internal discussion (which involved Marcin Babij) from 2024 about the same topic, and there we've considered several options:

a) in the last batch have a logic to write and evict only the pages which had any ibuffered changes on them. This sounds like a new custom and complicated logic for flushing, I'd be against adding it to this WL, as it looks like additional code to review, maintain, prove correctness, and remember about. Also, honestly I am not sure where and how to store the info that a given page had/didn't have ibuffered changes
b) Marcin had this idea that as soon as a page is read during recovery, we immediately write it back and evict without anyone asking for it. (That idea also solves other issues we have with LRU flushing and eviction not working well during recovery leading to very slow acquisition of free pages). That idea could be extended to evict only conditionally on the page having ibuffered changes or being in last batch. That, too I am afraid is too complicated, and better to deal separately in thoughtful way
c) a much simpler approach could be all or nothing: once the redo log recovery is done with applying (but before doing the final flush) check if the ibuf B-tree is empty. If it is, then it means we don't have to evict anything. Otherwise do the eviction. I think it strikes the right balance between simplicity, ease of verifying correctness and handling 90% of real world customer cases: those which use IBUF a lot will benefit from recovery being faster as we don't apply changes, and those who don't use it at all, will have the old behaviour. Only the customers who "use IBUF but not too much" will suffer 10%
d) In the buf_page_io_complete, in the place which currently in trunk calls ibuf_merge_or_delete_for_page, instead add a call to new function:

   page_in_bp_needs_ibuf_merge=true;
}```
Just before the processing of last batch, set the `page_in_bp_needs_ibuf_merge=false;`, and evict *all* pages from BP if the flag is true after applying changes from redo log. The body of this new function should be inspired by `ibuf_merge_or_delete_for_page` so something like
```bool ibuf_has_changes_buffered(page_id_t page_id,  const page_size_t &page_size){
  if( trx_sys_hdr_page(page_id) || fsp_is_system_temporary(page_id.space()) || ibuf_fixed_addr_page(page_id, page_size) ||
      fsp_descr_page(page_id, page_size)) {
     return false;
  }
  auto space = fil_space_acquire_silent(page_id.space());
  if(space == nullptr){
     return false;
  }
  ibuf_mtr_start(&mtr);
  const auto bitmap_bits = ibuf_bitmap_page_get_bits(ibuf_bitmap_get_map_page(page_id, page_size, UT_LOCATION_HERE, &mtr), page_id, page_size,
                                          IBUF_BITMAP_BUFFERED, &mtr);

  ibuf_mtr_commit(&mtr);  
  fil_space_release(space);
  return bitmap_bits;
}```
and later:
I think the safest way to accomplish (d) is to first restore a global flag, but instead of naming it `recv_no_ibuf_operations` name it `bool recv_is_last_batch=false`.
Set it to `true` before the final call to `recv_apply_hashed_log_recs()` which eats the suffix.
Add a new flag `bool recv_page_in_bp_needs_merge = false;`
Then make the call to `buf_pool_invalidate()` conditional:
```if(!recv_is_last_batch || recv_page_in_bp_needs_merge) {
  buf_pool_invalidate();
}```
and in the `buf_page_io_complete` add the following:
```    if (recv_recovery_is_on()) {
      /* Pages must be uncompressed for crash recovery. */
      ut_a(uncompressed);
+     if(recv_is_last_batch && !recv_page_in_bp_needs_merge && ibuf_has_changes_buffered(page->id, page->size)){
+         recv_page_in_bp_needs_merge=true;
+     }
      recv_recover_page(true, (buf_block_t *)bpage);
    } else if (uncompressed && !Compression::is_compressed_page(frame) &&
               fil_page_get_type(frame) == FIL_PAGE_INDEX &&
               page_is_leaf(frame) &&
               !fsp_is_system_temporary(bpage->id.space()) &&
               !fsp_is_undo_tablespace(bpage->id.space()) &&
               !bpage->was_stale()) {
      ibuf_merge_or_delete_for_page((buf_block_t *)bpage, bpage->id,
                                    &bpage->size, true);
    }```
</QUOTE>

Note, I do not necessarily share the views from 2 years ago now :) I am just sharing this as food for thought.
What I think now in 2026 is more like:
* I think that checking if ibuf_is_empty() will probably not move the needle for people who use ibuf, because presumably if they really *use* it then it is not empty. What is however quite probable even if they use it heavily is that the intersection of its content with the pages mentioned in redo log is not that big (although this is difficult to judge really, kinda depends on temporal access patterns as one way it could happen is when pages are evicted due to LRU policy in the order which is wildly different than the flush-list (oldest-modification) order, which seems to me to be necessary to arrive at a situation where deltas above checkpoint_lsn are somehow meant for pages which were not in the BP at the moment of the crash)
* I think it checking the ibuf bitmap to see if there are any change for a particular page is the most correct/precise thing to do. But for this to be correct/precise we need to make sure that the ibuf bitmap page itself is fully recovered, which in turn means such checks are only correct during the last batch. Thankfully, we only really are interested in the answer in the last batch, so everything fits. Also, I believe the cost of looking up the bit in the ibuf bitmap is negligible - after all InnoDB already pays this price at run time for each read, so people already bear with it.
* Thus I think one promising direction is to have io-completer check if the last batch of recovery and if so check the ibuf bitmap and if so "do something about it"
* what to do about it I am not sure. I am open to all options: 
     a) write-back and evict (basically: single-page-flush) the page right after applying deltas to it
     b) somehow "mark it" for later, so that all such pages can be written back and evicted in a more batched manner
     c) simply remember that any such situation happened at all, and later write-back and evict everything without going into details

GIven that any solution to this whole issue will have to involve restoring the flush_list order, which in turn might require iterating over everything in the BP *and* that some of the pages which io completer sees might not survive in the BP till the end of recovery due to memory pressure and page cleaners background activity, I think solution (a) is not as tempting as (b) or (c).

But then, how about the following approach, perhaps the simplest of all:
After the redo log recovery finishes, do a full scan over BP, which does everything:

with locked LRU:
for(page in BP->lru) {
all_ids.push(page.id)
}
// this makes the ibuf bitmap accesses more localized
sort(all_ids)
for(id in all_ids){
if(has_buffered_changes(id)){
if (page=buf_get_page(id)){
buf_flush_page(buf_pool, page, BUF_FLUSH_SINGLE_PAGE, true)
}
}
}
with locked flush list
for( page in BP->flush_list) {
dirty_pages.push(&page)
}
sort(dirty_pages, by oldest_modification)
BP->flush_list = dirty_pages

Perhaps this could be done in parallel for all BP instances.

[inaam-rana]

Jakub,

You are totally right. I missed ibuf-apply-is-non-atomic-and-generates-redo part. I only thought about the second case where changes are flushed but checkpoint hasn't moved forward or crash recovery crashing and restarting. The hole you mentioned is real and the heuristic is broken. Minus that, the rest of it is fairly static, a leaf page of secondary index. I think even though this will have more false positives it will still filter out clustered index pages which should be majority of buffer pool.

You can see that I am reluctant to read ibuf bitmap page. There is really no technical reason for that. I just feel like adding another IO dependency with the whole thing in flux (I understand we do a read for dict_boot()) during redo apply just seems a little scary. If you are confident about it, I am ok to go ahead with it.

What I had in mind was something like this (this is without bitmap page read):

• From recy_sys have a set of page_id (actually an array with size equal to n_buf_pool_instances)
• In recv_recover_page: if heuristic is met (page is a leaf page of secondary index), store the id in relevant set under recv_sys::mutex
• If it is in n-1 batches clear the set (basically throw it away)
• If it is last batch
  • wait for LRU flush to end
  • spawn n_buf_pool_instances thread where each thread
    • sort the flush list (may be using std::vector)
    • prepare/dispatch flush batch from the specific bp instance (accessing pages through page_hash)
    • wait for flush batch to end
    • evict pages from LRU list (at this point we can assert that all of them are clean)
    • in debug builds run some sanity check like iterating over flush_list to see no pages from our set exist etc.
    • done
  • carry on with regular recovery

I guess if we feel confident on reading bitmap page from IO helper thread during recovery last batch then we can replace the heuristics with authentic source of truth.

[jlopusza-oracle]

A few more thoughts from a walk:

• BUF_FLUSH_SINGLE_PAGE in itself is "problematic legacy" and there are some hand-wavy plans to get rid of it, mostly due to performance impact of it. So implementation plan which depends on using it is a bit at odds with overall direction
• OTOH, the discussion so far suggests, that majority of pages which have ibuffered changes at the end of redo log recovery AND were even mentioned in the redo log at all, fall into the category "the applier had nothing to apply really", so they are already clean. In such case we don't have to (perhaps even shouldn't) try to write it back to disc, but simply evict it right away (see code in buf_flush_single_page_from_LRU() for inspiration).
• The only case I could think of where the page is dirty AND has ibuffered changes, is the one edge case I've presented above, so must be very rare as it only happens in the middle of io completion logic, which means at most as many pages are in this state as there are io completion threads (typically one per BP instance) + whatever is the number of synchronous reads into BP (that is probably limited by the number of client sessions), that any performance impact of BUF_FLUSH_SINGLE_PAGE doesn't matter
• that said, the buf_flush_page(..BUF_FLUSH_SINGLE_PAGE..) in theory can return false when it was unable to do the flush for whatever reason, even such weak reason as being unable to acquire a sx-latch without waiting or seeing a positive io-fix count. My intuition is that neither of these cases is possible before opening for public, but retry-loop certainly makes code even more complicated and its lack - more shaky.
• during redo recovery all the flushing happens in a rather turbo-overcomplicated way, where there's a dedicated recv_writer_thread() which doesn't do anything beyond pinging the buf_flush_page_coordinator_thread() every 100ms to perform BUF_FLUSH_LRU, which the coordinator thread then passes on to all the helper page cleaners. At the end of each batch (well, that depends on MySQL version) we instruct the flush coordinator to do BUF_FLUSH_LIST instead, but then we anyway evict anything, which removes the only distinction between BUF_FLUSH_LIST and BUF_FLUSH_LRU so not clear why we not simply always use BUF_FLUSH_LRU (probably: because it might not evict some pages when there is no memory pressure to do so). Where I am going with this is that perhaps this whole area has to be redesigned anyway, as suggestged in Bug#35398087 recv_writer thread is not really needed anymore, so maybe we could arrange it so that if this is the last batch, then instead of doing BUF_FLUSH_LIST, we would do something like BUF_FLUSH_LRU but limited to pages which have ibuffered changes only. The benefit could be that instead of doing single-page flushes with ad hoc logic, we could benefit from already implemented batched, multithreaded implementation in page cleaners
• that said, those page cleaners are also awaiting redesign, to separate LRU evictions, flushes and flush_list flushes, as each of them has different demand, cost, etc.
• another idea is to somehow leverage existing page cleaner logic, by artificially sorting LRU list so that the pages which have ibuffered changes are first to evict, and then request BUF_FLUSH_LRU until all of them are gone. This looks hackish to me, but if it can be done in few lines and win us batching and simplicity, I'd consider it worth exploring
• yet another idea is to not really evict anything, but rather apply the ibuf changes, kinda like in the old days with one difference: unpause the page cleaners and log checkpointer first. I.e. the sequence would be: finish the redo log recovery, sort the flush_lists, let the page cleaners and log checkpointer start normal operation, now scan the bp to find all pages which require ibuf merges, and do them, only once this is done, open for public. Not obvious a priori how this approach compares to others in terms of IO and duration

[jlopusza-oracle]

The following is a PoC of what I have in mind. It's based on 26.x but I think it is easy to backport.

diff --git a/storage/innobase/fil/fil0innodb_pages_persistence.cc b/storage/innobase/fil/fil0innodb_pages_persistence.cc
index 4f55d7ef0395..6f54d01ad9e4 100644
--- a/storage/innobase/fil/fil0innodb_pages_persistence.cc
+++ b/storage/innobase/fil/fil0innodb_pages_persistence.cc
@@ -230,12 +230,6 @@ Pages_persistence::recover_pages(Lsn &flushed_lsn) {
 
   ut_a(!recv_sys->found_corrupt_log);
 
-  if (!srv_force_recovery && !srv_read_only_mode) {
-    ut_ad(buf_are_flush_lists_empty_validate());
-    /* this is not really needed, as the buffer pool should be empty now */
-    buf_flush_sync_all_buf_pools();
-  }
-
   srv_recovery_crash(3);
   const auto recovered_lsn = recv_sys->recovered_lsn;
 
diff --git a/storage/innobase/ibuf/ibuf0ibuf.cc b/storage/innobase/ibuf/ibuf0ibuf.cc
index e6b8e2e5350e..c581884c97e1 100644
--- a/storage/innobase/ibuf/ibuf0ibuf.cc
+++ b/storage/innobase/ibuf/ibuf0ibuf.cc
@@ -4353,6 +4353,42 @@ bool ibuf_is_empty(void) {
   return (is_empty);
 }
 
+void ibuf_evict_if_has_buffered_changes(const page_id_t &page_id) {
+  if (trx_sys_hdr_page(page_id) ||
+      fsp_is_system_temporary(page_id.space())) {
+    return;
+  }
+
+  fil_space_t *space = fil_space_acquire_silent(page_id.space());
+  if (space == nullptr) {
+    return;
+  }
+
+  const page_size_t page_size(space->flags);
+  if (ibuf_fixed_addr_page(page_id, page_size) ||
+      fsp_descr_page(page_id, page_size)) {
+    fil_space_release(space);
+    return;
+  }
+
+  mtr_t mtr;
+  mtr_start(&mtr);
+
+  const page_t *bitmap_page =
+      ibuf_bitmap_get_map_page(page_id, page_size, UT_LOCATION_HERE, &mtr);
+  const bool buffered =
+      ibuf_bitmap_page_get_bits(bitmap_page, page_id, page_size,
+                                IBUF_BITMAP_BUFFERED, &mtr) != 0;
+
+  mtr_commit(&mtr);
+
+  if (buffered) {
+    buf_page_force_evict(page_id, page_size, true);
+  }
+
+  fil_space_release(space);
+}
+
 /** Prints info of ibuf. */
 void ibuf_print(FILE *file) /*!< in: file where to print */
 {
diff --git a/storage/innobase/include/ibuf0ibuf.h b/storage/innobase/include/ibuf0ibuf.h
index fe8ebf2e7e87..5fa6562fd7f5 100644
--- a/storage/innobase/include/ibuf0ibuf.h
+++ b/storage/innobase/include/ibuf0ibuf.h
@@ -313,6 +313,11 @@ ulint ibuf_count_get(const page_id_t &page_id);
 /** Looks if the insert buffer is empty.
  @return true if empty */
 bool ibuf_is_empty(void);
+
+/** Evicts a page if the change buffer bitmap records buffered changes for it.
+The caller must not own any page latches or buffer pool mutexes.
+@param[in] page_id page identifier */
+void ibuf_evict_if_has_buffered_changes(const page_id_t &page_id);
 /** Prints info of ibuf. */
 void ibuf_print(FILE *file); /*!< in: file where to print */
 
diff --git a/storage/innobase/log/log0recv.cc b/storage/innobase/log/log0recv.cc
index fee5444b8fc8..31252cb9df30 100644
--- a/storage/innobase/log/log0recv.cc
+++ b/storage/innobase/log/log0recv.cc
@@ -38,6 +38,7 @@ this program; if not, write to the Free Software Foundation, Inc.,
 #include <sys/types.h>
 #include <ostream>
 
+#include <algorithm>
 #include <array>
 #include <iomanip>
 #include <map>
@@ -856,9 +857,67 @@ static void recv_apply_log_rec(recv_addr_t *recv_addr) {
   }
 }
 
+/** Write back and evict pages in a buffer pool instance for which the change
+buffer bitmap records buffered changes. Page identifiers are copied while the
+LRU mutex is held. Bitmap page reads and page eviction are deliberately done
+after releasing it, because both operations acquire page latches and may
+themselves modify the LRU list.
+@param[in,out] buf_pool buffer pool instance */
+static void recv_evict_pages_with_ibuffered_changes(buf_pool_t *buf_pool) {
+  std::vector<page_id_t> page_ids;
+
+  mutex_enter(&buf_pool->LRU_list_mutex);
+  page_ids.reserve(UT_LIST_GET_LEN(buf_pool->LRU));
+  for (const buf_page_t *page : buf_pool->LRU) {
+    page_ids.push_back(page->id);
+  }
+  mutex_exit(&buf_pool->LRU_list_mutex);
+
+  std::sort(page_ids.begin(), page_ids.end());
+
+  for (const page_id_t &page_id : page_ids) {
+    ibuf_evict_if_has_buffered_changes(page_id);
+  }
+}
+
+/** Restore the flush list order after crash recovery. During recovery dirty
+pages are intentionally inserted without maintaining the normal LSN ordering.
+The list runs from the newest oldest_modification at the head to the oldest at
+the tail, which is where flush-list batches and checkpoint discovery start.
+@param[in,out] buf_pool buffer pool instance */
+static void recv_sort_flush_list(buf_pool_t *buf_pool) {
+  std::vector<buf_page_t *> dirty_pages;
+
+  buf_flush_list_mutex_enter(buf_pool);
+  dirty_pages.reserve(UT_LIST_GET_LEN(buf_pool->flush_list));
+  for (buf_page_t *page : buf_pool->flush_list) {
+    dirty_pages.push_back(page);
+  }
+
+  std::sort(dirty_pages.begin(), dirty_pages.end(),
+            [](const buf_page_t *lhs, const buf_page_t *rhs) {
+              return lhs->get_oldest_lsn() > rhs->get_oldest_lsn();
+            });
+
+  /* writer_mutex and the waits in recv_apply_hashed_log_recs() guarantee
+  that no flush-list batch is active. oldest_hp can retain an entry from an
+  earlier checkpoint query, so reset it before changing any links. */
+  ut_ad(buf_flush_list_mutex_own(buf_pool));
+  ut_a(buf_pool->flush_hp.get() == nullptr);
+  buf_pool->oldest_hp.set(nullptr);
+
+  buf_pool->flush_list.clear();
+  for (buf_page_t *page : dirty_pages) {
+    UT_LIST_ADD_LAST(buf_pool->flush_list, page);
+  }
+
+  buf_flush_list_mutex_exit(buf_pool);
+}
+
 /** Empties the hash table of stored log records, applying them to appropriate
-pages. */
-static void recv_apply_hashed_log_recs() {
+pages.
+@param[in] last_batch whether this is the final batch of crash recovery */
+static void recv_apply_hashed_log_recs(bool last_batch) {
   ut_ad(!recv_sys->found_corrupt_log);
   mutex_enter(&recv_sys->mutex);
   ut_a(!srv_read_only_mode);
@@ -946,7 +1005,7 @@ static void recv_apply_hashed_log_recs() {
   mutex_enter(&recv_sys->mutex);
   ut_a_eq(recv_sys->n_pages_to_recover.value(), 0);
 
-  /* Flush all the file pages to disk and invalidate them in the buffer pool */
+  /* Quiesce background flushing before post-processing the recovered pages. */
   log_background_threads_inactive_validate();
   ut_a(recv_sys->flush_end != nullptr);
 
@@ -962,28 +1021,31 @@ static void recv_apply_hashed_log_recs() {
   TBD: why is it important to wait for BUF_FLUSH_LRU to finish here? */
   buf_flush_await_no_flushing(nullptr, BUF_FLUSH_LRU);
 
-  os_event_reset(recv_sys->flush_end);
-
-  /* We are about to request BUF_FLUSH_LIST, in hope to write all dirty pages
-  back to disc, so that we can then invalidate the BP, before next batch.
-  However, buf_flush_page_and_try_neighbors() skips over io-fixed pages, so
-  they would be left in BP even if dirty. We awaited for
-  recv_sys->n_pages_to_recover to drop to zero, but this happens before io
-  completer releases the latch and io-fix from the block.
-  Therefore we wait for all read operations to finish here by using a method
-  which looks at a counter which is decremented only after io completer
-  io-unfixes the block. Also, this is important for the subsequent
-  buf_pool_invalidate() that internally uses buf_LRU_scan_and_free_block()
-  which has the same issue: skips over io-fixed pages. */
+  /* recv_sys->n_pages_to_recover reaches zero before the I/O completer releases
+  the latch and I/O fix from the last block. Both page eviction in the final
+  batch and full buffer-pool invalidation in earlier batches require all read
+  operations to have completed. */
   buf_pool_wait_for_no_pending_io();
 
-  recv_sys->flush_type = BUF_FLUSH_LIST;
+  if (last_batch) {
+    for (ulong i = 0; i < srv_buf_pool_instances; ++i) {
+      recv_evict_pages_with_ibuffered_changes(buf_pool_from_array(i));
+    }
 
-  os_event_set(recv_sys->flush_start);
+    for (ulong i = 0; i < srv_buf_pool_instances; ++i) {
+      recv_sort_flush_list(buf_pool_from_array(i));
+    }
+  } else {
+    os_event_reset(recv_sys->flush_end);
 
-  os_event_wait(recv_sys->flush_end);
+    recv_sys->flush_type = BUF_FLUSH_LIST;
 
-  buf_pool_invalidate();
+    os_event_set(recv_sys->flush_start);
+
+    os_event_wait(recv_sys->flush_end);
+
+    buf_pool_invalidate();
+  }
 
   /* Allow batches from recv_writer thread. */
   mutex_exit(&recv_sys->writer_mutex);
@@ -2023,7 +2085,7 @@ strict ordering.
 
 #ifndef UNIV_HOTBACKUP
     if (recv_heap_used() > mem_limit) {
-      recv_apply_hashed_log_recs();
+      recv_apply_hashed_log_recs(false);
     }
 #endif /* !UNIV_HOTBACKUP */
   }
@@ -2268,7 +2330,7 @@ static
       ut_a_eq(recv_sys->n_pages_to_recover.value(), 0);
       ut_a(recv_sys->spaces->empty());
     } else if (log_test == nullptr) {
-      recv_apply_hashed_log_recs();
+      recv_apply_hashed_log_recs(true);
     }
   }
 

There are ways to improve upon it, for sure:

• we could do the scans in multiple-threads, one per BP. The complication here is that the bitmap page for a secondary index page could be in a different BP instance/shard, which means that there might be some cross-talk. Thankfully we do not modify the bitmap pages, so they will not be added to flush list and thus will not cause much trouble, so perhaps we don't even need any synchronization between threads nor between the two phases (say, in a form of join() all threads before starting phase 2). But I am not 100% sure.
• we could filter down the list of pages to check - Inaam in his presentation proposed to check if they are index leaf pages. As we already have a pointer to bpage inside the loop it is very tempting to do a dirty read (nobody should be modifying the pages at the moment) but the proper way to do it is to take s-latch for each of them. I am not 100% sure this is a big speedup overall - intuitively 99% of the pages are leaf pages due to the huge fanout of internal nodes. What would be more interesting is to somehow filter out clusted index pages. This is a bit more difficult though - a page header knows the "index id", but doesn't know (AFAICT) if it corresponds to secondary or primary index. This then means we would have to separately lookup info aboud index id. Again not sure if this speeds up anything much
• the part which does evictions, could probably be way more multithreaded, by assigning chunks of the array to various threads. Again unclear if that helps

Anyway, I'd appreciate testing this patch/idea on real world data

[jlopusza-oracle]

I've submitted to code review fixes for first two issues

[inaam-rana]

Kuba,

This is very very impactful. Many many thanks for digging into this and benchmarking.
As always some comments/suggestions/how-about-if-we etc.

• Surprising to see flush taking more than 50% of the recovery time in first line. But then life is full of surprises.
• I don't have access to recv_writer bug but one behaviour I saw in the code is that it triggers LRU flushing which itself is throttled by innodb_lru_scan_depth i.e.: as long as we have innodb_lru_scan_depth free pages available, recv_writers signals to page_cleaner likely result in no-op.
• Not clear to me why we are sorting in recv_evict_pages_with_ibuffered_changes().
• When I suggested using max_trx_id I was thinking more in terms of using it alongwith recv_evict_pages_with_ibuffered_changes() which remains the definitive source of truth. If we think in terms of bigger buffer pools then maybe calling recv_evict_pages_with_ibuffered_changes() for each page can become an issue. Imagine a 320GB buffer pool i.e.: 20 million pages. If 20% pages are dirty that means on recovery we will have to read in 4 million pages. 4 million mtrs accessing the bitmap page, even if in the buffer pool, can become a drag. But may be it is not such a big deal if we do it in parallel.
• One suggestion for your patch is maybe to bypass this new optimization if we see the redo size is being reduced.

[inaam-rana]

It looks like you already tested using max_trx_id as a filter i.e.: calling recv_evict_pages_with_ibuffered_changes() selectively. It didn't show much improvement. I guess then we can put to rest this micro optimization. The simpler the better.

[jlopusza-oracle]

Hi Inaam,

Surprising to see flush taking more than 50% of the recovery time in first line. But then life is full of surprises.

Yeah, that is a bit strange, but the way I am explaining it to myself, is that with 100MB/s IO budget, we are mostly constrained by IO throughput, not by CPU, so the first part of recovery where we read redo log (8GB) and pages mentioned in it (earlier I wrote it is 22GB, which would fit the "44GB" in flush writes column, but now I see that the data/sysbench directory has 26GB so I am not entirely sure what happens, perhaps the 8GB of logs somehow avoid talking about many of the pages, which I should perhaps expect based on ibuf being non-empty), and then the later part has to write back all the pages to which any delta was applied "twice" due to double-write buffering. So we have 8+X vs 2*X. Given that X is probably 22, we have something like 30GB read and 44GB write, and 44/30 * 357 is 523, so roughly in the same ballpark as the 501 we see in that column. Also it is kinda in the ballpark of "amount of data / 100MB/s + some inefficiency".
What I am trying to say is: I am not alarmed by this.

I don't have access to recv_writer bug but one behaviour I saw in the code is that it triggers LRU flushing which itself is throttled by innodb_lru_scan_depth i.e.: as long as we have innodb_lru_scan_depth free pages available, recv_writers signals to page_cleaner likely result in no-op.

There are several things wrong there, but one of them is that LRU flushing during first N-1 batches of recovery does nothing, due to contradictory set of requirements: we leave only 256 blocks in BP for pages (rest is consumed by delta hashmap), and we also require LRU to have at least 256 pages. This results in a lot of single page flushing. The other problem is, that even if you fixed that, then the flush coordinator orders new LRU flush at 10Hz, and each of them could free at most all the 256 pages, which gives just 40MB/s, which is way too low to saturate even this slow drive, not to mention modern hardware.

Not clear to me why we are sorting in recv_evict_pages_with_ibuffered_changes().
Perhaps this optimization is not needed (also I believe it doesn't cost us much?) but the idea behind it was that a single bitmap page talks about 16K pages which have similar page_id (same page_no/16k), so if we sort page_ids then naturally pages for which the bit is stored on the same bitmap page will be processed one after another, which should improve locality, along whole stack from io, through bp, and cpu caches.

When I suggested using max_trx_id I was thinking more in terms of using it alongwith recv_evict_pages_with_ibuffered_changes() which remains the definitive source of truth. If we think in terms of bigger buffer pools then maybe calling recv_evict_pages_with_ibuffered_changes() for each page can become an issue. Imagine a 320GB buffer pool i.e.: 20 million pages. If 20% pages are dirty that means on recovery we will have to read in 4 million pages. 4 million mtrs accessing the bitmap page, even if in the buffer pool, can become a drag. But may be it is not such a big deal if we do it in parallel.

If 20% pages are dirty, then I think it is safe to say you will have to read all the bitmap pages, as probability that none of 16K of the bits in a bitmap is needed is close to zero. We have one bitmap page for each 16K*16KB of data, so for 320GB database it is 1280 bitmap pages to read in.
Thanks to sorting we will process them one by one, and so they should be hot in BP, and hopefully also in CPU caches.
One thing to note here, is that paradoxically (I'd need to measure it) it might turn out that checking max_trx_id or index level or any other data within the actual data page, might lead to CPU cache misses and even bigger effort to s-latch/unlatch the page buffer (although, I think we could afford doing dirty reads here), because there are, as you've said 4 mln such pages to inspect, and each a CPU cache miss.
Anyway, I think this idea can be easily added later, if need arises. For now, as you can see, I've struggled to get a case where the scan takes longer than 0.5 second, so blindly extrapolating from this 50GB BP to 320GP BP means something less than 4 seconds.
I posit, that 4s is negligible compared to duration of all the other things like reading in the 320GB to BP in the first place.

One suggestion for your patch is maybe to bypass this new optimization if we see the redo size is being reduced.

Can you elaborate on this please? Is it because in such case we expect to need to write-back everything anyway?

[inaam-rana]

Kuba,

I agree that no need to do micro optimization of max_trx_id now. It can easily be added later if needed.
And, yes, the idea of doing full flush and eviction if redo capacity is reduced is to avoid complexity. In new scheme of things our checkpoint age will be what it was at time of crash. If redo capacity is reduced then we might find ourselves in a tight spot in terms of available extra space in redo log etc. It seems things will still work fine but I was thinking, given redo size changes are rare, why even bother with selective flushing.