Details
-
Bug
-
Status: Closed (View Workflow)
-
Major
-
Resolution: Fixed
-
10.2(EOL), (14)
10.3(EOL), 10.4(EOL), 10.5, 10.6, 10.7(EOL), 10.8(EOL), 10.9(EOL), 10.10(EOL), 10.11, 11.0(EOL), 11.1(EOL), 11.2(EOL), 11.3(EOL), 11.4
Description
The class buf::Block_hint that was added in order to fix MDEV-23693 is an unnecessary performance hog that causes unnecessary lookups on buf_pool.page_hash.
Originally, I was thinking that this function could be removed as part of rewriting the buffer pool resizing in MDEV-29445, but it turns out that we can do it independently of that. We only need to add a field btr_pcur_t::old_page_id to remember the page identifier where the persistent cursor position was stored, and pass this identifier to buf_page_optimistic_get(), which will have to perform a buf_pool.page_hash lookup anyway.
Furthermore, in btr_pcur_optimistic_latch_leaves() we had better look up the current page normally, and perform some tricks to prevent a deadlock when acquiring a latch on the previous page, if needed. This will also allow us to simplify the function btr_pcur_move_backward_from_page().
Attachments
Issue Links
- causes
-
-
- Closed
-
- relates to
-
-
- Open
-
-
-
- Closed
-
-
-
- Closed
-
-
MDEV-29445 reorganise innodb buffer pool (and remove buffer pool chunks)
-
- Closed
-
-
-
- Closed
-
Activity
ok, assigning back then. Here is my comment from the pull request review.
The patch is doing way more than removing buf::Block_hint. In fact it is making a major design change.
I checked the design and doesn't look very promising. The current design is to buffer fix the page and take the locks previous-current-next in order. This is used for usual scan and repositioning. During reposition the only possible conflict is a concurrent pessimistic operation.
The modified design is doing a try latch on reverse order which is bound to conflict and cause repeated retry (which we nee to do) with any concurrent Query. Historically the reverse scan is also known to have performance issue.
I don't agree with the design change and I think will cause issues for us in future. May be if you can explain the reason for the performance hog, we can improve it without changing the latch order.
As part of this change, btr_pcur_optimistic_latch_leaves() is modified so that it will use common code btr_latch_prev() for acquiring a latch on the previous page. Some change in this function is necessary, because the caller would no longer hold a buffer-fix on the current block. Previously, we were reading the FIL_PAGE_PREV and FIL_PAGE_NEXT fields of the page while holding a buffer-fix only. This part can be improved. We could refactor part of buf_page_optimistic_get() to a new function buf_page_optimistic_fix(), which would be close to what buf::Block_hint did, and invoke that function in btr_pcur_optimistic_latch_leaves().
This is also fixing a MDEV-31767 like bug in the function btr_latch_prev(). Currently, after acquiring a page latch, we are not checking that the page ID is what we expected. If a corrupted page had been read into the buffer pool and is being evicted, the operation must fail gracefully.
Today, I was able to improve performance by shortening the critical section of buf_pool.page_hash in buf_page_optimistic_get(). In my oltp_update_index test, the throughput is at par with the baseline. After revising btr_pcur_optimistic_latch_leaves() so that it will acquire the latches in the canonical order, we could see a performance improvement.
debarun, I have implemented some changes. You are absolutely right that the fast path btr_pcur_optimistic_latch_leaves() had better minimize the chances of retry logic. In the slow path it is unavoidable. I actually got an rr replay trace from mleich where we hit trouble because there was a concurrent page split or merge. The buf_block_t::modify_clock is only being incremented when a pointer to a record may become invalid (due to deleting a record, reorganizing a page or evicting a block). While the field should be updated on page splits, as far as I can tell, it won’t be updated on all pages that are involved in a page merge.
origin/st-10.6-MDEV-33588-MDEV-33819 ff6d6cca9542387ed1099006d93afbc7858b9fce 2024-04-05T14:19:45+03:00
performed well in RQG testing.
Based on the quick performance numbers that I just posted to
MDEV-29445, this may need more work. We may want to take a shortcut when buffer pool resizing is not in progress.