Comments/changenote and simplify implementation of new_oid

CHANGES.rst

@@ -24,6 +24,10 @@
 - Add initial support for the `MySQL Connector/Python
   <https://dev.mysql.com/doc/connector-python/en/>`_ driver. See
   :issue:`155`.
+- Backport `ZODB #140
+  <https://github.com/zopefoundation/ZODB/pull/140>`_ to older
+  versions of ZODB. This improves write performance, especially in
+  multi-threaded scenarios, by up to 10%.
 
 2.0.0 (2016-12-23)
 ==================

relstorage/__init__.py

@@ -20,6 +20,8 @@ if not hasattr(Connection, 'new_oid'):
     # storage. This has to be a data descriptor because
     # connection will try to reset this.
     # See https://github.com/zopefoundation/ZODB/issues/139
+    # Merged in https://github.com/zopefoundation/ZODB/pull/140,
+    # expected release in 5.1.2
 
     class NewOid(object):
 

relstorage/adapters/interfaces.py

@@ -343,10 +343,17 @@ class IOIDAllocator(Interface):
     """Allocate OIDs and control future allocation"""
 
     def new_oids(cursor):
-        """Return a sequence of new, unused OIDs."""
+        """
+        Return a new ``list`` of new, unused integer OIDs.
+
+        The list should be contiguous and must
+        be in sorted order from highest to lowest.
+        """
 
     def set_min_oid(cursor, oid):
-        """Ensure the next OID is at least the given OID."""
+        """
+        Ensure the next OID is at least the given integer OID.
+        """
 
 
 class IPackUndo(Interface):

relstorage/adapters/mysql/oidallocator.py

@@ -41,8 +41,8 @@ class MySQLOIDAllocator(AbstractOIDAllocator):
         # cursor.connection.insert_id(), which was specific to MySQLdb)
         n = cursor.lastrowid
 
-        if n % 100 == 0:
+        if n % 1000 == 0:
             # Clean out previously generated OIDs.
             stmt = "DELETE FROM new_oid WHERE zoid < %s"
             cursor.execute(stmt, (n,))
-        return range(n * 16 - 15, n * 16 + 1)
+        return self._oid_range_around(n)

relstorage/adapters/oidallocator.py

@@ -22,10 +22,6 @@ import abc
 
 @six.add_metaclass(abc.ABCMeta)
 class AbstractOIDAllocator(object):
-    # All of these allocators allocate 16 OIDs at a time.  In the sequence
-    # or table, value (n) represents (n * 16 - 15) through (n * 16).  So,
-    # value 1 represents OID block 1-16, 2 represents OID block 17-32,
-    # and so on.
 
     @abc.abstractmethod
     def set_min_oid(self, cursor, oid):
@@ -34,3 +30,20 @@ class AbstractOIDAllocator(object):
     @abc.abstractmethod
     def new_oids(self, cursor):
         raise NotImplementedError()
+
+    # All of these allocators allocate 16 OIDs at a time.  In the sequence
+    # or table, value (n) represents (n * 16 - 15) through (n * 16).  So,
+    # value 1 represents OID block 1-16, 2 represents OID block 17-32,
+    # and so on. The _oid_range_around helper method returns a list
+    # around this number sorted in the proper way.
+    # Note than range(n * 16 - 15, n*16+1).sort(reverse=True)
+    # is the same as range(n * 16, n*16 -16, -1)
+    if isinstance(range(1), list):
+        # Py2
+        def _oid_range_around(self, n):
+            return range(n * 16, n * 16 - 16, -1)
+    else:
+        def _oid_range_around(self, n):
+            l = list(range(n * 16, n * 16 - 16, -1))
+            l.sort(reverse=True)
+            return l

relstorage/adapters/oracle/oidallocator.py

@@ -59,4 +59,4 @@ class OracleOIDAllocator(AbstractOIDAllocator):
         stmt = "SELECT zoid_seq.nextval FROM DUAL"
         cursor.execute(stmt)
         n = cursor.fetchone()[0]
-        return range(n * 16 - 15, n * 16 + 1)
+        return self._oid_range_around(n)

relstorage/adapters/postgresql/oidallocator.py

@@ -41,4 +41,4 @@ class PostgreSQLOIDAllocator(AbstractOIDAllocator):
         stmt = "SELECT NEXTVAL('zoid_seq')"
         cursor.execute(stmt)
         n = cursor.fetchone()[0]
-        return range(n * 16 - 15, n * 16 + 1)
+        return self._oid_range_around(n)

relstorage/storage.py

@@ -187,7 +187,7 @@ class RelStorage(UndoLogCompatible,
     def __init__(self, adapter, name=None, create=None,
                  options=None, cache=None, blobhelper=None,
                  # The top-level storage should use locks because
-                 # new_oid is shared among all connections. But the new_instance
+                 # new_oid is (potentially) shared among all connections. But the new_instance
                  # objects don't need to.
                  _use_locks=True,
                  **kwoptions):
@@ -399,12 +399,12 @@ class RelStorage(UndoLogCompatible,
             else:
                 log.info("Reconnected.")
 
-    def _with_store(self, f, *args, **kw):
-        """Call a function with the store connection and cursor."""
+    def __with_store(self, f):
+        """Call a function with the store cursor."""
         if self._store_cursor is None:
             self._open_store_connection()
         try:
-            return f(self._store_conn, self._store_cursor, *args, **kw)
+            return f(self._store_cursor)
         except self._adapter.connmanager.disconnected_exceptions as e:
             if self._transaction is not None:
                 # If transaction commit is in progress, it's too late
@@ -418,7 +418,7 @@ class RelStorage(UndoLogCompatible,
                 log.exception("Reconnect failed.")
                 raise
             log.info("Reconnected.")
-            return f(self._store_conn, self._store_cursor, *args, **kw)
+            return f(self._store_cursor)
 
     def zap_all(self, **kwargs):
         """Clear all objects and transactions out of the database.
@@ -1090,19 +1090,20 @@ class RelStorage(UndoLogCompatible,
         if self._is_read_only:
             raise ReadOnlyError()
         with self._lock:
-            # This method is actually called on the storage object of
-            # the DB, not the storage object of a connection for some
-            # reason. This means this method is shared among all
-            # connections using a database.
-            if self._preallocated_oids:
-                oid_int = self._preallocated_oids.pop()
-            else:
-                def f(_conn, cursor):
-                    return list(self._adapter.oidallocator.new_oids(cursor))
-                preallocated = self._with_store(f)
-                preallocated.sort(reverse=True)
-                oid_int = preallocated.pop()
+            # See comments in __init__.py. Prior to that patch and
+            # ZODB 5.1.2, this method was actually called on the
+            # storage object of the DB, not the storage object of a
+            # connection for some reason. This meant that this method
+            # (and the oid cache) was shared among all connections
+            # using a database and was called outside of a transaction
+            # (starting its own long-running transaction). The
+            # DB.new_oid() method still exists, so we still need to
+            # support that usage, hence `with_store`.
+            if not self._preallocated_oids:
+                preallocated = self.__with_store(self._adapter.oidallocator.new_oids)
                 self._preallocated_oids = preallocated
+
+            oid_int = self._preallocated_oids.pop()
             self._max_new_oid = max(self._max_new_oid, oid_int)
             return p64(oid_int)