Bug #117259 Bugfix of Bug#37318367 in 8.0.41 contains an obvious logical issue

• Submitted: 22 Jan 7:17
• Modified: 30 Jan 1:03
• Reporter: Yichang SONG (OCA)
• Status: Closed
• Category: MySQL Server: InnoDB storage engine
• Severity: S2 (Serious)
• Version: 8.0.41
• OS: Any
• CPU Architecture: Any

[22 Jan 7:17] Yichang SONG

Description:
The bugfix f682827fe91bf2b61ca91b26da20dddf50f756ef
```
Bug#37318367 Inplace ALTER on table with spatial idx might cause lost rows if concurrent purge
```

add a new restore API restore_to_first_unprocessed().

For function btr_pcur_t::is_after_last_on_page() and btr_pcur_t::move_to_next_on_page(),the assertion 
```
ut_ad(m_pos_state == BTR_PCUR_IS_POSITIONED) 
```
requires the m_pos_state to be BTR_PCUR_IS_POSITIONED.

At storage/innobase/row/row0pread.cc:446, the code attempts to execute operations (is_after_last_on_page() and move_to_next_on_page()) that are strictly permitted only in BTR_PCUR_IS_POSITIONED state, while the cursor is actually in BTR_PCUR_IS_POSITIONED_OPTIMISTIC state. This violates will trigger assertion failures in debug builds.

How to repeat:
This logical issue is visible even to the naked eye.

[22 Jan 7:51] MySQL Verification Team

Hello Yichang Song,

Thank you for the report and feedback.

regards,
Umesh

[25 Jan 0:49] Rahul Sisondia

Hello Yichang Song,

Indeed, it was my oversight, apologies for that. Appreciate for quickly spotting the bug and reporting that here. 
Looks like even in the case of optimistic restore through this method,
cursor would never be in BTR_PCUR_IS_POSITIONED_OPTIMISTIC state at the first 
place because the method is called after the last record on the page.
so it is a dead code.

[27 Jan 9:55] Yichang SONG

Hello Mr. Sisondia,

I agree with your point that this is indeed dead code. I am just very curious whether the scenario where the m_pos_state is set to `BTR_PCUR_IS_POSITIONED_OPTIMISTIC` during restore only occurs in `row0sel.cc:5176` and `row0sel.cc:5210`:

```
pcur->store_position(&mtr);

/* The found record was not a match, but may be used
as NEXT record (index_next). Set the relative position
to BTR_PCUR_BEFORE, to reflect that the position of
the persistent cursor is before the found/stored row
(pcur->old_rec). */
ut_ad(pcur->m_rel_pos == BTR_PCUR_ON);
pcur->m_rel_pos = BTR_PCUR_BEFORE;
```

Here, after `store_position`, `m_rel_pos` is explicitly changed, which creates an opportunity for `m_pos_state` to be set to `BTR_PCUR_IS_POSITIONED_OPTIMISTIC` during restore. If I'm correct, then perhaps the commonly seen logic below:

```
if (pcur->m_pos_state == BTR_PCUR_IS_POSITIONED_OPTIMISTIC) {
  ut_ad(pcur->m_rel_pos == BTR_PCUR_BEFORE ||
        pcur->m_rel_pos == BTR_PCUR_AFTER);
}
```

should remove the assertion for `pcur->m_rel_pos == BTR_PCUR_AFTER`.

[28 Jan 0:54] Rahul Sisondia

> should remove the assertion for `pcur->m_rel_pos == BTR_PCUR_AFTER`.

I agree. At present we can do optimistic restore in one direction only. 

I shall remove this assert to avoid the confusion to maintainers and readers.

[30 Jan 1:03] Philip Olson

Posted by developer:
 
Fixed as of the upcoming MySQL Server 8.0.42, 8.4.5, and 9.3.0 releases, and here's the proposed changelog entry from the documentation team for review (which is needed as I don't understand this topic well):

Refactored code related to BPR_PCUR_* positioning for restore operations.

Thank you for the bug report.