[MDEV-27499] tps regression with 10.8 (w.r.t to 10.6.5) Created: 2022-01-14 Updated: 2022-01-25 Resolved: 2022-01-18 |
|
| Status: | Closed |
| Project: | MariaDB Server |
| Component/s: | Storage Engine - InnoDB |
| Affects Version/s: | N/A |
| Fix Version/s: | 10.5.14, 10.6.6, 10.7.2, 10.8.1 |
| Type: | Bug | Priority: | Blocker |
| Reporter: | Krunal Bauskar | Assignee: | Marko Mäkelä |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | performance | ||
| Attachments: |
|
||||||||
| Issue Links: |
|
||||||||
| Description |
|
| Comments |
| Comment by Krunal Bauskar [ 2022-01-14 ] | ||||||||||||||
configuration details: https://github.com/mysqlonarm/benchmark-suites/blob/master/mysql-sbench/conf/mdb.cnf/100tx3m_106_cpubound.cnf + skip-log-bin + thread-pool enabled. | ||||||||||||||
| Comment by Marko Mäkelä [ 2022-01-14 ] | ||||||||||||||
|
Thank you! As of now, there is no difference whatsoever for storage/innobase between 10.7 and 10.8. Between the current 10.6 and 10.7 there are more differences, but I do not think of anything that could explain your observation. The periods of 0 transactions per second would suggest that all threads are waiting for a log checkpoint to occur. I recently fixed We have not seen anything like this in our internal testing, so we would rely on you to debug this further. In your configuration, I do not see anything special.
| ||||||||||||||
| Comment by Krunal Bauskar [ 2022-01-14 ] | ||||||||||||||
|
Marko, You are right. I see the regression even with 10.6-trunk (#e6a0611) | ||||||||||||||
| Comment by Krunal Bauskar [ 2022-01-17 ] | ||||||||||||||
|
I did some more investigation and found that the issue is likely cropping from the said commit commit 4c3ad24413f234599eda27f4958dd3ff21df3203 Said commit segregated logic to wait for dirty pages up to the said lsn in form of a new function buf_flush_wait(). This function in addition to waiting for dirty pages also waits for the checkpoint to complete in other sense resetting of buf_flush_sync_lsn. Till then all threads are blocked in the said function causing the tps to drops down to 0. Issue is hit when the checkpoint age touches the max-threshold (Innodb_checkpoint_max_age) when a sync flush starts. Tried commenting the said snippet and tps continued to maintain above 0 reducing from the original level on hitting the threshold. diff --git a/storage/innobase/buf/buf0flu.cc b/storage/innobase/buf/buf0flu.cc +#if 0 Marko, | ||||||||||||||
| Comment by Marko Mäkelä [ 2022-01-17 ] | ||||||||||||||
|
krunalbauskar, thank you for narrowing it down. I think that the wait condition is too aggressive, and threads could end up waiting for longer than necessary if the page cleaner remains in ‘furious flushing’ mode. Can you try the following patch?
| ||||||||||||||
| Comment by Krunal Bauskar [ 2022-01-17 ] | ||||||||||||||
|
Marko, 1. the said patch doesn't help. | ||||||||||||||
| Comment by Marko Mäkelä [ 2022-01-17 ] | ||||||||||||||
|
krunalbauskar, in case you wonder why there are two wait loops in the function buf_flush_wait(): The first one waits for the minimum oldest_modification in the buffer pool to be large enough. That is the prerequisite for a log checkpoint. The oldest_modification of each modified page would be cleared in buf_page_write_complete() in I/O callback threads. The final loop is waiting for the checkpoint to actually be advanced by the buf_flush_page_cleaner(). That condition is currently waiting for the page cleaner to completely exit the ‘furious flushing’ mode before exiting. I now realize that my initially suggested change cannot possibly help, because whenever buf_flush_sync_lsn is nonzero, it would be guaranteed to be at least lsn, by design. What we really might need is to wait for the log checkpoint to have advanced enough. Something like this:
I will have to review whether that second loop is needed at all. It should be unnecessary for the correctness of log_checkpoint_margin(), but I have to consider other callers as well. | ||||||||||||||
| Comment by Marko Mäkelä [ 2022-01-17 ] | ||||||||||||||
|
I believe that the second loop of buf_flush_wait() (waiting for the checkpoint) that was added in Edit: I think that the loop is completely unnecessary. For log file resizing, we explicitly disable checkpoints, so we should only care that the buffer pool is clean, not whether the page cleaner exited the ‘furious flushing’ mode. I have submitted the removal of the loop for testing. Thank you for catching this, krunalbauskar. For some reason, the condition failed to reproduce in our performance tests. Perhaps axel did not test with a small innodb_log_file_size lately? I think that I did run some perfomrance tests like that during the | ||||||||||||||
| Comment by Matthias Leich [ 2022-01-18 ] | ||||||||||||||
|
| ||||||||||||||
| Comment by Marko Mäkelä [ 2022-01-19 ] | ||||||||||||||
|
The second wait loop turns out to be necessary for log file resizing after all. We got a few failures of the test innodb.log_file_size, like this:
I am unable to reproduce this failure locally. It might be acceptable to replace this assertion with a similar one (!buf_pool.get_oldest_modification(0)), but then we might get into a race condition where the page cleaner would attempt to write a checkpoint to the newly created log file. I will add that wait loop to buf_flush_sync(), like I originally planned to. Other callers of buf_flush_sync() are in debug code. |