sui icon indicating copy to clipboard operation
sui copied to clipboard

[TransactionManager] lock transaction execution via `TransactionManager`

Open mwtian opened this issue 2 years ago • 2 comments

Currently it is a bit hard to lock transaction execution for all code paths we are interested in, because the scoped TxGuard object needs to be passed around. Since all transactions will go through TransactionManager, it seems a natural place to guarantee only one transaction is in any of pending / executing / executed state.

Also, the pending_certificates table used by TransactionManager for recover seems to duplicate the purpose of wal table. We should keep only one of them.

In this change,

  • TransactionManager::enqueue_to_execute() takes on the responsibility of locking the execution of a transaction, writes the certificate to pending_certificates table, and taking shared object locks if needed.
  • TransactionManager::commit() is added, which unlock the execution of a transaction, clears its entry in pending_certificates and shared object locks.

These two functions form a lock() / unlock() pair for certificate execution. After this change

  • for consensus output, TransactionManager locking is active and encloses CertTxGuard acquire and release.
  • for certificates from RPC, and certificates with effects, TransactionManager locking is not used.

There are additional refactoring possible:

  • remove CertTxGuard API for locking certificate execution.
  • remove wal table and the associated recovery logic.

But these require integrating TransactionManager with handle_certificate_with_effects() first, so all code paths are covered by TransactionManager locking. This will be a later change.

mwtian avatar Nov 10 '22 19:11 mwtian

Since all transactions will go through TransactionManager

I don't know about this - I think we still want to be able to process transactions upon request (see handle_certificate in authority_server.rs).

Potentially, we could require transactions from authority_server to go through TransactionManager, but I'm not sure if that's an improvement over the current situation.

mystenmark avatar Nov 12 '22 03:11 mystenmark

I don't know about this - I think we still want to be able to process transactions upon request (see handle_certificate in authority_server.rs).

Yes, the plan is for certificates from handle_certificate() rpc handler to go through TransactionManager as well. The advantages are:

  • Make the handler cleaner and more consistent between owned object and shared object transactions. All certificates will wait on effects, instead of executing certificates directly, and hoping for shared object transaction the execution can be deduplicated.
  • Client no longer has to be exposed with a variety of errors from execution, and forced to retry them.

I guess an overview about the proposed changes would be helpful. I will try to produce that.

mwtian avatar Nov 12 '22 05:11 mwtian