[ZODB-Dev] RE: [Zope-Annce] ZODB 3.2.4 release candidate 1released

[Chris McDonough]

Thanks again for taking the time on this...

On Wed, 2004-09-15 at 15:52, Tim Peters wrote:
> It's too late for 3.3(.0), and since this would be a major incompatible
> change (not "a bugfix") anyway, it would have to target 3.4 (not 3.3.1).
> Note that the storage API documents that storages raise KeyError.  Most
> storage implementations would have to change, and so would all code mucking
> with storages (like Connection.py).  There are lots of storages that do
> raise KeyError (not POSKeyError) now; inside ZODB, FileStorage is the only
> storage that raises POSKeyError instead of plain KeyError.  I agree it would
> be better if everything did switch to POSKeyError, though.

OK, so I will do nothing then for now.

< example of btrees exception case >

> > This symptom has in the past been attributable to mutating the items in a
> > BTree while iterating over a BTreeItems doodad.  But I don't do that
> > anymore (note the list())  and I'm wondering if this sounds like a
> > plausible series of events under Zope-2.7.2 + its version of ZODB that
> > might explain it:
> 
> Not really, but I'll give you a concrete, demonstrably broken scenario
> later.  A key missing detail in the above is whether iobtree ever gets
> committed, or whether it's solely in memory over its lifetime.

Sorry, iobtree does get committed at various points over its lifetime
but it's actually unknown whether it gets committed between the time a
set of keys are added to it and when the key error happens for a key
that is part of that set.  I haven't been able to reproduce it
(unhelpful, I realize, but true).

> > - a reference to a soon-to-be-unreachable object is created
> >   via the "pending modifications in subtransactions reused
> >   from cache when txn not committed or aborted" bug.  This
> >   object is referenced in a leaf node by a bucket of an
> >   IOBTree and its key is also kept by the IOBTree itself in
> >   its interior node structure.
> 
> Not entirely clear what you have in mind there, but the plausible readings I
> came up with flop because this is an IOBTree, and the test case only looks
> at its *keys*.  Integer keys aren't persistent objects in their own right,
> they're embedded directly in IO bucket and btree nodes.

Yup, I didn't know if the value related to the missing key needed to be
materialized at all given the example.  "No" is the answer I presume.

> > - The set of keys is iterated over and __delitem__ is called on
> >   the BTree with each key.  When it reaches the ghosted object and
> >   tries to unghost it, a POSKeyError is generated.  (does it *need*
> >   to unghost it?)
> 
> The only relevant things that *can* be ghosted here are IOBTree and IOBucket
> nodes.  Both certainly have to be unghosted to peer into their contents.

OK, it's helpful to know what must be unghosted to do this operation in
any case.

> > - Whatever code implements __delitem__ on a BTree catches the
> >   POSKeyError and reraises it as a KeyError
> 
> That one is unlikely in the C code, despite that Python dicts are unable to
> report errors correctly.  The BTree lookup code has nothing in common with
> dict lookup code; there's no try/except in C, so suppressing errors by
> accident is less likely than when writing in Python; and, most importantly,
> while you don't have to pay much attention to what is and isn't persistent
> in Python code, in C the distinction *always* has to made, on every line of
> C code.  Unghostifying doesn't happen by magic at the C level, you have to
> call something explicitly to make that happen.  So POSKeyError from an
> attempted load at the C level isn't "hiding", it punches you in the face.
> Every line of C code that needs to look inside a persistent object has
> something like this first:
> 
>     PER_USE_OR_RETURN(self, NULL);
> 
> It's remarkably unhelpful to know what that expands to:
> 
> 	{if((self)->state==cPersistent_GHOST_STATE &&
>        cPersistenceCAPI->setstate((PyObject*)(self)) < 0)
>            return (NULL);
>        else if ((self)->state==cPersistent_UPTODATE_STATE)
>           (self)->state=cPersistent_STICKY_STATE;
>       };
> 
> That point is that failure to load is a very distinct cause of failure in
> C-level code.

Right.  Thanks for the explanation, also very helpful.

Curious if this code (present in BTree_getm of BTreeTemplate.c) could do
arguably the wrong thing in the face of a POSKeyError:

  UNLESS (PyArg_ParseTuple(args, "O|O", &key, &d)) return NULL;
  if ((r=_BTree_get(self, key, 0))) return r;
  UNLESS (PyErr_ExceptionMatches(PyExc_KeyError)) return NULL;

>From a quick reading of the docs a PyErr_ExceptionMatches also compares
true if a base class matches.  I mean, I guess it doesn't matter much
(eventually you get None back).  But it'd be nice to know if a
POSKeyError resulted from this call instead of hiding it unintentionally
(if that's indeed what it does).  But probably not critical at all.

> > <waves hands furiously>
> 
> Here's code that breaks, provided you use a pre-repaired ZODB 3.2.  It's a
> minor variant of code I posted before.
> 
> import ZODB
> from BTrees.IOBTree import IOBTree
> from ZODB.FileStorage import FileStorage
> 
> st = FileStorage("temp.fs")
> db = ZODB.DB(st )
> cn = db.open()
> rt = cn.root()
> 
> tree = rt['tree'] = IOBTree([(1, 2), (2, 4)])
> get_transaction().commit()
> 
> # It's vital here that *some* state of `tree` got
> # committed.  Then when phantom cache state disappears
> # later, instead of getting a POSKeyError, we magically
> # get back the old state.
> 
> # Do the fatally confused "subtxn commit, close, open" dance.
> 
> tree[3] = 6
> get_transaction().commit(1)
> cn.close()
> 
> cn = db.open()
> rt = cn.root()
> 
> tree = rt['tree']
> 
> # Now `tree` has 3 elements in cache, but only 2 elements in
> # the database.
> 
> keys = list(tree.keys())
> print "current keys", keys  # prints [1, 2, 3]
> 
> # Oops.  Simulate the cache getting cleaned wrt `tree`.
> tree._p_deactivate()
> 
> # This one is entirely different:  the BTree gets restored from
> # the database, which only had two keys.
> new_keys = list(tree.keys())
> 
> for k in keys:
>     try:
>         del tree[k]
>     except KeyError:
>         # This triggers!  Key 3 vanished.
>         print "oops!  key", k, "is missing now"
>         print "the keys actually were", new_keys
> 
> If you run that, it prints:
> 
>     current keys [1, 2, 3]
>     oops!  key 3 is missing now
>     the keys actually were [1, 2]

Whew.   That must have took some work to think up. ;-)

At very least, I now have a decoy explanation for now until we find the
next case.! ;-)

- C