Bug #91561 Reduced Transaction mutex references

• Submitted: 6 Jul 2018 5:16
• Modified: 3 Jan 11:29
• Reporter: Sandeep Sethia (OCA)
• Status: Won't fix
• Category: MySQL Server: InnoDB storage engine
• Severity: S3 (Non-critical)
• Version: 8.0
• OS: CentOS
• 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] Umesh Shastry

Hello Sandeep Sethia,

Thank you for the report and contribution.

Thanks,
Umesh

[23 Jul 2018 6:46] Sandeep Sethia

Any update?

[3 Jan 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.