loadAt

[navytux]

loadAt is new optional storage interface that is intended to replace loadBefore
with more clean and uniform semantic. Compared to loadBefore, loadAt:

1. returns data=None and serial of the removal, when loaded object was found to
   be deleted. loadBefore is returning only data=None in such case. This loadAt
   property allows to fix DemoStorage data corruption when whiteouts in overlay
   part were not previously correctly taken into account.

   DemoStorage does not take whiteouts into account -> leading to data corruption #318

2. for regular data records, does not require storages to return next_serial,
   in addition to (data, serial). loadBefore requirement to return both
   serial and next_serial is constraining storages unnecessarily, and,
   while for FileStorage it is free to implement, for other storages it is
   not - for example for NEO and RelStorage, finding out next_serial, after
   looking up oid@at data record, costs one more SQL query:

   https://lab.nexedi.com/nexedi/neoppod/blob/fb746e6b/neo/storage/database/mysqldb.py#L484-508
   https://lab.nexedi.com/nexedi/neoppod/blob/fb746e6b/neo/storage/database/mysqldb.py#L477-482

   https://github.com/zodb/relstorage/blob/3.1.1-1-ge7628f9/src/relstorage/storage/load.py#L259-L264
   https://github.com/zodb/relstorage/blob/3.1.1-1-ge7628f9/src/relstorage/adapters/mover.py#L177-L199

   next_serial is not only about execution overhead - it is semantically
   redundant to be there and can be removed from load return. The reason
   I say that next_serial can be removed is that in ZODB/py the only place,
   that I could find, where next_serial is used on client side is in client
   cache (e.g. in NEO client cache), and that cache can be remade to
   work without using that next_serial at all. In simple words whenever
   after

     loadAt(oid, at)  ->  (data, serial)
   

   query, the cache can remember data for oid in [serial, at] range.

   Next, when invalidation message from server is received, cache entries,
   that had at == client_head, are extended (at -> new_head) for oids that
   are not present in invalidation message, while for oids that are present
   in invalidation message no such extension is done. This allows to
   maintain cache in correct state, invalidate it when there is a need to
   invalidate, and not to throw away cache entries that should remain live.
   This of course requires ZODB server to include both modified and
   just-created objects into invalidation messages

   ( Include both modified and just created objects into invalidations ZEO#160 ,
   interface: Require invalidations to be called with full set of objects and not to skip transactions #319 ).

   Switching to loadAt should thus allow storages like NEO and, maybe,
   RelStorage, to do 2x less SQL queries on every object access.

   DemoStorage does not take whiteouts into account -> leading to data corruption #318 (comment)

In other words loadAt unifies return signature to always be

   (data, serial)

instead of

   POSKeyError                          object does not exist at all 
   None                                 object was removed
   (data, serial, next_serial)          regular data record

used by loadBefore.

This patch:

• introduces new interface.
• introduces ZODB.utils.loadAt helper, that uses either storage.loadAt,
  or, if the storage does not implement loadAt interface, tries to mimic
  loadAt semantic via storage.loadBefore to possible extent + emits
  corresponding warning.
• converts MVCCAdapter to use loadAt instead of loadBefore.
• changes DemoStorage to use loadAt, and this way fixes above-mentioned
  data corruption issue; adds corresponding test; converts
  DemoStorage.loadBefore to be a wrapper around DemoStorage.loadAt.
• adds loadAt implementation to FileStorage and MappingStorage.
• adapts other tests/code correspondingly.

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

[navytux]

( fixed travis wrt ZOPE_INTERFACE_STRICT_IRO=1 )

[navytux]

Fixed CI on Windows by not forgetting to close test database before its removal. Python38 failure is due to "Failed to connect to github.com port 443" and so should be transient.

interdiff

--- a/src/ZODB/tests/testDemoStorage.py
+++ b/src/ZODB/tests/testDemoStorage.py
@@ -357,6 +357,10 @@ def getObjAt(at):
         self.assertEqual(getObjAt(atUnlink), 2)
         self.assertRaises(ZODB.POSException.POSKeyError, getObjAt, atGC)
 
+        # end
+        db.close()
+        zdemo.close() # closes zbase and zoverlay as well
+
 
 def test_suite():
     suite = unittest.TestSuite((

[navytux]

( rebased on top of master since #320 was merged )

[navytux]

I decoupled this patch from #322 - that ReadConflictError -> POSKeyError switch requires more involvement for which I don't currently have time. On the other hand existing behaviour could be still preserved with careful programming around loadAt usage in MVCCAdapter.load. All ZODB tests pass now - without complete switch to POSKeyError - including checkPackNowWhileWriting and friends.

Once again loadAt is needed to fix DemoStorage data corruption (#318), and opens the door to do 2x less SQL queries on every load for NEO and RelStorage (see #318 (comment)). I also need loadAt as the basis for further work on DemoStorage enhancements.

@jimfulton, @jamadden, @vpelletier, @jmuchemb, @arnaud-fontaine, @gidzit, @klawlf82, @hannosch, please take a look.

Thanks beforehand,
Kirill

[navytux]

loadAt is also needed for DemoStorage.deleteObject (#341).

[navytux]

For the reference - see also https://lab.nexedi.com/kirr/neo/commit/4fb6bd0a?expanded=1 which proides DemoStorage-analogue for ZODB/go with Load organization similar to loadAt-based DemoStorage as implemented in this pull-request.

[navytux]

/cc @d-maurer

[navytux]

Anyone?

[d-maurer]

The ZODB logic relating to historical data (including MVCC) was largely centered around before. You have changed this to at - requiring wide spread modifications. I would much prefer to keep the before centered approach and implement an optimized version of loadBefore (for backward compatibility under a new name or with a new keyword parameter) instead of introducing a new loadAt.

I suggest to keep loadBefore (without deprecation), define loadBefore_opt with streamlined return value and implement it by default in BaseStorage via loadBefore and change calls to loadBefore to calls to loadBefore_opt in the ZODB code base where the third return value of loadBefore is not necessary.

[d-maurer]

I would prefer raise NotImplementedError over not defining the methods.

[navytux]

ok

[navytux]

-> see navytux@d968a9a8

[d-maurer]

Kirill Smelkov wrote at 2021-5-11 10:29 -0700: ***@***.*** commented on this pull request. ***@***.*** thanks for feedback.

> My favorite solution would be: > - change the `loadBefore` specification regarding the handling of object deletion records. This would make external storage implementations non-compliant but we could adapt the primary user of the method to ensure that this does not cause worse behavior than the current one. > - support a simplified loadBefore (no third result component) variant as an optional optimization. Would you please clarify this? I, frankly, don't understand how it will fully work. Even if we change ZODB code to expect that loadBefore can return both `None` or (`None`, `serial`), there are other storages - wrapper storages that I cited above - that call loadBefore and check for `None`. Do you suggest we change them all as well?

I had not expected many external (outside `ZODB`) users of `loadBefore`. I have not yet seen "deletion records" -- seems to be a newer concept, potentially not yet widely used. Maybe, we can break the interface with a version bump and an incompatibility note in the documentation?

... Maybe I'm missing something, but I don't see how we could change the interface without breaking things somewhere.

All you did was initiated by a `DemoStorage` problem. You used this to demontrate that the `loadBefore` lacks information for "deletion record"s. I am in favor to fix this.

That's why I suggest to keep existing interface intact and, for the new behaviour, introduce new interface. Once again, maybe I'm missing something, so would you please explain your idea in more detail?

You have understood my proposal - no need to describe it again. It will break the interface with potential consequences for users. Let's hear what those (or others) have to say.

... > > In zopefoundation/ZEO#174 I argued that it is not necessary to have atomic invalidation > > processing for the ZEO client cache because "normal" (MVCC) loads > > load only earlier object state and will not see partially invalidated > > cache state. > > ... > > Those operations may observe partial invalidation processing. > > I do not exclude that this can have bad effects. > While this problem was noticed via looking at the new code added by this PR, > there are existing places in ZODB5 that use this approach: > ... > - `load_current` is also implemented via `loadBefore(maxtid)`: > ... > and `load_current` is used throughout ZODB in many places: > > https://github.com/search?q=load_current+repo%3Azopefoundation%2FZODB&type=code > > I guess the only solution to fix this is to return to process invalidations > atomically, like I was suggesting during review of zopefoundation/ZEO#167. Dieter, would you please comment on this?

I am again convinced that the loads mentioned in my original comment do not require atomic invalidation processing: they ignore the returned data and only look for the serial. If the information comes form the cache, the result is the same as if invalidation processing had been slightly postponed; If a server load is necessary, it is automatically delayed until invalidation processing is terminated (as invalidation processing and server load are performed by the (mostly sequential) IO thread). I also do not think that `load_current` requires atomic invalidation processing: `load_current` affects a single oid; invalidation processing for other oids should not matter as far as `load_current` is concerned. Thus, as long as invalidation processing for a single oid is atomic (ensured by the cache lock), `load_current` should be safe.

[d-maurer]

@navytux wrote:

... and that cache can be remade to
    work without using that next_serial at all. In simple words whenever
    after
    ```
      loadAt(oid, at)  ->  (data, serial)
    ```
    
    
    query, the cache can remember data for oid in [serial, at] range.
    Next, when invalidation message from server is received, cache entries,
    that had at == client_head, are extended (at -> new_head) for oids that
    are not present in invalidation message,

This will not give the same cache performance as before: formerly, the cache had precise information about the currency of the object revision and for non current revisions about its validity range. Now an object revision is considered current only if it had been loaded for tid >= cache_head(at load time) (and was not invalidated since that time); however, loads for tid < cache_head can retrieve current revisions as well which is no longer recognized. For revisions loaded with tid < cache_head, the validity range is only approximated which may result in additional loads in the future.

I do not know whether the performance loss is significant in practice because most loads are likely to be for tid >= cache_head. Nevertheless, I see the potential loss as an argument against the deprecation of loadBefore; storages which can determine the exact validity range efficiently should be able to provide this information also in the future.

[navytux]

@d-maurer thanks for feedback, and first of all I apologize for the delay with replying on my side.

While, probably, it was my fault not to present things clearly, let me explain
why returning only (data, serial) without next_serial should not decrease
cache performance for current - non-historical - connections:

Imagine the situation when the database is under constant change. Transactions
are committed on server and propagate via invalidations to client.
Client.last_tid is potentially a bit behind on-server last_tid. When an
on-client thread opens a Connection, that connection gets assigned an at
that denotes the Connection's snapshot of the database state. Connection.at,
is potentially a bit behind Client.last_tid, because during the time when
that client transaction is processed, client could receive some further
invalidations from the server. At any time, there might be several "current"
on-client Connections that potentially all have different Connection.at.

In order to be efficient with putting entries into the cache, the client
maintains ΔTail that keeps history of all ZODB invalidations from
Cache.head till the "at" of its oldest "current" Connection. Then, when an
object is loaded for a connection with Connection.at < Cache.head, the
client looks into ΔTail in (Connection.at, Cache.head]
range to find out what is the first next serial that changed the object. If
there is an entry - that entry's serial is used as next_serial when putting
data into the cache after the call to load. If there is no such an entry - it
means that the data, that were loaded via at < Cache.head, is still current
even at Cache.head state, and so the data can be put into the cache with
next_serial=None.

    ΔTail.tail                            ΔTail.head

       ↓                                      ↓
────────────────────────────────────────────────────────────────────
                ↑            ↑                ↑                ↑

              conn₁        conn₂          Cache.head        on-server
                                              =             last_tid
                                          Client.last_tid

This organization makes sure that for "current" connections - that were not
opened in historical mode - the cache will be updated in exactly the same way
as it would be updated if next_serial is returned from load.

Yes, for historical connections - that are opened via DB.open(at/before=...)
and are maintained in DB.historical_pool instead of DB.pool for "current"
Connections - the update of the cache will be less efficient compared to the
situation when next_serial is provided by load: the next_serial of the
cache entry would be set as Connection.at instead of real next_serial, thus
potentially forcing another load to happen if another historical connection is
opened with different at. However, in my experience, historical connections
are seldomly used, and when used, at for only one particular point in time is
used to investigate a particular database state. It is thus the performance of
"current" access that matters the most.

For the reference: support for historic connections was added in late 2007 in 5f2b2d70a18a.
For the reference 2: I was talking a bit about ΔTail in #290 (comment).

---

I have not yet seen "deletion records" -- seems to be a newer concept

Deletion records were added in late 2008 in d2d83b88131b (see also http://web.archive.org/web/20120426053905/http://wiki.zope.org/ZODB/ExternalGC).

I had not expected many external (outside ZODB) users of loadBefore.
...
Maybe, we can break the interface with a version bump and an incompatibility note in the documentation?

You have understood my proposal - no need to describe it again.
It will break the interface with potential consequences for
users. Let's hear what those (or others) have to say.

I really don't understand why we need to break things to fix other things,
especially when compatibility-preserving solution is possible.

Making a major version change with compatibility breakage also does not work
well in my experience. If the new versions, besides breaking compatibility,
provides other benefits - that might somewhat work, but in our case the benefit
of fixed DemoStorage wrt deletion records is imho low for an average ZODB user,
while the downsides of compatibility breakage are high. This approach also
reminds me about Python2/Python3 situation where many companies are still
using Python2 because a) porting to Python3 has non-small cost, and b) this
porting does not usually bring any significant benefit. And all that more than
10 years after Python3 was released.

For the reference, where I work ZODB4
is still being used, because, even if it was not intended to be a compatibility breakage, the switch to pickle protocol=3 broke things in one major place for us. This, hopefully, will be addressed, but I would like to stress that changes in behaviour expose cost on users, especially changes that are intentionally backward-incompatible.

Personally I do not need, nor want, to make an experiment to see what happens if we break things.
I know beforehand that people will scream or silently get annoyed and pin ZODB to an older release.
I know the outcome of such experiment will be "this should not be the way to evolve a software".

Once again I really don't understand why we need to break things to fix other things,
especially when compatibility-preserving solution is possible.

Kirill

[navytux]

I have not yet seen "deletion records" -- seems to be a newer concept

Deletion records were added in late 2008 in d2d83b88131b (see also http://web.archive.org/web/20120426053905/http://wiki.zope.org/ZODB/ExternalGC).

For the reference: deletion records can also appear without any external garbage-collection - as a result of regular undo of object creation.

[navytux]

For the reference: deletion records can also appear without any external garbage-collection - as a result of regular undo of object creation.

They were known and handled at least from 2002 - see 6c7d0c429b19 and:

ZODB/src/ZODB/FileStorage/fsdump.py

Lines 35 to 38 in 1fb097b

	for j, rec in enumerate(trans):
	if rec.data is None:
	fullclass = "undo or abort of object creation"
	size = ""

[d-maurer]

Kirill Smelkov wrote at 2021-5-21 05:31 -0700:

... let me explain why returning only `(data, serial)` without `next_serial` should not decrease cache performance for current - non-historical - connections: Imagine the situation when the database is under constant change. Transactions are committed on server and propagate via invalidations to client. `Client.last_tid` is potentially a bit behind on-server `last_tid`. When an on-client thread opens a `Connection`, that connection gets assigned an `at` that denotes the Connection's snapshot of the database state. `Connection.at`, is potentially a bit behind `Client.last_tid`, because during the time when that client transaction is processed, client could receive some further invalidations from the server. At any time, there might be several "current" on-client `Connection`s that potentially all have different `Connection.at`. In order to be efficient with putting entries into the cache, the client maintains `ΔTail` that keeps history of all ZODB invalidations from `Cache.head` till the "at" of its oldest "current" `Connection`. Then, when an object is loaded for a connection with `Connection.at` < `Cache.head`, the client looks into `Tail` in `(Connection.at, Cache.head]` range to find out what is the first next serial that changed the object. If there is an entry - that entry's serial is used as `next_serial` when putting data into the cache after the call to load. If there is no such an entry - it means that the data, that were loaded via `at` < `Cache.head`, is still current even at `Cache.head` state, and so the data can be put into the cache with `next_serial=None`.

What are you describing above? How things currently work or how things could be changed to be efficient without "end_tid" information? I have looked at the ZEO [5] code and have not found a "tail": the cache is updated in a completely straight forward way which relies on `end_tid` to distinguish current from "historical" data. What you are calling "client" above is actually a layered entity where the cache is on a low (storage) layer and `Connection.at` is maintained at a higher (`MVCCAdapter[Instance]`) layer. Currently, the `MVCCAdapter` layer does not seem to have the API to implement the scheme above. Likely, this should even be implemented by the `MVCCAdapter` layer because all storages with an associated cache would need it. I believe (with you) that it is possible to get cache management efficient even without "end_tid" information. Unlike you, I fear that it requires significant (architectural) changes. And until those changes have been done, `loadBefore` should not get deprecated: `FileStorage` is still a major base storage and can deliver `end_tid` information without any performance loss.

... I really don't understand <ins>*why we need to break things to fix other things*</ins>, especially when compatibility-preserving solution is possible.

The solution you currently have deprecates `loadBefore` in favor of a method which does not provide validity range information. As far as I see it, ZEO [5] currently relies on such information for efficient cache management. I see your argument that for some storages validity range information is costly (and used only for special cases). But this does not apply for e.g. `FileStorage`. The use of its `loadBefore` should not be deprecated until validity range information is no longer used for cache management.

... in our case the benefit of fixed DemoStorage wrt deletion records is imho low for an average ZODB user,

Indeed, this may contribute to my reluctance to approve your PR: it is quite large and proposes a major storage API change including the deprecation of one of its central methods -- mainly to fix the marginal problem above. Of course, I, too, have proposed an (even more drastic) API change involving potential breakage: you have found an API weakness (there are cases when it is important to distinguish between an unknown object and one explicitly deleted and this is not API supported) and I thought is a good idea to fix this weakness. When I made the proposal, I was unaware of the API callers outside the ZODB. It would be trivial for the authors of the respective code to adapt to the change (handle `data is None` in a straight forward way); thus, I still feel it is a good idea - even though it is backward incompatible.

[navytux]

@d-maurer, thanks for feedback.

I indeed explained how things could be changed to work without next_serial
information, and you are right that currently there is nothing like that
implemented in ZEO: ZEO indeed currently uses end_tid to update its cache.

You are, again, right, that the caching layer, that I explained, is indeed
generic, and could be implemented as generic IStorage wrapper inside ZODB itself.

I believe (with you) that it is possible to get cache
management efficient even without "end_tid" information.

Thanks.

Unlike you, I fear that it requires significant (architectural) changes.
And until those changes have been done, loadBefore should not
get deprecated: FileStorage is still a major base storage and can
deliver end_tid information without any performance loss.

You are probably right here. Until we don't have that efficient caching scheme
implemented, loadBefore should remain in non-deprecated state.

I cannot allow myself to break things, but I have amended my change to
undeprecate loadBefore:

navytux@5ae48fe1d933

Hopefully this could help us to find some common ground.

Kirill

P.S. And DemoStorage is not only for demonstrations. DemoStorage provides generic storage overlaying, similarly to e.g. overlayfs. Besides demonstrations, such overlaying is used e.g. in tests - to create fresh ZODB instance for each test quickly - and could be used in other similar situations.

[navytux]

@d-maurer, thanks for approval. @jamadden, do you want to provide any input on the changes?

[icemac]

This PR is already approved, is there still something to do before merging it?

[navytux]

@icemac, thanks for asking.

@jamadden, 6 months ago, at #318 (comment), you said you "hope to find time to examine these PRs in depth "soon."

5 months ago, at #323 (comment) after @d-maurer's approval, I've asked whether you want to provide any input on this patch.

Are you still interested in reviewing it?

We can wait if you still plan to have a look.
Alternatively, if things changed on your side, it does not make sense to wait any longer.

So could you please explicitly state what's in your mind regarding this pull request.

Thanks beforehand for feedback,
Kirill

[navytux]

( PR refreshed by syncing to master to resolve trivial conflict on CHANGES.rst )

[jamadden]

I appreciate the reminder, and I apologize for the no-show; there are various personal and professional reasons I haven't found the time to review this PR in depth. I will do my best to look at it within the next week; feel free to move ahead without me if that doesn't happen.

I have some vague memories about some concerns I might have had:

• I'm not thrilled about the proliferation of Yet Another Load Method. In my (not very considered yet, and certainly forgetful) opinion, if this is the way forward, it should be the only load method (maybe that needs more work to require storages to properly support MVCC? Or the wrapper that implements it for them might need to work with storages that implement a different interface?). Yeah, it breaks backwards compatibility, but just call it 6.0 and be done with it. The overall simplification in all storage code is probably worth it. We're still supporting Python 2.7 with this, so there should be few migration blockers.
• It wasn't clear to me the impact this would have on RelStorage, since its connections are already internally consistent and have no way to load data from the future; an arbitrary "at" request is impossible to fulfill.

[icemac]

I created some merge conflicts by merging #357, sorry.

[jamadden]

OK, I think I'm allowed to relay this now: The company that I co-founded and which sponsored all my open source work for the last decade or so…will, umm...not be doing that…or any development work. It's been all-hands-on-deck to focus on things that could prevent that, but at this point it seems unlikely.

I still care, deeply, about these projects. They're important to me on a personal level, and I think they are important to the wider community. But my time, at this point, is no longer going to be subsidized to work on them. I sincerely hope that changes! But in the meantime, I will have to focus on finishing the most important work I had outstanding to the community (currently I think that means gevent and greenlet), and then I have to find some way to pay the bills.

I am deeply sorry for any turmoil this has caused. I didn't want that.

[jimfulton]

On Wed, Nov 10, 2021 at 3:53 PM Jason Madden ***@***.***> wrote: OK, I think I'm allowed to relay this now: The company that I co-founded and which sponsored all my open source work for the last decade or so…will, umm...not be doing that…or any development work. It's been all-hands-on-deck to focus on things that could prevent that, but at this point it seems unlikely.

I'm very sorry to hear that. You've done a ton of great work! Thank you!!! Jim

…

-- Jim Fulton http://jimfulton.info

[navytux]

@jamadden thanks for feedback. And I'm very sorry to hear that things are unfortunate on your side.
Let's hope this situation could improve for the better.

In the meantime please feel to skip what I will post below for the record.

---

• loadBeforeEx intention was to replace loadBefore and deprecate it. It turned out we cannot deprecate loadBefore until ZEO client cache is reworked not to use next_serial. It is clear how to do this rework, but it was not done. For this reason loadBefore stays in undeprecated state for now:

  loadAt #323 (comment)
  loadAt #323 (comment) (first part)
  loadAt #323 (comment) (first part)
  loadAt #323 (comment)

• I try not to break backward compatibility. One of the motivation for loadAt/loadBeforeEx is to fix DemoStorage does not take whiteouts into account -> leading to data corruption #318. I find it reasonable not to break things to fix other things, especially when backward-compatible solution is possible. The other reason is that besides ZODB and main storages (FileStorage, ZEO, NEO and RelStorage), there are many third-party places which depend on loadBefore being provided and particular semantic. It is not practical to discover and go through all those places and change them all. Also personally I find major breaking changes to be disasters, so unless such a change brings in some huge benefit, breaking backward compatibility is not worth it. In other words breaking backward-compatibility has its cost and this cost should be evaluated against expected gain. All this reasons were explained in detail in

  loadAt #323 (comment) (first part)
  loadAt #323 (review) (first part)
  loadAt #323 (comment) (third part)

• the impact for RelStorage should be minimal. RelStorage already provides loadBefore, and loadBeforeEx can be implemented similarly. This implementation will have the same properties as loadBefore regarding your concern. I might be misunderstanding something but there is something like "stale handling" in RelStorage which loadBefore utilizes to detect time when SQL connection view needs to be refreshed. Once again even if RelStorage provides MVCC natively, loadBefore is already there and loadBeforeEx can be added the same way. Taking recent news into account I went ahead and sketched it myself:

  Provide loadBeforeEx zodb/relstorage#485

---

@icemac, no problem. I resolved those conflicts.

Kirill

[navytux]

A bit of argument to maybe reconsider loadAt -> loadBeforeEx renaming back: #341 (comment).

[navytux]

( synced to master to resolve conflict on CHANGES.rst )