[WIP] MVCCAdapter.load: Raise ReadConflictError only if pack is running simultaneously

[navytux]

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

[navytux]

( fixed travis wrt ZOPE_INTERFACE_STRICT_IRO=1 )

[navytux]

ZEO cache coherency fix that was revealed by checkPackVSConnectionGet test added by this patch: https://github.com/zopefoundation/ZEO/pull/162.

[navytux]

Rebased on top of master since https://github.com/zopefoundation/ZODB/pull/320 was merged.

Reminder:

1. https://github.com/zopefoundation/ZODB/pull/320 removed pre-MVCC leftovers without change of semantic. It is prerequisite for this patch (change 2).
2. this patch: let's switch MVCCAdapter to raise POSKeyError when load cannot find the object, or sees that it was deleted, and only raise ReadConflictError if load actually detects simultaneous pack that overlaps with MVCCAdapter view of the database. This is implemented via teaching storages to report lastPack time. It is prerequisite for change 3.
3. let's add loadAt and switch ZODB codebase to it. This fixes DemoStorage corruption (https://github.com/zopefoundation/ZODB/issues/318) and offloads storage servers from doing unneccessary work on every object access. In particular this provides potential to reduce 2x number of SQL queries on every load for NEO (https://github.com/zopefoundation/ZODB/issues/318#issuecomment-657685745). This is https://github.com/zopefoundation/ZODB/pull/323 and was initial motivation for all this work.

/cc @jamadden, @vpelletier, @jimfulton, @vpelletier, @jmuchemb, @arnaud-fontaine, @gidzit, @klawlf82

[jmuchemb]

In NEO, we are about to implement partial pack, and even different ways of packing partially. This PR seems to be a deadend because lastPack won't be enough.

IStorage is a better place than MVCCAdapterInstance to raise the appropriate exception. For minimum compatibility breakage, I suggest the following changes:

• in MVCCAdapterInstance, just do s/ReadConflictError/POSKeyError/
• in IStorage.loadBefore, clarify that ReadConflictError should be raised if the deletion could result from some pack
• update storage implementations

In the worst case (old storage implementation), POSKeyError could be raised instead of ReadConflictError. Would it be a major issue ?

An alternative solution is to add a more generic method (rather than lastPack, which assumes too much about how pack is working). For example:

     def isBeingPacked(oid, before_tid): # same parameters as loadBefore

[navytux]

( @jmuchemb your feedback is not ignored - I just did not get back to this topic, hopefully yet. Sorry for the delay with replying )

[navytux]

I've reworked loadAt not to depend on this patch (https://github.com/zopefoundation/ZODB/pull/323#issuecomment-800482354). I would like to put this patch on hold for now and focus on merging loadAt first.