Plone is loosing two phase commit functionality

do3cc · March 9, 2017, 1:17pm

Hi,

today my sentry gave me an error: "A storage error occurred during the second phase of the two-phase commit. Resources may be in an inconsistent state."

I vaguely remembered that I looked into collective.solr and decided that it does not handle two-phase-commits properly.
(https://en.wikipedia.org/wiki/Two-phase_commit_protocol)

The error made me have a second look into this and I stumbled upon this code snippet:

github.com

plone/collective.indexing/blob/356df89dd18ba039207dc072b55a717afda826dd/src/collective/indexing/transactions.py#L45-L65


def tpc_begin(self, transaction):
    pass


def commit(self, transaction):
    pass


def before_commit(self):
    self.queue.process()
    self.queue.clear()


def tpc_vote(self, transaction):
    pass


def tpc_finish(self, transaction):
    self.queue.commit()
    self.registered = False


def tpc_abort(self, transaction):
    self.queue.abort()
    self.queue.clear()
    self.registered = False

This is the management summary:
collective.indexing is wrapping TPC functionality ignoringTPC requirements so that it can provide a simpler commit interface. So collective.solr using collective.indexing has no way to implement 2PC properly.

Longer explanation:
In the first TPC phase, every storage is getting asked if they can "guarantee" that their transaction can be committed.
After everybody confirmed this, in the second TPC phase everbody is told to actually commit. The implementation above does never ask its storages if a commit is possible and first tries to commit in the second phase where a sucessful commit should be "guaranteed".
Example: Add an item. During second phase of TPC, I first I ask solr to commit, and solr finishes, then I ask ZODB to commit and get a WriteConflictError. ZODB does not store the document but solr has the information.
On the next search I get a result for a document that does not exist and trigger a site error when trying to open the page.

This is a problem considering that we are planning/doing/did add collective.indexing to core.

The solr example might look not soo bad, considering that everything can be repaired with a full reindex. But if you rely on TPC for another database that is not just an index, there is no working TPC implementation when using collective.indexing.

I am mentioning this here instead of just a ticket because I assume that there is a general interest into how to handle this. After all, a full fix would mean changing interfaces, and the alternative, giving up on TPC, might have big consequences for users who need this.

A bit of history: I was told that Nexedi originally provided the TPC code to ZODB because they needed this for accessing multiple database in a transactional save way for ERP5. ERP5 does not use Plone, just Zope, so it does not matter to them what we do with collective.indexing, but there might be Plone users who rely on this.

datakurre · March 9, 2017, 5:02pm

@do3cc I did not fully understand. Does ZODB really raise WriteConflictError only in tpc_finish, not already in tpc_vote? Shouldn't it be other way around: ZODB is quaranteed to commit in tpc_finish, but SOLR may fail and then it's missing index for the new content?

jaroel · March 9, 2017, 5:18pm

This can only happen when there's an exception in tpc_finish or tpc_abort.

My guess there went something wrong in https://github.com/collective/collective.solr/blob/master/src/collective/solr/indexer.py#L290, maybe in self.manager.closeConnection() ?

Maybe collective.indexing is in the wrong here, as it calls IIndexQueue.commit in ISavepointDataManager.tcp_finish implemented by collective.indexing.transactions. QueueTM, which calls IIndexQueueProcessor.commit in it's default implementation.

do3cc · March 9, 2017, 10:19pm

Please ignore my example, it is misleading and I misunderstood some part of the code.
The queue implementation from collective indexing, that handles the optimized indexing in the portal catalog has no TPC problem.

With a slightly deeper understanding I must modify my original statements a bit:

collective.solr provides a mechanism for 3rd parties to index data on the end of a transaction. It does NOT provide TPC semantics here. This is fine for indexing mechanisms that store the data into the zodb and just not implement anything in the commit handlers, as zodb does the commit on its own.
If a 3rd party uses the mechanism to hook into the end of a transaction (IIndexQueue) it CAN mess up TPC if an exception gets thrown in the commit handler.

The commit handler gets executed during tpc_finish, where everybody (only the zodb and collective indexing) promised that a commit is fine with them.
I think (1 2 3 4 ) that the collective.indexing resource provider always get executed first in tpc_finish. I did stop diving into code and did not check what happens if zodbs tpc_finish won't get executed. I guess the transaction won't commit.

This is probably less bad than I thought initially. Still, if this would be a project that is in heavy use, and not short before getting decommissioned, I'd have a deeper look into the failure modes here to see if failure handling can be improved.

do3cc · March 9, 2017, 10:36pm

Yes, I am suspecting an unhandled exception there. Upon second inspection of the code I noticed that there must have been a second error log in sentry with a traceback. I can't access it right now, but tomorrow I will have a look for it again.

datakurre · March 10, 2017, 6:06am

Good point. I did not realize that commit does indeed stop when the first tpc_finish raises an exception.

Transaction data managers are sorted by their sortKey method. From a quick look it seems that ZODB storages use storage name based sort keys. Because c.indexing has integer as a sort key, it will always be sorted before ZODB storages and exception in c.indexing will prevent ZODB commit.

do3cc · March 10, 2017, 9:08am

No traceback. I'll just pray it won't happen again.