DemoStorage: Add support for deleteObject (IExternalGC)

[navytux]

So that it is generally possible to zodbrestore[1,2] zodbdumped[3]
database/transactions into DemoStorage. Because without this patch, when dump
contains deletion records, e.g.

txn 0285cbacc06d3a4c " " 
user ""
description ""
extension ""
obj 0000000000000007 delete

it fails like this:

Traceback (most recent call last):
  ...
  File "/home/kirr/src/wendelin/z/zodbtools/zodbtools/zodbrestore.py", line 94, in main
    zodbrestore(stor, asbinstream(sys.stdin), _)
  File "/home/kirr/src/wendelin/z/zodbtools/zodbtools/zodbrestore.py", line 43, in zodbrestore
    zodbcommit(stor, at, txn)
  File "/home/kirr/src/wendelin/z/zodbtools/zodbtools/zodbcommit.py", line 131, in zodbcommit
    _()
  File "/home/kirr/src/wendelin/z/zodbtools/zodbtools/zodbcommit.py", line 118, in _
    stor.deleteObject(obj.oid, current_serial(obj.oid), txn)
AttributeError: 'DemoStorage' object has no attribute 'deleteObject'
    demo_test.go:105: demo:(file:///tmp/demo470143633/δ0285cbac258bf266/base.fs)/(file:///tmp/demo470143633/δ0285cbac258bf266/δ.fs): zpyrestore: exit status 1

To be able to implement DemoStorage.deleteObject, we have to fix
FileStorage.deleteObject first: currently FileStorage.deleteObject raises
POSKeyError if an object is not present in the database, but for situation
where

• demo=base+δ,
• object is present in base, and
• object is not present in δ

it creates a problem because there is no way to add whiteout record for the
object into δ.

This change should be generally good; let me quote
https://lab.nexedi.com/kirr/neo/commit/543041a3 for why:

---- 8< ----
Even though currently it is not possible to create such data record via
FileStorage(py).deleteObject (because it raises POSKeyError if there is
no previous object revision), being able to use such data records is
semantically useful in overlayed DemoStorage settings, where δ part
marks an object that exists only in base with delete record whiteout.

It is also generally meaningful to be able to create "delete" record
even if object was not previously existing: "deleteObject" is actually
similar to "store" (and so should be better named as "storeDelete"). If
one wants to store deletion, there should not be a reason to reject it,
because deleteObject already has seatbelt in the form of oldserial, and
if the user calls deleteObject(oid, oldserial=z64), he/she is already
telling that "I know that this object does not exist in this storage
(oldserial=z64), but still please create a deletion record for it". Once
again this is useful in overlayed DemoStorage settings described above.

For the reference, such whiteout deletion records pass ZODB/scripts/fstest just fine.
---- 8< ----

DemoStorage.deleteObject implementation is straightforward, but builds on
loadAt[4] as precondition.

/cc @jimfulton, @jamadden, @perrinjerome

[1] https://lab.nexedi.com/nexedi/zodbtools/blob/129afa67/zodbtools/zodbrestore.py
[2] https://lab.nexedi.com/nexedi/zodbtools/merge_requests/19
[3] https://lab.nexedi.com/nexedi/zodbtools/blob/129afa67/zodbtools/zodbdump.py
[4] #323

[navytux]

Needs loadAt (#323) and so includes it here.
The change itself is in commit 49f5959.
Probably WIP because testing coverage is not complete.

[navytux]

See also https://lab.nexedi.com/kirr/neo/commit/4fb6bd0a?expanded=1 for the reference.

[navytux]

/cc @d-maurer

[navytux]

I was running ZODB/go tests with forgetting to switch my ZODB/py checkout from master to my custom branch, and once again hit the problem of AttributeError: 'DemoStorage' object has no attribute 'deleteObject' as explained in the description. So I recalled about this pull request.

So is there anyone with feedback maybe?
@jamadden, @d-maurer?

Once again, hereby patch builds on top of #323 which, by the way, fixes data corruption problem of DemoStorage (#318).

[icemac]

Is there anyone able to review this PR?

[navytux]

Refreshed the code by syncing with #323 to resolve conflicts. As explained in the main message, DemoStorage.deleteObject implementation is straightforward, but builds on loadAt (now loadBeforeEx) as precondition.

@d-maurer, please note how loadAt -> loadBeforeEx renaming made the code somewhat less clear: c3820ff.

There, we are obtaining base.lastTransaction() and changes.lastTransaction() and then want to load data from storages at that particular states. After at -> before transition, we cannot use that lastTransaction return as is and need to do p64(u64(·)+1) all the time around it.

To me it is a common situation - when something determines revision as of which database state it wants to observe data snapshot, and then accesses/loads data at that particular revision. Frequently determining revision is done via .lastTransaction(), but it can be also done by other means. With loadAt accessing that snapshot is straightforward, while with loadBefore* we/users are forced to do p64(u64(·)+1) dance all the time.

I don't insist on this strongly, but maybe it would make sense to reconsider loadAt -> loadBeforeEx rename and rename loadBeforeEx back to -> loadAt before it is merged?

This will unify loading semantic with loadSerial (which loads at particular revision).
And actually we can also deprecate and eventually remove loadSerial, because in practice, usage of loadSerial can be almost directly replaced with loadAt.

Thanks beforehand for feedback,
Kirill

[navytux]

And I would appreciate if someone would look at the main patch in the PR: 49f5959.

[icemac]

@d-maurer Do you think you are able to review this PR? Or is there anyone else with deep enough ZODB knowledge?

[d-maurer]

Michael Howitz wrote at 2022-3-17 00:15 -0700: ***@***.*** Do you think you are able to review this PR? Or is there anyone else with deep enough ZODB knowledge? This PR depends on the `loadAt` PR (--> #323). I have approved this PR (after long discussions). But, @jamadden might have had some concerns (--> "#323 (comment)"), in particular he was unsure about the impact on `RelStorage`. PR #323 introduces a new storage method (`loadAt` or `loadBeforeEx`); @navitux has implemented it for the storages directly provided by `ZODB`; however other storages lack the new method and the provided fallback might not be efficient. If #323 is accepted, this PR is trivial and poses no further risk.

[navytux]

@icemac, @d-maurer, thanks for heads up and for feedback.

This patch is indeed small, hopefully safe, change, but it requires #323 to work.

Regarding potential impact of #323 on RelStorage - I believe I've addressed that with #323 (comment) and zodb/relstorage#485. However taking into account that @jamadden might be having a hard time, it is understandable that there is no feedback on this.

Another point is that personally, even though #323 is approved (@d-maurer thanks for the approval and for the discussion), I'm not so comfortable to merge it in loadBeforeEx form compared to merging it as loadAt. The reason here is that until we establish a public interface the freedom to change naming is still there, but once the interface is established it has to be kept in stone.

In my view loadAt is more descriptive and "speaking" name compared to loadBeforeEx. It is also more handy to use loadAt compared to loadBefore(Ex) because one does not need +1 transactionID obtained via e.g. lastTransaction. loadAt can also make loadSerial obsolete today, without any blockers.

Dieter, would you please try to review your decision to prefer loadBefore-style for new function compared to loadAt? I understand that you still might be keeping on your preferences, but maybe taking another look afresh might be helpful. Because you originally asked to change loadAt->loadBefore* in the hope to simplify the code and keep diff of changes minimal, but in the end it did not work like that. And also, like I said above, with loadAt it is straightforward to e.g. directly use tid, that lastTransaction returns, while with loadBefore that tid has to be converted to before = p64(u64(tid)+1).

Let me qoute #341 (comment) to explain the latter point:

---- 8< ----
@d-maurer, please note how loadAt -> loadBeforeEx renaming made the code somewhat less clear: c3820ff.

There, we are obtaining base.lastTransaction() and changes.lastTransaction() and then want to load data from storages at that particular states. After at -> before transition, we cannot use that lastTransaction return as is and need to do p64(u64(·)+1) all the time around it.

To me it is a common situation - when something determines revision as of which database state it wants to observe data snapshot, and then accesses/loads data at that particular revision. Frequently determining revision is done via .lastTransaction(), but it can be also done by other means. With loadAt accessing that snapshot is straightforward, while with loadBefore* we/users are forced to do p64(u64(·)+1) dance all the time.

I don't insist on this strongly, but maybe it would make sense to reconsider loadAt -> loadBeforeEx rename and rename loadBeforeEx back to -> loadAt before it is merged?

This will unify loading semantic with loadSerial (which loads at particular revision).
And actually we can also deprecate and eventually remove loadSerial, because in practice, usage of loadSerial can be almost directly replaced with loadAt.
---- 8< ----

So @d-maurer, would you please provide some feedback on this? I understand that you might reject the idea to return to loadAt naming, and probably I will accept and live with that, but it would be a pity not to use a more fitting version. That's why I'm asking for you to please review this once again from scratch.

Thanks beforehand for feedback,
Kirill

[navytux]

( synced to updated #323 to resolve conflict on CHANGES.rst )

[d-maurer]

Kirill Smelkov wrote at 2022-3-17 09:48 -0700:

... Dieter, would you please try to review your decision to prefer loadBefore-style for new function compared to loadAt?

I have not advocated for `loadBeforeEx` but to change (incompatibly) the behavior of `loadBefore` for delete records. The `loadBeforeEx` has been a compromise, you proposed; I dislike the name (likely as much as you do). `loadAt` is a good name -- but ZODB internals use the "before" semantics and forward and backwards adaptations are necessary to switch to `loadAt` -- or a rewrite of those ZODB internals. I am also author and maintainer of `dm.historical` which uses the "before" semantics (because the ZODB does). It would not be difficult to switch to "at" semantics but it is nasty to have code depending on the ZODB version.

I understand that you still might be keeping on your preferences, but maybe taking another look afresh might be helpful.

Personally, I do not see the need to change anything. Your change allows to fix a bug in `DemoStorage` (and other stacked storages) -- but this bug affects only "delete" records (which I never have used). I have also never used the stacking feature of `DemoStorage`. Thus, form my point of view, this is a bug which does not require fixing -- at least not at the cost of a new load method. Unlike you, I would not mind that `loadBefore` would change its behavior for "delete" records in an incompatible way.

[navytux]

@d-maurer, thanks for feedback.

Personally, I do not see the need to change anything.
...

To fix the data corruption bug is not the only motivation for me. As said in the description of #323 fixing data corruption for DemoStorage is only the first point. The second point is to save work for storages beside FileStorage to save time on every load by not unneedingly retrieving next_serial from disk/database. This change does affect regular - not only deletion - records. And that's why a new method is intoduced to avoid backward compatibility breakage to loadBefore.

---

Regarding ZODB internals - all that is needed to switch them from loadBeforeEx back to loadAt is to semantically revert the following patch: 805bf36f4442. I can do that in practically no time, and we already know that after that amendment, the whole amount of changes to ZODB internals of whole #323 will stay almost the same. So in my view ZODB internals are not keeping us to use "before" semantic.

After reading your message about dm.historical I was initially going to agree that if dm.historical uses "before" semantic via loadBefore, then it makes sense not to break that and keep #323 in current form as is.

But I looked at dm.historical sources and ... as I see it, nothing there uses loadBefore at all.

If I understood it correctly, dm.historical is published only in the form of tarballs on PyPI, So I will refer to relevant parts of dm.historical source code by quoting them here from what I obtained via dm.historical-2.0.4.tar.gz:

The first place that depends on "before" semantic is Connection class:

class Connection(Connection):
  def __init__(self, baseConnection, timeOrSerial, before=False):
    '''creates a (historical) connection based on *baseConnection* that delivers state at or before *timeOrSerial*.

    *baseConnection* must be an (open) ZODB connection. Its 'db' is used
    as our 'db' as well.

    *timeOrSerial* is either a time ('DateTime' or float/int) or a serial/transactionid ('string').
    '''

but it inherits from ZODB.Connection.Connection and passes timeOrSerial to that ZODB.Connection constructor as before=...:

    ts = None
    if isinstance(timeOrSerial, bytes): ts = TimeStamp(timeOrSerial)
    if isinstance(timeOrSerial, DateTime): timeOrSerial = timeOrSerial.timeTime()
    if isinstance(timeOrSerial, (int, float)):
      ts = TimeStamp(*(gmtime(timeOrSerial)[:5] + (timeOrSerial%60,)))
    db = baseConnection.db()
    c = super(Connection, self).__init__
    if "before" in getargspec(c).args:
      # this is a modern ZODB which directly supports historical connections
      if not before:
        # we must slightly increment "ts" as we need the state
        #   `at or before` while the connection gives us `before`.
        ts = ts.laterThan(ts)
      c(db, before=to_bytes(ts))
      self.open() 

this will continue to work irregardless of whether ZODB internally uses loadBefore, loadBeforeEx or loadAt, because after any form of #323 ZODB.Connection stays fully backward compatible!

---

The other place in dm.historical is _HistoricalStorageWrapper class. But it only uses .history and .loadSerial methods of a given storage. It does not use loadBefore at all! And nothing in dm.historical uses loadBefore at all !

This means that whatever we do - merge #323 with loadBeforeEx, or merge #323 with loadAt - dm.historical will continue to work without any change to its sources.

Does all this maybe change anything in your view, please?

Kirill

[d-maurer]

Kirill Smelkov wrote at 2022-3-17 11:35 -0700: ***@***.***, thanks for feedback.

> Personally, I do not see the need to change anything. > ... To fix the data corruption bug is not the only motivation for me. As said in the description of #323 fixing data corruption for DemoStorage is only the first point. The second point is to save work for storages beside FileStorage to save time on every load by not _unneedingly_ retrieving next_serial from disk/database. This change does affect regular - not only deletion - records. And that's why a new method is intoduced to avoid backward compatibility breakage to loadBefore.

I have not yet seen a statement about your proposal from the authors of those storages -- apart from some concerns regarding `relstorage` (maybe meanwhile resolved). We have discussed that ZEO uses the additional information (you want to drop) for its cache consistency. You might have a solution concept - but it is not yet implemented.

Regarding ZODB internals - all that is needed to switch them from loadBeforeEx back to loadAt is to semantically revert the following patch: 805bf36f4442. I can do that in practically no time, and we already know that after that amendment, the whole amount of changes to ZODB internals of whole #323 will stay almost the same. So in my view ZODB _internals_ are not keeping us to use "before" semantic.

I remember that you had (of course) implemented the "before -> at" semantic adaptations. They have been the reason for my initial objection -- the ZODB should either consistently use "at" or "before" semantics, not switch between the two. I advocated to stay with "before" semantics.

... But I looked at dm.historical sources and ... as I see it, nothing there uses loadBefore at all. ... This means that whatever we do - merge #323 with loadBeforeEx, or merge #323 with loadAt - dm.historical will continue to work without any change to its sources.

You are right: like most of my packages, `dm.historical` is very stable and it was a long time ago I actively worked on it. I was wrong to fear that your proposal could break something.

Does all this maybe change anything in your view, please?

No, it does not.

[navytux]

@d-maurer, thanks for feedback.

I have not yet seen a statement about your proposal from the authors of those storages

Well, when I'm speaking about NEO, I'm meaning to be the author of NEO/go version myself

https://navytux.spb.ru/~kirr/neo

I also contributed to, and have more-or-less thorough understanding of, NEO/py implementation.

We have discussed that ZEO uses the additional information
(you want to drop) for its cache consistency. You might have
a solution concept - but it is not yet implemented.

This is valid point. Without rework of ZEO cache loadBefore cannot be deprecated and dropped.

Will it be accepted if I implement what I had described in #323 (comment) and prove that it works well for ZEO and in general?

I remember that you had (of course) implemented the
"before -> at" semantic adaptations. They have been the reason
for my initial objection -- the ZODB should either
consistently use "at" or "before" semantics, not switch between
the two. I advocated to stay with "before" semantics.

I agree that consistency is a good goal. But ZODB is already inconsistent: loadBefore works on tid as "before", while e.g. loadSerial accepts tid as "at". And e.g. ZODB.DB - its public interface - accepts both "at" and "before" in its .open.

We cannot change the public interface due to backward compatibility. But we can make the internals consistent. Currently they are already inconsistent in between loadBefore and loadSerial. With loadAt and loadSerial everything will be consistently using "at" semantic. Of course, if loadBefore gets deprecated, which, once again, depends on ZEO cache rework.

You are right ...

Thanks for confirming.

No, it does not.

I'm sorry to hear that, but I understand your reasoning. It is fair to say that loadAt is not generally very useful until loadBefore cannot be deprecated for real because ZEO cache depends on it.

Once again, I'm willing to fix ZEO cache not to depend on next_serial sooner or later.

Kirill

[d-maurer]

Kirill Smelkov wrote at 2022-3-18 02:06 -0700:

... > We have discussed that ZEO uses the additional information > (you want to drop) for its cache consistency. You might have > a solution concept - but it is not yet implemented. This is valid point. Without rework of ZEO cache loadBefore cannot be deprecated and dropped. Will it be accepted if I implement what I had described in #323 (comment) and prove that it works well for ZEO and in general?

I cannot speak about whether it is accepted or not -- only about whether I might approve it or not. The general concept may work and I might approve an implementation. However, I need to see the concrete implementation (and what it entails) before my approval.

... I agree that consistency is a good goal. But ZODB is already inconsistent: loadBefore works on tid as "before", while e.g. loadSerial accepts tid as "at". And e.g. ZODB.DB - its public interface - accepts both "at" and "before" in its `.open`. We cannot change the public interface due to backward compatibility. But we can make the internals consistent. Currently they are already inconsistent in between loadBefore and loadSerial.

You are aware that your "at" (which acutally means "<= at") is different from the "at" of `loadSerical` (which means "== at"). Otherwise, we would not need a `loadAt` but could use `loadSerial`). I recognize that the ZODB uses "before" semantics for historical reasons: formerly, the first invalidation serial after a connection open defined the snapshot boundary amd the connection read the state *before* that boundary. Now, the MVCC adapter maintains information about the last transaction and the connection loads "at" this transaction boundary. For historical reasons this "at" is transformed into a "before". With your `loadAt`, the "before" must later be retransformed into "at" semantics. I much dislike this double transformation -- especially, because the `TimeStamp` does not provide an API for the back transformation. If we should switch to `Storage.loadAt`, we should rework the ZODB code to consistently use "at" semantics.

... Once again, I'm willing to fix ZEO cache not to depend on `next_serial` sooner or later.

`ZEO` and `FileStorage` are the only storages I am currently really interested in. Thus, my personal decisions depend on good support for them.

[navytux]

@d-maurer, thanks for feedback. It is all clear and reasonable. I understand and agree that FileStorage and ZEO should remain with good support, as those storages are historically the most widely used. And that it requires to have the concrete implementation to be able to review it and its consequences. So let's wait for me to do this next step. I do not promise it will be soon, but, hopefully, I should be able to do it.

P.S. and you are right - loadSerial has ==at semantic while loadAt has <=at semantic. However loadSerial can be also replaced with loadAt usage - it is just loadAt(at) + assert that loaded serial == at. So in the end ZODB code could be using at everywhere internally and only loadAt.