[Zodb-checkins] SVN: ZODB/trunk/src/ Fixed a serious bug:

[Jim Fulton]

Log message for revision 96649:
  Fixed a serious bug:
    Packing a blob-enabled file storage in a ZEO server caused blob data
    to be lost.
  

Changed:
  U   ZODB/trunk/src/CHANGES.txt
  U   ZODB/trunk/src/ZODB/FileStorage/fspack.py
  U   ZODB/trunk/src/ZODB/FileStorage/tests.py

-=-
Modified: ZODB/trunk/src/CHANGES.txt
===================================================================
--- ZODB/trunk/src/CHANGES.txt	2009-02-17 19:21:14 UTC (rev 96648)
+++ ZODB/trunk/src/CHANGES.txt	2009-02-17 20:04:44 UTC (rev 96649)
@@ -22,6 +22,15 @@
   XXX There are known issues with this implementation that need to be
   sorted out before it is "released".
 
+3.9.0a11 (2009-02-17)
+=====================
+
+Bugs Fixed
+----------
+
+- Packing a blob-enabled file storage in a ZEO server caused blob data
+  to be lost.
+
 3.9.0a10 (2009-01-05)
 =====================
 

Modified: ZODB/trunk/src/ZODB/FileStorage/fspack.py
===================================================================
--- ZODB/trunk/src/ZODB/FileStorage/fspack.py	2009-02-17 19:21:14 UTC (rev 96648)
+++ ZODB/trunk/src/ZODB/FileStorage/fspack.py	2009-02-17 20:04:44 UTC (rev 96649)
@@ -267,7 +267,7 @@
                     # special case, pack to before creation time
                     continue
                 raise
-            
+
             reachable[oid] = pos
             for oid in self.findrefs(pos):
                 if oid not in reachable:
@@ -343,7 +343,7 @@
                 os.path.join(storage.blob_dir, '.removed'), 'w')
         else:
             self.pack_blobs = False
-            
+
         path = storage._file.name
         self._name = path
         # We open our own handle on the storage so that much of pack can
@@ -501,12 +501,22 @@
                     if data and ZODB.blob.is_blob_record(data):
                         # We need to remove the blob record. Maybe we
                         # need to remove oid:
-                        if h.oid not in self.gc.reachable:
-                            self.blob_removed.write(h.oid.encode('hex')+'\n')
-                        else:
-                            self.blob_removed.write(
-                                (h.oid+h.tid).encode('hex')+'\n')
-                
+
+                        # But first, we need to make sure the record
+                        # we're looking at isn't a dup of the current
+                        # record. There's a bug in ZEO blob support that causes
+                        # duplicate data records.
+                        rpos = self.gc.reachable.get(h.oid)
+                        is_dup = (rpos
+                                  and self._read_data_header(rpos).tid == h.tid)
+                        if not is_dup:
+                            if h.oid not in self.gc.reachable:
+                                self.blob_removed.write(
+                                    h.oid.encode('hex')+'\n')
+                            else:
+                                self.blob_removed.write(
+                                    (h.oid+h.tid).encode('hex')+'\n')
+
                 pos += h.recordlen()
                 continue
 

Modified: ZODB/trunk/src/ZODB/FileStorage/tests.py
===================================================================
--- ZODB/trunk/src/ZODB/FileStorage/tests.py	2009-02-17 19:21:14 UTC (rev 96648)
+++ ZODB/trunk/src/ZODB/FileStorage/tests.py	2009-02-17 20:04:44 UTC (rev 96649)
@@ -14,16 +14,18 @@
 
 from zope.testing import doctest
 
+import cPickle
 import os
 import time
 import transaction
 import unittest
+import ZODB.blob
 import ZODB.FileStorage
 import ZODB.tests.util
 
 def pack_keep_old():
     """Should a copy of the database be kept?
-    
+
 The pack_keep_old constructor argument controls whether a .old file (and .old directory for blobs is kept.)
 
     >>> fs = ZODB.FileStorage.FileStorage('data.fs', blob_dir='blobs')
@@ -61,7 +63,7 @@
     True
     >>> db.close()
 
-    
+
     >>> fs = ZODB.FileStorage.FileStorage('data.fs', blob_dir='blobs',
     ...                                   create=True, pack_keep_old=False)
     >>> db = ZODB.DB(fs)
@@ -85,11 +87,44 @@
     >>> os.path.exists('blobs.old')
     False
     >>> db.close()
-    
-    
     """
 
+def pack_with_repeated_blob_records():
+    """
+    There is a bug in ZEO that causes duplicate bloc database records
+    to be written in a blob store operation. (Maybe this has been
+    fixed by the time you read this, but there might still be
+    transactions in the wild that have duplicate records.
 
+    >>> fs = ZODB.FileStorage.FileStorage('t', blob_dir='bobs')
+    >>> db = ZODB.DB(fs)
+    >>> conn = db.open()
+    >>> conn.root()[1] = ZODB.blob.Blob()
+    >>> transaction.commit()
+    >>> tm = transaction.TransactionManager()
+    >>> oid = conn.root()[1]._p_oid
+    >>> blob_record, oldserial = fs.load(oid)
+
+    Now, create a transaction with multiple saves:
+
+    >>> trans = tm.begin()
+    >>> fs.tpc_begin(trans)
+    >>> open('ablob', 'w').write('some data')
+    >>> _ = fs.store(oid, oldserial, blob_record, '', trans)
+    >>> _ = fs.storeBlob(oid, oldserial, blob_record, 'ablob', '', trans)
+    >>> fs.tpc_vote(trans)
+    >>> fs.tpc_finish(trans)
+
+    >>> time.sleep(.01)
+    >>> db.pack()
+
+    >>> conn.sync()
+    >>> conn.root()[1].open().read()
+    'some data'
+
+    >>> db.close()
+    """
+
 def test_suite():
     return unittest.TestSuite((
         doctest.DocFileSuite(