MySQL Bugs: #108731: just need to maintain FIL_PAGE

Bug #108731	just need to maintain FIL_PAGE_LSN once during recvory
Submitted:	11 Oct 2022 5:07	Modified:	19 Dec 2023 1:33
Reporter:	alex xing (OCA)	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: InnoDB storage engine	Severity:	S5 (Performance)
Version:	8.0.30	OS:	Any
Assigned to:		CPU Architecture:	Any
Tags:	Contribution

Description:
just need to maintain FIL_PAGE_LSN once during recvory in recv_recover_page_func
Since this function only apply for a particular page, only need to maintain the FIL_PAGE_LSN at the end of the apply phase.

How to repeat:
just read the code

Suggested fix:
maintain FIL_PAGE_LSN once just as the below patch

a simple patch to describe the optimization (*) I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it.

Contribution: optimize.patch (text/plain), 1.42 KiB.

Hello Alex Xing,

Thank you for the report and contribution.

regards,
umesh

BTW, I believe:
```
end_lsn = recv->start_lsn + recv->len;
```
is a Bug, because it does not account for header and footer of redo log blocks.

It is also a bug, because it adds `recv->len` which is the length of the record, to `recv->start_lsn` with is start of the mtr (which can have many records).

It is also wrong for another reason: it seems to express an idea that each redo log record has a range of lsns, and thus we should be thinking of applying a record to a single page as the unit of operation, and mark our progress in terms of records. But this is conceptually wrong, as the atomic unit of operation is an mtr, not a single record. If an MTR does several modifications to several pages, they all "happen at once" and should all result in the same end_lsn - the one at which mtr ends. This is for example how we compute latest modification lsn during mtr commit at run time.
This is also how we compute recv->start_lsn and recv->end_lsn fields in `recv_add_to_hash_table` (see comments for start_lsn and end_lsn arguments).

There's however another bug in `recv_multi_rec` which unnecessarily bumps the `old_lsn` and `recv_sys->recovered_lsn` as it iterates over individual records of a multi-record mtr, before passing `old_lsn` to `recv_add_to_hash_table` - this bug causes the `start_lsn` to be more like "start of the record", despite what documentation says, while `end_lsn` is still the mtr's (not record's) end. 

This is messy, but seems to work, because in practice there's no big deal if we write a wrong lsn (even one pointing into block trailer, or middle a of mtr) to the page's header - in case will not crash, nobody cares, and in case we crash again the recovery will simply redo some work. I think a problematic case would be if a single mtr modified the same page several times, and we somehow written the page back to disc in the middle of applying that changes, but we never do something like that, precisely because an mtr is the unit of atomicity.

The proposed patch suggests to use `end_lsn` but also to remove `end_lsn` declaration and computation, so I am not sure how it compiles, but if we are to change this piece of logic, then I'd propose to get rid of all computations, and simply use the fact that `recv_t` struct already has `end_lsn` field:
```
/** Stored log record struct */
struct recv_t {
  using Node = UT_LIST_NODE_T(recv_t);

  /** Log record type */
  mlog_id_t type;

  /** Log record body length in bytes */
  ulint len;

  /** Chain of blocks containing the log record body */
  recv_data_t *data;

  /** Start lsn of the log segment written by the mtr which generated
  this log record: NOTE that this is not necessarily the start lsn of
  this log record */
  lsn_t start_lsn;

  /** End lsn of the log segment written by the mtr which generated
  this log record: NOTE that this is not necessarily the end LSN of
  this log record */
  lsn_t end_lsn;

  /** List node, list anchored in recv_addr_t */
  Node rec_list;
};
```

I would also propose to rename the fields of this struct to avoid such confusion in future.
Let it be:
len          ---> record_len
data         ---> record_data
start_lsn    ---> mtr_start_lsn
end_lsn      ---> mtr_end_lsn

I think that seeing `recv->mtr_start_lsn + recv->record_len` would give the author or reviewer a pause.

> The proposed patch suggests to use `end_lsn` but also to remove `end_lsn` declaration and computation, so I am not sure how it compiles

Ah..turns out there are two local variables named `end_lsn` declaredin `recv_recover_page_func(..)`.
First is computed correctly:
```
#ifndef UNIV_HOTBACKUP
  lsn_t end_lsn = 0;
#endif /* !UNIV_HOTBACKUP */
...
  for (auto recv : recv_addr->rec_list) {
#ifndef UNIV_HOTBACKUP
    end_lsn = recv->end_lsn;
...
```

the other is not:

```
      lsn_t end_lsn;
...
      end_lsn = recv->start_lsn + recv->len;
```

I still claim we should use the first one.

Posted by developer:
 
Fixed as of the upcoming MySQL Server 8.3.0 release, and here's the proposed changelog entry from the documentation team:

---
Removed redundant code related to handling FIL_PAGE_LSN during recovery.

Our thanks to Alexi Xing for the contribution.
---

Thank you for the bug report.

Hi Philip Olson,
  I'm honored that my ideas have been recognized, but it's important to note that my name is Alex Xing, not Alexi Xing. Could you please correct it in the release note?