[ZODB4] IStorage: Require lastTransaction() to invalidate DB before returning ...
... and to provide correct "current" view for load(). This is ZODB4 backport of https://github.com/zopefoundation/ZODB/pull/313, which itself is just a more explicit language of https://github.com/zopefoundation/ZODB/commit/4a6b0283#diff-881ceb274f9e538d4144950eefce8682R685-R695 It has been amended with ZODB4-specific It is guaranteed that after lastTransaction returns, "current" view of the storage as observed by load() is ≥ returned tid. because on ZODB<5 - contrary to ZODB5 - there is "load current" for which correct semantic must be also provided: https://github.com/zopefoundation/ZODB/pull/307#discussion_r436662348 Original description follows: ---- 8< ---- Because if lastTransaction() returns tid, for which local database handle has not yet been updated with invalidations, it could lead to data corruption due to concurrency issues similar to https://github.com/zopefoundation/ZODB/issues/290: - DB refreshes a Connection for new transaction; - zstor.lastTransaction() is called to obtain database view for this connection. - objects in live-cache for this Connection are invalidated with invalidations that were queued through DB.invalidate() calls from storage. - if lastTransaction does not guarantee that all DB invalidations for transactions with ID ≤ returned tid have been completed, it can be that: incomplete set of objects are invalidated in live cache i.e. data corruption. This particular data corruption has been hit when working on core of ZODB and was not immediately noticed: https://github.com/zopefoundation/ZODB/pull/307#pullrequestreview-423017996 this fact justifies the importance of explicitly stating what IStorage guarantees are / must be in the interface. This guarantee - already holds for FileStorage (no database mutations from outside of single process); - is already true for ZEO4 and ZEO5 https://github.com/zopefoundation/ZODB/pull/307#pullrequestreview-423017996 https://github.com/zopefoundation/ZODB/pull/307#discussion_r434166238 - holds for RelStorage because it implements IMVCCStorage natively; - is *not* currently true for NEO because NEO sets zstor.last_tid before calling DB.invalidate: https://lab.nexedi.com/nexedi/neoppod/blob/fc58c089/neo/client/handlers/master.py#L109-124 However NEO is willing to change and already prepared the fix to provide this guarantee because of data corruption scenario that can happen without it: https://github.com/zopefoundation/ZODB/pull/307#discussion_r436662348 neoppod@a7d101ec neoppod@a7d101ec (comment 112238) neoppod@a7d101ec (comment 113331) In other words all storages that, to my knowledge, are in current use are either already providing specified semantic, or will be shortly fixed to provide it. This way we can fix up the interface and make the semantic clear. /cc @jamadden, @vpelletier, @arnaud-fontaine, @jwolf083, @klawlf82, @gitzit, @jimfulton
Showing with 13 additions and 0 deletions