2nd level caching for zcatalog by means of plone.memoize

[andbag]

@hannosch I have now implemented a prototype for the caching of the catalog resultset based on plone.memoize. The cache key generation is based on the combination of the per-index counters. In addition, I have upgraded all PluginIndexes to support the getCounter logic (new interface IIndexCounter added). It's currently only a proof of concept and therefore, the caching tests are missing. Can you please do the code review?

[andbag]

@hannosch I have rebased the PR to master branch and added some tests for caching support. I believe that the caching mechanism now works reliably.

[icemac]

What is the status of this PR? Is it ready to merge?

[andbag]

From my point of view, the pull request is ready to be merged.

[eprigorodov]

The worst problem we have in our environment is that ZODB cannot properly maintain its cache size (set via cache_size_bytes) which leads to memory overflows. Setting small cache size creates a bottleneck in ZEO. plone.memoize seems to have no controls on the memory size it consumes, so this improvement can actually be a trade-off.

[andbag]

BTW: A separate mount point for the ZCatalog (e.g. portal_catalog in Plone) with individual cache sizes can also improve the cache management and performance of ZODB. This eliminates competing database accesses with different purposes.

[eprigorodov]

Thanks for the advice, that can be useful for large ZCatalogs.

I was wrong about memory usage in plone.memoize. The code uses plone.memoize.ram, which is based on zope.ramcache, which has a default size limit (1000 objects) and maintains the cache size.

[eprigorodov]

Another concern was transaction isolation. zope.ramcache tells explicitly that it shares cached data between all threads.

The code prevents possible race scenarios by mixing index counters into the cache key.

[andbag]

You are not bound to zope.ramcache's default settings. plone.memoize.ram uses a utility for cache choosing. Default settings can easily be changed or extended by providing an own utility. In addition, you can use other caching storages (e.g. memcached, https://docs.plone.org/external/plone.app.caching/docs/ram-cache.html).

I can use additionally the _p_serial (transaction id) of the index counter objects for the cache key generation. Ambiguities of result sets should then be excluded.
@eprigorodov: Do you know code examples that emulate threads that access the ZODB simultaneously?

[andbag]

ZCatalog's cache key consider now the transaction ids of the index counter. This makes the cache key unique and thread aware. Ambiguities of result sets should be solved by the conflict error handling.

[eprigorodov]

That might work not that well: the _p_serial attribute does not identify current transaction, but rather the past transaction in which the object was modified. The following example illustrates the _p_serial behavior (it does not make a real race condition):

from persistent import Persistent
import transaction
from ZODB import DB
from ZODB.DemoStorage import DemoStorage

class MockCounter(Persistent):
    pass

storage = DemoStorage()
db = DB(storage)
conn = db.open()

root = conn.root()

transaction.begin()
root.counter0 = MockCounter()
root.counter1 = MockCounter()
root.counter0.value = 1
root.counter1.value = 2
transaction.commit()

transaction.begin()
assert root.counter1._p_serial == root.counter0._p_serial
root.counter1.value = 3
transaction.commit()

transaction.begin()
# Here, if _p_serial meant a current transaction ID,
# then it should have been the same for both counters,
# but it is not:
assert root.counter1._p_serial != root.counter0._p_serial
root.counter0.value = 4
root.counter1.value = 5
transaction.commit()

transaction.begin()
# And here it is the same for both objects again
# because both have been modified in some past transaction.
assert root.counter1._p_serial == root.counter0._p_serial
transaction.abort()

conn.close
storage.close()
db.close()

So it is quite possible that two concurrent transaction that started to work with the same index object would see the same _p_serial value and thus can generate the same cache key and overwrite cache entry. I am trying now to formulate a working race scenario and write the threaded code for it.

[andbag]

Thank you for the info. Currently I am experimenting with a transactional aware CacheAdapter that implements the IDataManager interface. It would be great if there was an adequate test scenario.

[eprigorodov]

Here is a gist that works in the stock ZCatalog and fails under the cached implementation due to inconsistent cache read (tested on commit 9d8aea6).

The race scenario is:

• Thread-1 and Thread-2 execute the same method which indexes a new object and then makes a query that should include it
• both threads start at the same time, so they read the same committed version of FieldIndex:
  • _counter in both threads equals to N
  • _p_serial in both threads equals to 'M'
• Thread-1 indexes its own object1
  • FieldIndex increases its _counter to N+1
• Thread-2 indexes its own object2
  • FieldIndex increases its _counter to N+1
• Thread-2 makes a query that should return a result set, containing a newly added object2
  • caching descriptor builds a cache key, based on a query, _counter == N+1 and _p_serial == 'M'
  • ZCatalog makes a new result set
  • the query result is cached and becomes visible to Thread-1
• Thread-2 verifies query result and commits its changes
• Thread-1 does not see any committed changes in its transaction, because ZODB isolates transactions on snapshot level
• Thread-1 makes the same query that should returns a result set, containing a newly added object1
  • caching descriptor builds a cache key, based on a query, _counter == N+1 and _p_serial == 'M'
  • that cache key is already present in the cache
  • cache returns result set from query made by Thread-2
• Thread-1 iterates result set and reaches an entry corresponding to object2 document
  • object2 document is not visible in Thread-1 transaction yet
  • ZCatalog tries to instantiate a result entry and fails with the KeyError

The gist contains a conflict retry mechanism similar to ZPublisher. That was mostly done to handle ConflictError exceptions that are inevitable in this scenario.

[hannosch]

One annoying comment on the PR: Unfortunately plone.memoize is licensed under the GPL only. So it can't be used here, as the code is under the ZPL.

We can use zope.ramcache directly or store things again on the request object if so desired.

[andbag]

OK, I chose plone.memoize because it is easy to use RAMCache by default or e.g. memcache for large sites with many Zope workers. For better transaction management, I am currently working on a solution that implements IDataManager. Perhaps this is the best time to program an own cache utility for the ZCatalog.
@eprigorodov: Thank you for the test script. I have programmed a test based on your concept, s.

Products.ZCatalog/src/Products/ZCatalog/tests/test_cache.py

Line 193 in 88d0d90

def test_cache_mvcc_aware(self):

[eprigorodov]

@andbag, you are welcome, but that was just a single race scenario out of many others possible.

Using transaction-aware cache is also not a guarantee per se. For example, IDataManager enabled cache could prevent dirty reads, but implement "read committed" isolation. Then committed entries might leak into concurrent transactions in the same way.

One possible approach could be storing cache in the _v_-attribute of the Catalog object itself, that will prevent its sharing at all. The Catalog is persistent, so each thread/connection keeps its own object instance and ZODB ensures correct isolation. _v_-attributes can also survive between requests / transactions if object was not invalidated due to concurrent write. Have a look at https://pypi.org/project/zope.cachedescriptors/

[andbag]

Okay, I understand. While I focused on data consistency, you want to guarantee data isolation during a transaction. Of course I know the'v' attribute, but it should not be the only cache solution. Our university runs a very large Plone site with many workers who could benefit from a shared cache.

[andbag]

@icemac, yes, you're right. In addition to the licensing issue, we need to clarify whether the current cache implementation preserves data consistency and isolation during a transaction.

[andbag]

@hannosch, I'm a little frustrated that the wheel has to be invented a second time for caching. You are also a maintainer of plone.memoize. Isn't it possible to publish plone.memoize under dual licence model?

[icemac]

@andbag I think relicensing of a Plone package has to be decided by one of the teams of the Plone foundation (but I am not sure which team). There were some packages in the past which got a new license on demand for something which is not GPL. (but I do not remember which packages where the ones.) I'd suggest to ask on https://community.plone.org.

[andbag]

The licensing question is waiting for an answer from the Plone Community (s. https://community.plone.org/t/re-licensing-plone-memoize-to-support-products-zcatalog/7034)

[polyester]

Writing here, as not sure everybody follows community.plone.org. Normally, a license change to a Plone package would be requested by the Framework team, and then the Board of the Plone Foundation decides. So that's the easiest way.

It would definitely also help to hear @hannosch on it, as he's a/the principal author.

And to know what's the plan. From what I gather, it is not to literally take code from plone.memoize into the zopefoundation org repo, but just to depend on it. That could be accomplished by making plone.memoize available as LGPL, which is a much smaller ask, and you can probably just write directly to the Board ([email protected])

but I'm not 100% sure on that, as the technical details of this PR are slightly above my head.

Re-licensing as ZPL would be a bigger ask, we then would prefer to relicense as MIT to keep our licensing zoo down. (Since it's the preferred license of the JavaScript world there's already quite some MIT code in the Plonefoundation repos).

It's possible to do, if you want to copy actual code into the Zopefoundation repo, but at least it would need consent of @hannosch , and some indication by the Plone Framework team that this couldn't inadvertently harm Plone.

(oh, and I'm on the Plone Board as president, which is why I'm commenting...)

[smcmahon]

Here's the licensing policy:

https://plone.org/foundation/materials/foundation-resolutions/plone-framework-components-relicensing-policy

Many plone.* components have been relicensed. The modified BSD is entirely compatible with the ZPL.

[andbag]

Interesting! While the documentation at plone.org says that plone.memoize was supposedly re-licensed under BSD in Sep 2010, the current code in the repo of plone.memoize is licensed under the GPL.
@hannosch Is this misinformation or is there a reason for this?

Currently the PR depends only on plone.memoize (re-licensing to LGPL or BSD (?) seems to be sufficient). However, it is a design question whether the cache is shared with other plone/zope modules or whether the ZCatalog has its own cache logic. At the moment I see no reason why one should prefer to separate the cache logic. What is your (especially @hannosch) opinion?

[polyester]

The plot thickens ;-)
Yes, it appears it was already formally approved after some initial discussion

But probably never actually implemented as a pull request on plone.memoize.