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:
None 
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
Description:
Moved mutex locking to MVCC::view_close() instead of doing it in every view open and close for isolation_level <= TRX_ISO_READ_COMMITTED . This helps in reducing the number of reference for transaction mutexes and improves the performance by 3% in OLTP RO for sysbench . 

How to repeat:
Code review
[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.