[ZODB-Dev] RelStorage 1.5.1 and persistent 4.0.5+ Incompatible (patch)

[jason.madden at nextthought.com]

Hi all,

We recently started testing with newer versions of ZODB3 and RelStorage, specifically ZODB3 3.11.0a1 together with the current release of RelStorage 1.5.1. The updated ZODB3 release brings in the newly-independent release of persistent (currently 4.0.6), and together there seems to be a small incompatibility resulting in the following errors at commit time:

> File 'python2.7/site-packages/transaction-1.4.1-py2.7.egg/transaction/_transaction.py', line 394 in _commitResources
>    rm.tpc_vote(self)
> File 'python2.7/site-packages/ZODB-4.0.0a4-py2.7.egg/ZODB/Connection.py', line 781 in tpc_vote
>    s = vote(transaction)
> File 'python2.7/site-packages/relstorage/storage.py', line 789 in tpc_vote
>    return self._vote()
> File 'python2.7/site-packages/relstorage/storage.py', line 823 in _vote
>    self._prepare_tid()
> File 'python2.7/site-packages/relstorage/storage.py', line 705 in _prepare_tid
>    tid_int = u64(tid)
> File 'python2.7/site-packages/ZODB-4.0.0a4-py2.7.egg/ZODB/utils.py', line 82 in u64
>    return unpack(">Q", v)[0]
> error: unpack requires a string argument of length 8


In `relstorage.storage.RelStorage._prepare_tid`, a `persistent.timestamp.TimeStamp` is used to get the `tid` value. Instead of using `TimeStamp.raw()`, however, RelStorage uses `repr(TimeStamp)`. 

Prior to persistent 4.0.5, `repr(TimeStamp)` was equivalent to `TimeStamp.raw()`, but now `repr(TimeStamp)` actually returns a platform-specific representation, using the appropriate escaping (commit e692af8281466fa309aae9273864039dcb287383 for the C version). The net result is that instead of `repr(TimeStamp)` returning an 8-byte string like b'\x01\x01\x01\x01\x01\x01\x01\x01' it returns a 25-byte string like b"'\\x01\\x01\\x01\\x01\\x01\\x01\\x01\\x01'" (on python2.7) and RelStorage's assumption is broken, leading to the above error.

I only spotted two uses of this assumption in RelStrorage, the above-mentioned `_prepare_tid`, plus `pack`. The following simple patch to change those places to use `raw` makes our own internal tests (python2.7, MySQL) pass.

RelStorage's setup.py requires ZODB3 >= 3.7.0. Based on what's at http://svn.zope.org/ZODB3/branches/3.7/src/persistent/, it looks like the `TimeStamp.raw` method has been available since at least ZODB 3.7, so it looks like this should be safe to use on all supported versions (but I haven't tried to test that).


===================================================================
--- storage.py	(revision 130026)
+++ storage.py	(working copy)
@@ -738,7 +738,7 @@
        now = time.time()
        stamp = TimeStamp(*(time.gmtime(now)[:5] + (now % 60,)))
        stamp = stamp.laterThan(TimeStamp(p64(last_tid)))
-        tid = repr(stamp)
+        tid = stamp.raw()

        tid_int = u64(tid)
        adapter.txncontrol.add_transaction(cursor, tid_int, user, desc, ext)
@@ -1138,8 +1138,7 @@
            try:
                if not skip_prepack:
                    # Find the latest commit before or at the pack time.
-                    pack_point = repr(
-                        TimeStamp(*time.gmtime(t)[:5] + (t % 60,)))
+                    pack_point = TimeStamp(*time.gmtime(t)[:5] + (t % 60,)).raw()
                    tid_int = adapter.packundo.choose_pack_transaction(
                        u64(pack_point))
                    if tid_int is None:


I hope this helps.

Thanks,
Jason