ConflictResolvingStorage should implement new_instance? about zodb

[jimfulton]

I don't think this hurts non-RelStorage storages, because this happens at a level below the adapter.

By RelStorage+zlibstorage, do you mean zlibstorage wrapping RelStoage? Because it copies interfaces, it's claiming to implement IMVCCStorage, but isn't. Perhaps it should implement new_instance.

I don't see off hand how adding new_instance to CRS would help.

FTR, I'd really like to conflict resolution out of the storage layer. If it was done by ZODB itself, we could lose this silly transformation dance and simplify storage implementations. Potentially, we could improve conflict resolution quite a bit as well. FWIW: https://github.com/zopefoundation/ZODB/wiki/OO-conflict-resolution

[jamadden]

By RelStorage+zlibstorage, do you mean zlibstorage wrapping RelStoage?

Yes.

Because it copies interfaces, it's claiming to implement IMVCCStorage, but isn't. Perhaps it should implement new_instance.

I have a bug open for that, although that's not quite enough.

I don't see off hand how adding new_instance to CRS would help.

CRS sets some attributes in registerDB. When zc.zlibstorage wraps a CRS, those attributes are necessary for conflict resolution to work. registerDB is only called once when a DB is constructed, on the root storage. Each time new_instance is called, a brand new object is created and returned. And because CRS doesn't implement new_instance, those attributes are not set on the new instance. So the new instance is effectively broken; instead of using the transformation attributes set by registerDB, it uses the default class-level transformation attributes.

The proposal here is that CRS.new_instance() should set on the new instance the same attributes that were set in registerDB.

[jimfulton]

What would call CRS.new_instance? CRS is a mix-in class.

Through the magic of the getattr hack, when new_instance is called on a zlibstorage, RelStorage's new_instance would be called. Would it then call CRS' new_instance? new_instance doesn't have the right semantics here, as it, you know, creates new instances. If RelStorage's new_instance called CRS's new_instance, it would get a new instance from CRS, when all it wants is to copy some attributes. This feels like the wrong model here.

If you really want to fix this, a better way IMO would be for zlibstorage to implement a new_instance that delegates to the base and then calls registerDB on the result.

(Of course, new_instance, implying cloning is a questionable model to begin with.)

Finally, this is a terrible place to discuss things. I'd strongly prefer IRC or some other real-time chat for this.

[jamadden]

I would probably implement it like this in CRS:

def _new_instance(self):
   # subclass-specific work done here
   raise AttributeError("new-instance")

def new_instance(self):
   instance = self._new_instance()
   instance._crs_untransform_record_data =  self._crs_untransform_record_data
   instance._crs_transform_record_data =  self._crs_transform_record_data
   return instance

Then in RelStorage I would always define _new_instance and only conditionally define new_instance (to look exactly like what's above) depending on whether CRS already had a definition or not.

[jimfulton]

This is too convoluted. Inheritance is a terrible way to provide an API. This is a RelStorage-specific problem that's easy to solve in RelStorage. Just copy those two attributes in RelStorage's new_instance and be done with it.