Bug #91561 | Reduced Transaction mutex references | ||
---|---|---|---|
Submitted: | 6 Jul 2018 5:16 | Modified: | 3 Jan 2019 11:29 |
Reporter: | Sandeep Sethia (OCA) | Email Updates: | |
Status: | Won't fix | Impact on me: | |
Category: | MySQL Server: InnoDB storage engine | Severity: | S3 (Non-critical) |
Version: | 8.0 | OS: | CentOS |
Assigned to: | CPU Architecture: | ARM | |
Tags: | Contribution, innodb |
[6 Jul 2018 5:16]
Sandeep Sethia
[6 Jul 2018 5:17]
Sandeep Sethia
Reduced transaction mutex references (*) I confirm the code being submitted is offered under the terms of the OCA, and that I am authorized to contribute it.
Contribution: trx_sys.mutex.patch (application/octet-stream, text), 1.79 KiB.
[6 Jul 2018 5:43]
MySQL Verification Team
Hello Sandeep Sethia, Thank you for the report and contribution. Thanks, Umesh
[23 Jul 2018 6:46]
Sandeep Sethia
Any update?
[3 Jan 2019 11:29]
Erlend Dahl
Posted by developer (Bin X Su): Thanks for the contribution. However, I checked the patch a bit, and found that there could be some potential issues: 1. The patch skips both MVCC::is_view_active(trx->read_view) call in ha_innobase::external_lock() and ha_innobase::store_lock(), however, this looks like incorrect. Since if this call can be ignored, then this patch should be safe: diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc index 7d60a2d..0351555 100644 --- a/storage/innobase/handler/ha_innodb.cc +++ b/storage/innobase/handler/ha_innodb.cc @@ -17460,8 +17460,8 @@ int ha_innobase::external_lock(THD *thd, /*!< in: handle to the user thread */ ut_d(trx->is_dd_trx = false); } - } else if (trx->isolation_level <= TRX_ISO_READ_COMMITTED && - MVCC::is_view_active(trx->read_view)) { + } else if (trx->isolation_level <= TRX_ISO_READ_COMMITTED) { + ut_ad(MVCC::is_view_active(trx->read_view)); mutex_enter(&trx_sys->mutex); trx_sys->mvcc->view_close(trx->read_view, true); However, this is not true, it will crash the MTR frequently. So this kind of change is somehow unsafe and probably incorrect. 2. The patch tries to acquire trx_sys mutex in MVCC::view_close() even if the own_mutex is true, this is completely wrong, since some callers with own_mutex=true will have to re-acquire the mutex again, which will crash the server always. I'm now sure about how the 3% gain comes from, if it comes from point 1, then it could be meaningless. Anyway, this patch itself is not safe enough and has some problems. So I would suggest not port it to our branches.