Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PS-9297 Lost a record while scanning in reverse order on cursor #5348

Open
wants to merge 1 commit into
base: release-8.0.37-29
Choose a base branch
from

Conversation

zhry110
Copy link

@zhry110 zhry110 commented Jul 5, 2024

https://perconadev.atlassian.net/browse/PS-9297

Problem

If the last user record on the prev leaf is pessimistically updated during move_backward_from_page, the record may be deleted from the prev leaf and reinserted into the current page. Scanning in reverse order will miss this record.

Cause

Same as PS-9092, move_backward_from_page cannot sense that records are moved from the prev leaf to the current leaf.

Solution

If btr_insert_into_right_sibling causes records to move between pages, prevent the next page from being optimistically accessed

https://perconadev.atlassian.net/browse/PS-9297

Problem
========
If the last user record on the prev leaf is pessimistically updated during move_backward_from_page, the record may be deleted from the prev leaf and reinserted into the current page. Scanning in reverse order will miss this record.

Cause
========
Same as PS-9092, move_backward_from_page cannot sense that records are moved from the prev leaf to the current leaf.

Solution
========
If btr_insert_into_right_sibling causes records to move between pages, prevent the next page from being optimistically accessed
@zhry110 zhry110 force-pushed the release-8.0.37-29 branch from 91d57ec to 8e03ddc Compare July 5, 2024 08:11
@percona-ysorokin
Copy link
Collaborator

Thanks for your contribution, @zhry110
Indeed a high quality patch with a non-trivial MTR test case 💪
Unfortunately, at the moment it is too late to include this patch into 8.0.37.
However, we will definitely do this in 8.0.38.

* ensure that the record will not be mistakenly skipped during reverse
* scanning on cursor.
*/
buf_block_modify_clock_inc(btr_cur_get_block(cursor));
Copy link
Contributor

@satya-bodapati satya-bodapati Jul 15, 2024

Choose a reason for hiding this comment

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

While PS-9092 blocked the merge(that caused records movement), this bug shows that records are moved without a merge.

But when a record is moved pessimistically, how can we assume it is transferred to a neighbour? You should limit this check if the movement is neighbour only.

If the record is moved to a non-neighbour, please check this case.
It should be protected by delete marking of record. ie we should read delete marked version of record and the new version that is moved to some far page is not visible/seen. This is OK.

To summarize:

  1. record movement due to merges (PS-9092)
  2. record movement to neighbour without merge (this PR )
  3. record movement to FAR pages (to be verified that delete marked version is read and new version is not SEEN).

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, but in our observations, pessimistic updates are limited to the current page and its sibling pages. Whether it is inserted after splitting or inserted directly into its sibling page, the inserted page needs to be disabled for optimistic access, so we do not need stricter judgment here. In addition, the original record has been physically deleted from the original page, and no changes to the delete-mark will be involved.

Copy link
Contributor

@satya-bodapati satya-bodapati Jul 16, 2024

Choose a reason for hiding this comment

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

Hmm, that's because for the query update c set val=repeat('b',6000) where id=2, the update only changes non-key fields, and the new record should always land on neighbour pages.

But if there are queries that update key, records can move with delete marking.
For example UPDATE c SET id = 20 WHERE id = 5;

Copy link
Author

@zhry110 zhry110 Jul 16, 2024

Choose a reason for hiding this comment

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

Whether it is PS-9092 or PS-9297, the necessary condition to trigger the problem is that the same physical record is moved between adjacent pages. But UPDATE c SET id = 20 WHERE id = 5; actually del-marks the record with id=5 and inserts the record with id=20 within a transaction. These two records are not physically the same record. Therefore I think this scenario is not affected by this problem, the ReadView of the read transaction will ensure that the correct version of the record is visible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants