Skip to content

Commit e21642f

Browse files
committed
MVCCAdapter.load: Raise ReadConflictError only if pack is running simultaneously
Currently when load(oid) finds that the object was deleted, it raises ReadConflictError - not POSKeyError - because a pack could be running simultaneously and the deletion could result from the pack. In that case we want corresponding transaction to be retried - not failed - via raising ConflictError subclass for backward-compatibility reason. However from semantic point of view, it is more logically correct to raise POSKeyError, when an object is found to be deleted or not-yet-created, and raise ReadConflictError only if a pack was actually running simultaneously, and the deletion could result from that pack. -> Fix MVCCAdapter.load to do this - now it raises ReadConflictError only if MVCCAdapterInstance view appears before storage packtime, which indicates that there could indeed be conflict in between read access and pack removing the object. To detect if pack was running and beyond MVCCAdapterInstance view, we need to teach storage drivers to provide way to known what was the last pack time/transaction. Add optional IStorageLastPack interface with .lastPack() method to do so. If a storage does not implement lastPack, we take conservative approach and raise ReadConflictError unconditionally as before. Add/adapt corresponding tests. Teach FileStorage, MappingStorage and DemoStorage to implement the new interface. NOTE: storages that implement IMVCCStorage natively already raise POSKeyError - not ReadConflictError - when load(oid) finds deleted object. This is so because IMVCCStorages natively provide isolation, via e.g. RDBMS in case of RelStorage. The isolation property provided by RDBMS guarantees that connection view of the database is not affected by other changes - e.g. pack - until connection's transaction is complete. /cc @jimfulton
1 parent 0d499a3 commit e21642f

File tree

9 files changed

+217
-18
lines changed

9 files changed

+217
-18
lines changed

src/ZODB/DemoStorage.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,14 +104,20 @@ def __init__(self, name=None, base=None, changes=None,
104104
self._temporary_changes = True
105105
changes = ZODB.MappingStorage.MappingStorage()
106106
zope.interface.alsoProvides(self, ZODB.interfaces.IBlobStorage)
107+
zope.interface.alsoProvides(self, ZODB.interfaces.IStorageLastPack)
107108
if close_changes_on_close is None:
108109
close_changes_on_close = False
109110
else:
110111
if ZODB.interfaces.IBlobStorage.providedBy(changes):
111112
zope.interface.alsoProvides(self, ZODB.interfaces.IBlobStorage)
113+
if ZODB.interfaces.IStorageLastPack.providedBy(changes):
114+
zope.interface.alsoProvides(self, ZODB.interfaces.IStorageLastPack)
112115
if close_changes_on_close is None:
113116
close_changes_on_close = True
114117

118+
if ZODB.interfaces.IStorageLastPack.providedBy(self):
119+
self.lastPack = changes.lastPack
120+
115121
self.changes = changes
116122
self.close_changes_on_close = close_changes_on_close
117123

src/ZODB/FileStorage/FileStorage.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@
5757
from ZODB.interfaces import IStorageIteration
5858
from ZODB.interfaces import IStorageRestoreable
5959
from ZODB.interfaces import IStorageUndoable
60+
from ZODB.interfaces import IStorageLastPack
6061
from ZODB.POSException import ConflictError
6162
from ZODB.POSException import MultipleUndoErrors
6263
from ZODB.POSException import POSKeyError
@@ -133,6 +134,7 @@ def __init__(self, afile):
133134
IStorageCurrentRecordIteration,
134135
IExternalGC,
135136
IStorage,
137+
IStorageLastPack,
136138
)
137139
class FileStorage(
138140
FileStorageFormatter,
@@ -145,6 +147,11 @@ class FileStorage(
145147

146148
# Set True while a pack is in progress; undo is blocked for the duration.
147149
_pack_is_in_progress = False
150+
# last tid used as pack cut-point
151+
# TODO save/load _lastPack with index - for it not to go to z64 on next
152+
# file reopen. Currently lastPack is used only to detect race of load
153+
# wrt simultaneous pack, so not persisting lastPack is practically ok.
154+
_lastPack = z64
148155

149156
def __init__(self, file_name, create=False, read_only=False, stop=None,
150157
quota=None, pack_gc=True, pack_keep_old=True, packer=None,
@@ -1183,6 +1190,11 @@ def packer(storage, referencesf, stop, gc):
11831190
finally:
11841191
p.close()
11851192

1193+
def lastPack(self):
1194+
"""lastPack implements IStorageLastPack."""
1195+
with self._lock:
1196+
return self._lastPack
1197+
11861198
def pack(self, t, referencesf, gc=None):
11871199
"""Copy data from the current database file to a packed file
11881200
@@ -1207,6 +1219,7 @@ def pack(self, t, referencesf, gc=None):
12071219
if self._pack_is_in_progress:
12081220
raise FileStorageError('Already packing')
12091221
self._pack_is_in_progress = True
1222+
stop = min(stop, self._ltid)
12101223

12111224
if gc is None:
12121225
gc = self._pack_gc
@@ -1245,6 +1258,8 @@ def pack(self, t, referencesf, gc=None):
12451258
self._file = open(self._file_name, 'r+b')
12461259
self._initIndex(index, self._tindex)
12471260
self._pos = opos
1261+
if stop > self._lastPack:
1262+
self._lastPack = stop
12481263

12491264
# We're basically done. Now we need to deal with removed
12501265
# blobs and removing the .old file (see further down).

src/ZODB/MappingStorage.py

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
@zope.interface.implementer(
3131
ZODB.interfaces.IStorage,
3232
ZODB.interfaces.IStorageIteration,
33+
ZODB.interfaces.IStorageLastPack,
3334
)
3435
class MappingStorage(object):
3536
"""In-memory storage implementation
@@ -186,13 +187,30 @@ def new_oid(self):
186187
self._oid += 1
187188
return ZODB.utils.p64(self._oid)
188189

190+
# ZODB.interfaces.IStorageLastPack
191+
@ZODB.utils.locked(opened)
192+
def lastPack(self):
193+
packed = self._last_pack
194+
if packed is None:
195+
return ZODB.utils.z64
196+
else:
197+
return packed
198+
189199
# ZODB.interfaces.IStorage
190200
@ZODB.utils.locked(opened)
191201
def pack(self, t, referencesf, gc=True):
192202
if not self._data:
193203
return
194204

195205
stop = ZODB.TimeStamp.TimeStamp(*time.gmtime(t)[:5]+(t%60,)).raw()
206+
# clip stop to last committed transaction.
207+
# don't use ._ltid as head - for MVCCMappingStorage ._ltid is specific
208+
# to current storage instance, not whole storage history.
209+
head = ZODB.utils.z64
210+
if self._transactions:
211+
head = self._transactions.maxKey()
212+
stop = min(stop, head)
213+
196214
if self._last_pack is not None and self._last_pack >= stop:
197215
if self._last_pack == stop:
198216
return

src/ZODB/interfaces.py

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
# -*- coding: utf-8 -*-
12
##############################################################################
23
#
34
# Copyright (c) Zope Corporation and Contributors.
@@ -749,6 +750,8 @@ def pack(pack_time, referencesf):
749750
extract object references from database records. This is
750751
needed to determine which objects are referenced from object
751752
revisions.
753+
754+
See also: IStorageLastPack.
752755
"""
753756

754757
def registerDB(wrapper):
@@ -876,6 +879,34 @@ def prefetch(oids, tid):
876879
more than once.
877880
"""
878881

882+
class IStorageLastPack(IStorage):
883+
884+
def lastPack(): # -> pack-cut-point (tid)
885+
"""lastPack returns ID of the last transaction used as pack cut point.
886+
887+
For a database view with at ≥ lastPack, the storage guarantees to
888+
persist all objects revisions to represent such view. For a database
889+
view with at < lastPack, the storage does not provide such guarantee.
890+
In particular pack can remove objects revisions that were non-current as
891+
of lastPack database view at pack time.
892+
893+
Similarly, the storage gurantees to persist all transactions in
894+
[lastPack, lastTransaction] range, while for [0, lastPack) range there
895+
is no such guarantee. In particular pack can remove transactions with
896+
only non-current objects revisions as of lastPack database view.
897+
898+
lastPack is non-decreasing - it can only grow, or stay equal over time.
899+
900+
lastPack is always ≤ IStorage.lastTransaction.
901+
902+
lastPack is related to pack_time passed to IStorage.pack - internally
903+
that time is converted to transaction ID format after clipping into
904+
valid range and looking up nearby transaction.
905+
906+
lastPack value cannot be cached - for client/storage case the call has
907+
to perform round-trip and synchronize with the server.
908+
"""
909+
879910

880911
class IMultiCommitStorage(IStorage):
881912
"""A multi-commit storage can commit multiple transactions at once.

src/ZODB/mvccadapter.py

Lines changed: 22 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -154,11 +154,12 @@ def load(self, oid):
154154
r = self._storage.loadBefore(oid, self._start)
155155
if r is None:
156156
# object was deleted or not-yet-created.
157-
# raise ReadConflictError - not - POSKeyError due to backward
158-
# compatibility: a pack(t+δ) could be running simultaneously to our
159-
# transaction that observes database as of t state. Such pack,
160-
# because it packs the storage from a "future-to-us" point of view,
161-
# can remove object revisions that we can try to load, for example:
157+
# raise POSKeyError, or ReadConflictError, if the deletion is
158+
# potentially due to simultaneous pack: a pack(t+δ) could be
159+
# running simultaneously to our transaction that observes database
160+
# as of t state. Such pack, because it packs the storage from a
161+
# "future-to-us" point of view, can remove object revisions that we
162+
# can try to load, for example:
162163
#
163164
# txn1 <-- t
164165
# obj.revA
@@ -168,15 +169,22 @@ def load(self, oid):
168169
#
169170
# for such case we want user transaction to be restarted - not
170171
# failed - by raising ConflictError subclass.
171-
#
172-
# XXX we don't detect for pack to be actually running - just assume
173-
# the worst. It would be good if storage could provide information
174-
# whether pack is/was actually running and its details, take that
175-
# into account, and raise ReadConflictError only in the presence of
176-
# database being simultaneously updated from back of its log.
177-
raise POSException.ReadConflictError(
178-
"load %s @%s: object deleted, likely by simultaneous pack" %
179-
(oid_repr(oid), tid_repr(p64(u64(self._start)-1))))
172+
if hasattr(self._storage, 'pack'):
173+
lastPack = getattr(self._storage, 'lastPack', None)
174+
if lastPack is not None:
175+
packed = lastPack()
176+
packConflict = (p64(u64(self._start)-1) < packed)
177+
else:
178+
packConflict = True # no lastPack information - assume the worst
179+
180+
if packConflict:
181+
# database simultaneously packed from back of its log over our view of it
182+
raise POSException.ReadConflictError(
183+
"load %s @%s: object deleted, likely by simultaneous pack" %
184+
(oid_repr(oid), tid_repr(p64(u64(self._start)-1))))
185+
186+
# no simultaneous pack detected, or lastPack was before our view of the database
187+
raise POSException.POSKeyError(oid)
180188

181189
return r[:2]
182190

src/ZODB/tests/MVCCMappingStorage.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ def new_instance(self):
4545
inst._commit_lock = self._commit_lock
4646
inst.new_oid = self.new_oid
4747
inst.pack = self.pack
48+
inst.lastPack = self.lastPack
4849
inst.loadBefore = self.loadBefore
4950
inst._ltid = self._ltid
5051
inst._main_lock = self._lock

src/ZODB/tests/PackableStorage.py

Lines changed: 107 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,14 @@
1616

1717
import doctest
1818
import time
19+
import warnings
20+
import gc
1921

2022
from persistent import Persistent
2123
from persistent.mapping import PersistentMapping
2224
from ZODB import DB
23-
from ZODB.POSException import ConflictError, StorageError
25+
from ZODB.POSException import ConflictError, StorageError, POSKeyError, \
26+
ReadConflictError
2427
from ZODB.serialize import referencesf
2528
from ZODB.tests.MinPO import MinPO
2629
from ZODB.tests.MTStorage import TestThread
@@ -528,6 +531,109 @@ def checkPackOnlyOneObject(self):
528531
eq(pobj.getoid(), oid2)
529532
eq(pobj.value, 11)
530533

534+
def checkPackVSConnectionGet(self):
535+
# verify behaviour of Connection.get vs simultaneous pack:
536+
#
537+
# For deleted objects, in normal circumstances, get should raise POSKeyError.
538+
# However when a pack was run simultaneously with packtime going after
539+
# connection view of the database, for storages that do not implement
540+
# IMVCCStorage natively, get should raise ReadConflictError instead.
541+
#
542+
# IMVCCStorage storages are not affected, since they natively provide
543+
# isolation, via e.g. RDBMS in case of RelStorage. The isolation
544+
# property provided by RDBMS guarantees that connection view of the
545+
# database is not affected by other changes - e.g. pack - until
546+
# connection's transaction is complete.
547+
db = DB(self._storage)
548+
eq = self.assertEqual
549+
raises = self.assertRaises
550+
551+
# connA is main connection through which database changes are made
552+
# @at0 - start
553+
tmA = transaction.TransactionManager()
554+
connA = db.open(transaction_manager=tmA)
555+
rootA = connA.root()
556+
rootA[0] = None
557+
tmA.commit()
558+
at0 = rootA._p_serial
559+
560+
# conn0 is "current" db connection that observes database as of @at0 state
561+
tm0 = transaction.TransactionManager()
562+
conn0 = db.open(transaction_manager=tm0)
563+
564+
# @at1 - new object is added to database and linked to root
565+
rootA[0] = objA = MinPO(1)
566+
tmA.commit()
567+
oid = objA._p_oid
568+
at1 = objA._p_serial
569+
570+
# conn1 is "current" db connection that observes database as of @at1 state
571+
tm1 = transaction.TransactionManager()
572+
conn1 = db.open(transaction_manager=tm1)
573+
574+
# @at2 - object value is modified
575+
objA.value = 2
576+
tmA.commit()
577+
at2 = objA._p_serial
578+
579+
# ---- before pack ----
580+
581+
# conn0.get(oid) -> POSKeyError (as of @at0 object was not yet created)
582+
errGetNoObject = POSKeyError
583+
if (not ZODB.interfaces.IMVCCStorage.providedBy(self._storage)) and \
584+
(not ZODB.interfaces.IStorageLastPack.providedBy(self._storage)):
585+
warnings.warn("FIXME %s does not implement lastPack" %
586+
type(self._storage), DeprecationWarning)
587+
errGetNoObject = ReadConflictError
588+
raises(errGetNoObject, conn0.get, oid)
589+
590+
# conn1.get(oid) -> obj(1)
591+
obj1 = conn1.get(oid)
592+
eq(obj1._p_oid, oid)
593+
eq(obj1.value, 1)
594+
595+
# --- after pack to latest db head ----
596+
db.pack(time.time()+1)
597+
598+
# IMVCCStorage - as before
599+
#
600+
# !IMVCCStorage:
601+
# conn0.get(oid) -> ReadConflictError
602+
# conn1.get(oid) -> ReadConflictError
603+
#
604+
# ( conn1: the pack removes obj@at1 revision, which results in conn1
605+
# finding the object in non-existent/deleted state, traditionally this
606+
# is reported as ReadConflictError for conn1's transaction to be
607+
# restarted.
608+
#
609+
# conn0: obj@at0 never existed, but after pack@at2 it is
610+
# indistinguishable whether obj@at0 revision was removed, or it never
611+
# existed -> ReadConflictError too, similarly to conn1 )
612+
conn0.cacheMinimize()
613+
conn1.cacheMinimize()
614+
del obj1
615+
gc.collect()
616+
if ZODB.interfaces.IMVCCStorage.providedBy(self._storage):
617+
raises(POSKeyError, conn0.get, oid)
618+
obj1 = conn1.get(oid)
619+
eq(obj1._p_oid, oid)
620+
eq(obj1.value, 1)
621+
622+
else:
623+
# !IMVCCStorage
624+
raises(ReadConflictError, conn0.get, oid)
625+
raises(ReadConflictError, conn1.get, oid)
626+
627+
# connA stays ok
628+
connA.cacheMinimize()
629+
objA_ = connA.get(oid)
630+
self.assertIs(objA_, objA)
631+
eq(objA_.value, 2)
632+
633+
# end
634+
self._sanity_check()
635+
db.close()
636+
531637
class PackableStorageWithOptionalGC(PackableStorage):
532638

533639
def checkPackAllRevisionsNoGC(self):

src/ZODB/tests/testConnection.py

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,20 @@ def doctest_get(self):
282282
Traceback (most recent call last):
283283
...
284284
POSKeyError: 0x01
285+
286+
A request for an object that doesn't exist yet as of connection view of
287+
the database will raise a POSKeyError too.
288+
>>> tm2 = transaction.TransactionManager()
289+
>>> cn2 = db.open(transaction_manager=tm2)
290+
>>> root2 = cn2.root()
291+
>>> obj2 = Persistent()
292+
>>> root2[2] = obj2
293+
>>> tm2.commit()
294+
295+
>>> cn.get(obj2._p_oid) # doctest: +ELLIPSIS
296+
Traceback (most recent call last):
297+
...
298+
POSKeyError: ...
285299
"""
286300

287301
def doctest_close(self):

src/ZODB/tests/testmvcc.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -386,7 +386,7 @@
386386
387387
We'll reuse the code from the example above, except that there will
388388
only be a single revision of "b." As a result, the attempt to
389-
activate "b" will result in a ReadConflictError.
389+
activate "b" will result in a POSKeyError.
390390
391391
>>> ts = TestStorage()
392392
>>> db = DB(ts)
@@ -413,7 +413,7 @@
413413
>>> r1["b"]._p_activate() # doctest: +ELLIPSIS
414414
Traceback (most recent call last):
415415
...
416-
ReadConflictError: ...
416+
POSKeyError: ...
417417
418418
>>> db.close()
419419
"""
@@ -427,7 +427,7 @@
427427
(re.compile("b('.*?')"), r"\1"),
428428
# Python 3 adds module name to exceptions.
429429
(re.compile("ZODB.POSException.ConflictError"), r"ConflictError"),
430-
(re.compile("ZODB.POSException.ReadConflictError"), r"ReadConflictError"),
430+
(re.compile("ZODB.POSException.POSKeyError"), r"POSKeyError"),
431431
])
432432

433433
def test_suite():

0 commit comments

Comments
 (0)