From 2368e93c0a3ea519dfedcc7948ef54ae5a5d274d Mon Sep 17 00:00:00 2001
From: Zsolt Parragi <zsolt.parragi@cancellar.hu>
Date: Tue, 22 Mar 2022 09:07:59 +0100
Subject: [PATCH] Improve locking iterator performance

Issue: locking iterator is several times slower than the default
iterator in queries based on secondary keys.

For example:

select COUNT(*) from sbtest1 where k % 10 = 1 for update;

This query ends up iterating over the entire table, and in the end locks
the entire table, but does so by lots of small locks and seeks every
time.

This diff shows that by simply locking the entire table instead we can
improve the performance of the iterator by 50% - it's still slower than
the default implementation, but now it's much closer to it.

This modification of course results in overlocking in queries such as

select COUNT(*) from sbtest1 where k % 10 = 1 for update limit 3000;

where instead of locking only a few thousands records, with this change
we lock the entire table instead.

This change is in this form a proof of concept, for a real patch I
think we could go in two directions:

* instead of a hard coded value, make this an adjustable session
  variable
* or instead of simply stopping after a threshold, continue locking in
  linear or exponential batches
---
 storage/rocksdb/rdb_locking_iter.cc | 37 +++++++++++++++++++++--------
 storage/rocksdb/rdb_locking_iter.h  |  1 +
 2 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/storage/rocksdb/rdb_locking_iter.cc b/storage/rocksdb/rdb_locking_iter.cc
index 35b3763f7f35..88082d229e0c 100644
--- a/storage/rocksdb/rdb_locking_iter.cc
+++ b/storage/rocksdb/rdb_locking_iter.cc
@@ -8,6 +8,8 @@
 /* This C++ file's header file */
 #include "./rdb_locking_iter.h"
 
+static const ulonglong max_lock_count = 1000;
+
 namespace myrocks {
 
 rocksdb::Iterator* GetLockingIterator(
@@ -119,11 +121,12 @@ void LockingIterator::lock_up_to(bool scan_forward,
     m_have_locked_until= true;
     m_locked_until.assign(end_key.data(), end_key.size());
     if (m_lock_count)  (*m_lock_count)++;
+    m_self_lock_count++;
   }
 }
 
 /*
-  Lock the range from target till the iterator end point that we are scaning
+  Lock the range from target till the iterator end point that we are scanning
   towards. If there's no iterator bound, use index start (or end, depending
   on the scan direction)
 */
@@ -141,7 +144,7 @@ void LockingIterator::lock_till_iterator_end(bool scan_forward,
       else
         m_kd->get_supremum_key(buf, &size);
 
-      DBUG_ASSERT(size == Rdb_key_def::INDEX_NUMBER_SIZE);
+      assert(size == Rdb_key_def::INDEX_NUMBER_SIZE);
       end = rocksdb::Slice((const char*)buf, size);
     }
   } else {
@@ -153,7 +156,7 @@ void LockingIterator::lock_till_iterator_end(bool scan_forward,
       else
         m_kd->get_infimum_key(buf, &size);
 
-      DBUG_ASSERT(size == Rdb_key_def::INDEX_NUMBER_SIZE);
+      assert(size == Rdb_key_def::INDEX_NUMBER_SIZE);
       end = rocksdb::Slice((const char*)buf, size);
     }
   }
@@ -171,6 +174,8 @@ void LockingIterator::lock_till_iterator_end(bool scan_forward,
 */
 void LockingIterator::Scan(bool scan_forward,
                            const rocksdb::Slice& target, bool call_next) {
+
+
   if (!m_iter->Valid()) {
     m_status = m_iter->status();
     m_valid = false;
@@ -180,6 +185,10 @@ void LockingIterator::Scan(bool scan_forward,
     }
     return;
   }
+  if (m_self_lock_count > max_lock_count) {
+    // already locked the entire index instead
+    return;
+  }
 
   while (1) {
 
@@ -197,11 +206,19 @@ void LockingIterator::Scan(bool scan_forward,
     auto end_key = m_iter->key();
     std::string end_key_copy= end_key.ToString();
 
-    lock_up_to(scan_forward, target, end_key);
-    if (!m_status.ok()) {
-      // Failed to get a lock (most likely lock wait timeout)
-      m_valid = false;
-      return;
+    if (m_self_lock_count == max_lock_count) 
+    {
+      // we can't handle too many small locks, just lock everything
+      lock_till_iterator_end(scan_forward, target);
+    } 
+    if (m_self_lock_count < max_lock_count) 
+    {
+      lock_up_to(scan_forward, target, end_key);
+      if (!m_status.ok()) {
+        // Failed to get a lock (most likely lock wait timeout)
+        m_valid = false;
+        return;
+      }
     }
 
     //Ok, now we have a lock which is inhibiting modifications in the range
@@ -265,7 +282,7 @@ void LockingIterator::Scan(bool scan_forward,
 */
 
 void LockingIterator::SeekToFirst() {
-  DBUG_ASSERT(0);
+  assert(0);
   m_status = rocksdb::Status::NotSupported("Not implemented");
   m_valid = false;
 }
@@ -276,7 +293,7 @@ void LockingIterator::SeekToFirst() {
 */
 
 void LockingIterator::SeekToLast() {
-  DBUG_ASSERT(0);
+  assert(0);
   m_status = rocksdb::Status::NotSupported("Not implemented");
   m_valid = false;
 }
diff --git a/storage/rocksdb/rdb_locking_iter.h b/storage/rocksdb/rdb_locking_iter.h
index 3cd091436490..0e6aaa5c95ad 100644
--- a/storage/rocksdb/rdb_locking_iter.h
+++ b/storage/rocksdb/rdb_locking_iter.h
@@ -42,6 +42,7 @@ class LockingIterator : public rocksdb::Iterator {
   bool  m_valid;
 
   ulonglong *m_lock_count;
+  ulonglong m_self_lock_count = 0;
 
   // If true, m_locked_until has a valid key value.
   bool m_have_locked_until;