Merge pull request #372 from zodb/unify-oids

SQLite: Fix a bug setting the minimum OID.

CHANGES.rst

@@ -5,7 +5,8 @@
 3.0b3 (unreleased)
 ==================
 
-- Nothing changed yet.
+- SQLite: Fix a bug that could lead to invalid OIDs being allocated if
+  transactions were imported from another storage.
 
 
 3.0b2 (2019-10-28)

src/relstorage/_compat.py

@@ -282,7 +282,7 @@ def __call__(self, replacement):
     from abc import ABC
 except ImportError:
     import abc
-    ABC = abc.ABCMeta('ABC', (object,), {})
+    ABC = abc.ABCMeta('ABC', (object,), {'__slots__': ()})
     del abc
 
 # Functions

src/relstorage/adapters/mysql/oidallocator.py

@@ -61,7 +61,7 @@ class MySQLOIDAllocator(AbstractTableOIDAllocator):
     # "Any AUTO_INCREMENT value is reset to its start value. This is
     # true even for MyISAM and InnoDB, which normally do not reuse
     # sequence values."
-    def set_min_oid(self, cursor, oid_int):
+    def _set_min_oid_from_range(self, cursor, n):
         # A simple statement like the following can easily deadlock:
         #
         # INSERT INTO new_oid (zoid)
@@ -78,8 +78,6 @@ def set_min_oid(self, cursor, oid_int):
         # Partly this is because MAX() is local to the current session.
         # We deal with this by using a stored procedure to efficiently make
         # multiple queries.
-
-        n = (oid_int + 15) // 16
         self.driver.callproc_multi_result(cursor, 'set_min_oid(%s)', (n,))
 
    # Notes on new_oids:

src/relstorage/adapters/oidallocator.py

@@ -27,6 +27,33 @@
 
 logger = __import__('logging').getLogger(__name__)
 
+class AbstractOIDAllocator(ABC):
+    """
+    These objects are intended to be stateless.
+
+    They consult an underlying database sequence than increases, and
+    then derive additional OIDs from that sequence. ``new_oids``
+    returns a list of those OIDs, with the largest such OID in
+    position 0.
+    """
+    __slots__ = ()
+
+    def new_instance(self):
+        return self
+
+    close = release = lambda self: None
+
+    def reset_oid(self, cursor):
+        raise NotImplementedError
+
+    @abc.abstractmethod
+    def set_min_oid(self, cursor, oid_int):
+        raise NotImplementedError
+
+    @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
@@ -55,31 +82,18 @@ def _oid_range_around_assume_list(n, _s=_OID_RANGE_SIZE):
 def _oid_range_around_iterable(n, _s=_OID_RANGE_SIZE, _range=_oid_range_around_assume_list):
     return list(_range(n))
 
-class AbstractOIDAllocator(ABC):
-    """
-    These objects are intended to be stateless.
 
-    They consult an underlying database sequence than increases, and
-    then derive additional OIDs from that sequence. ``new_oids``
-    returns a list of those OIDs, with the largest such OID in
-    position 0.
-    """
+class AbstractRangedOIDAllocator(AbstractOIDAllocator):
     __slots__ = ()
 
-    def new_instance(self):
-        return self
-
-    close = release = lambda self: None
-
-    def reset_oid(self, cursor):
-        raise NotImplementedError
-
-    @abc.abstractmethod
     def set_min_oid(self, cursor, oid_int):
-        raise NotImplementedError
+        # This takes a user-space OID and turns it into the internal
+        # range number.
+        n = (oid_int + 15) // 16
+        self._set_min_oid_from_range(cursor, n)
 
     @abc.abstractmethod
-    def new_oids(self, cursor):
+    def _set_min_oid_from_range(self, cursor, n):
         raise NotImplementedError
 
     if isinstance(range(1), list):
@@ -89,7 +103,7 @@ def new_oids(self, cursor):
         _oid_range_around = staticmethod(_oid_range_around_iterable)
 
 
-class AbstractTableOIDAllocator(AbstractOIDAllocator):
+class AbstractTableOIDAllocator(AbstractRangedOIDAllocator):
     """
     Implements OID allocation using a table, new_oid, with an incrementing
     column.

src/relstorage/adapters/oracle/oidallocator.py

@@ -21,18 +21,17 @@
 
 from ..._compat import metricmethod
 from ..interfaces import IOIDAllocator
-from ..oidallocator import AbstractOIDAllocator
+from ..oidallocator import AbstractRangedOIDAllocator
 
 
 @implementer(IOIDAllocator)
-class OracleOIDAllocator(AbstractOIDAllocator):
+class OracleOIDAllocator(AbstractRangedOIDAllocator):
 
     def __init__(self, connmanager):
         self.connmanager = connmanager
 
-    def set_min_oid(self, cursor, oid_int):
+    def _set_min_oid_from_range(self, cursor, n):
         """Ensure the next OID is at least the given OID."""
-        n = (oid_int + 15) // 16
         stmt = "SELECT zoid_seq.nextval FROM DUAL"
         cursor.execute(stmt)
         next_n = cursor.fetchone()[0]

src/relstorage/adapters/postgresql/oidallocator.py

@@ -21,17 +21,15 @@
 
 from ..._compat import metricmethod
 from ..interfaces import IOIDAllocator
-from ..oidallocator import AbstractOIDAllocator
+from ..oidallocator import AbstractRangedOIDAllocator
 
 
 @implementer(IOIDAllocator)
-class PostgreSQLOIDAllocator(AbstractOIDAllocator):
+class PostgreSQLOIDAllocator(AbstractRangedOIDAllocator):
 
     __slots__ = ()
 
-    def set_min_oid(self, cursor, oid_int):
-        """Ensure the next OID is at least the given OID."""
-        n = (oid_int + 15) // 16
+    def _set_min_oid_from_range(self, cursor, n):
         # This potentially wastes a value from the sequence to find
         # out that we're already past the max. But that's the only option:
         # curval() is local to this connection, and 'ALTER SEQUENCE ... MINVALUE n STARTS WITH n'

src/relstorage/adapters/sqlite/drivers.py

@@ -311,7 +311,7 @@ class Sqlite3GeventDriver(GeventDriverMixin,
     yield_to_gevent_instruction_interval = 100
 
     def configure_from_options(self, options):
-        if options.adapter:
+        if options.adapter and options.adapter.config.gevent_yield_interval:
             conf = options.adapter.config
             self.yield_to_gevent_instruction_interval = conf.gevent_yield_interval
 

src/relstorage/adapters/sqlite/oidallocator.py

@@ -25,10 +25,10 @@
 
 from relstorage._util import consume
 from ..interfaces import IOIDAllocator
-from ..oidallocator import AbstractOIDAllocator
+from ..oidallocator import AbstractRangedOIDAllocator
 
 @implementer(IOIDAllocator)
-class Sqlite3OIDAllocator(AbstractOIDAllocator):
+class Sqlite3OIDAllocator(AbstractRangedOIDAllocator):
     # Even though we use the new_oid table like AbstractTableOIDAllocator
     # does, because we have to take exclusive locks *anyway*, we can
     # ensure that it only ever has a single increasing row.
@@ -52,6 +52,8 @@ def close(self):
                 self._connection.close()
                 self._connection = None
 
+    # This is actually the last OID we returned. We begin allocating beginning with
+    # the *next* integer.
     _new_oid_query = 'CREATE TABLE new_oid (zoid INTEGER PRIMARY KEY NOT NULL);'
 
     def _connect(self):
@@ -73,7 +75,7 @@ def _connect(self):
                     self._new_oid_query + """
                 INSERT OR REPLACE INTO new_oid
                 SELECT MAX(x) FROM (
-                    SELECT 1 x
+                    SELECT 0 x
                     UNION ALL
                     SELECT MAX(zoid)
                     FROM new_oid
@@ -82,7 +84,7 @@ def _connect(self):
             self._connection = conn
         return self._connection
 
-    def set_min_oid(self, cursor, oid_int):
+    def _set_min_oid_from_range(self, cursor, n):
         # Recall that the very first write to the database will cause
         # the file to be locked against further writes. So there's some
         # benefit in avoiding writes if they're not needed. Because we
@@ -93,12 +95,12 @@ def set_min_oid(self, cursor, oid_int):
             conn = self._connect()
             rows = conn.execute(
                 'SELECT zoid FROM new_oid WHERE zoid < ?',
-                (oid_int,)).fetchall()
+                (n,)).fetchall()
             if rows:
-                # Narf, we need to open a transaction.
+                # Narf, we need to execute a write transaction.
                 consume(conn.execute(
                     'UPDATE new_oid SET zoid = :new WHERE zoid < :new',
-                    {'new': oid_int}))
+                    {'new': n}))
 
     def new_oids(self, cursor=None):
         return self.new_oids_no_cursor()
@@ -110,8 +112,8 @@ def new_oids_no_cursor(self):
             row, = conn.execute('SELECT zoid FROM new_oid')
             conn.execute('UPDATE new_oid SET zoid = zoid + 1')
             conn.commit()
-        return self._oid_range_around(row[0])
+        return self._oid_range_around(row[0] + 1)
 
     def reset_oid(self, cursor=None):
         with self.lock:
-            self._connect().execute('UPDATE new_oid SET zoid = 1')
+            self._connect().execute('UPDATE new_oid SET zoid = 0')

src/relstorage/adapters/tests/test_oidallocator.py

@@ -8,12 +8,12 @@
 
 from relstorage.tests import TestCase
 
-from ..oidallocator import AbstractOIDAllocator
+from ..oidallocator import AbstractRangedOIDAllocator
 
 
-class MockOIDAllocator(AbstractOIDAllocator):
+class MockOIDAllocator(AbstractRangedOIDAllocator):
 
-    def set_min_oid(self, cursor, oid_int):
+    def _set_min_oid_from_range(self, cursor, n):
         raise NotImplementedError
 
     def new_oids(self, cursor):

src/relstorage/tests/hftestbase.py

@@ -347,6 +347,33 @@ def checkIteratorIsConsistent(self):
 
         storage2.close()
 
+    def checkSetMinOid(self):
+        # Verify that OID allocation goes as expected when we use
+        # set_min_oid. There have been problems in the past translating
+        # between user-space OIDs and the 16-range gapped OIDs that are actually
+        # stored.
+
+        # TODO: This test is independent of history keeping status.
+        # We should have file for all those type tests and only mix it in
+        # to one of the test classes.
+
+        storage = self._storage
+        I = bytes8_to_int64
+        for offset in range(1, 50):
+            # Ensure no gaps.
+            oid = storage.new_oid()
+            self.assertEqual(offset, I(oid))
+
+        storage._oids.set_min_oid(32768)
+        oid = storage.new_oid()
+        self.assertEqual(32769, I(oid))
+
+        # Close to the 64-bit boundary.
+        storage._oids.set_min_oid(2 ** 62)
+        # Iterate through several ranges. This has been a problem in the past.
+        for offset in range(1, 50):
+            self.assertEqual(2 ** 62 + offset, I(storage.new_oid()))
+
 
 
 class HistoryFreeToFileStorage(AbstractToFileStorage,