Re: [tarantool-patches] Re: [PATCH 4/4] txn: undo commit/rollback triggers when reverting to savepoint

[Vladimir Davydov <vdavydov.dev@gmail.com>]

Please take a look at an alternative solution then:

  https://www.freelists.org/post/tarantool-patches/PATCH-v2-03-Support-savepoints-in-DDL-transactions

Rather than trying to maintain global commit/rollback trigger list and
removing triggers from them, the second version of this patch simply
attaches triggers to txn statements. I hope you'll find it easier for
understanding.

On Mon, Jul 29, 2019 at 04:11:50PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/19 21:09]:
> 
> I spent some time thinking about this patch.
> 
> I agree we need to make commit and rollback
> triggers, as well as some SQL related members, part of the
> savepoint state.
> 
> DDL now can be part of a transaction, and since we may roll back
> to a savepoint, we need to make sure that all parts of the
> transaction, including a commit/rollback trigger, are part of
> the savepoint.
> 
> However the patch itself suffers from quite a bit of
> rot.
> 
> Initially, the idea of struct txn was very simple: it is a linked
> list of txn_stmt objects, and each txn_stmt object represents a
> change in transaction state. The history is easy to navigate over,
> and a savepoint is just a link inside the history chain.
> 
> commit/rollback triggers were not part of this story because they
> were only used for DDL which was non-transactional.
> 
> This code was allowed to rot and when some SQL related members
> were added directly to struct txn without considering the issue of
> sub-statements and savepoints.
> 
> This patch, in this vein, instead of designing the transaction
> system to make sure that every part of transaction state is part
> of a change history, adds a few extra savepoint members and
> makes sure that transaction state is carefully copied to the
> savepoint when it is created. This is contrary to how savepoints
> used to work, when a savepoint was just a link in the history of
> changes of the transaction state.
> 
> As an ugly artefact, we store such members as has_triggers,
> on_commit, on_rollback in struct txn twice: once in struct txn
> itself, and another time in txn->in_sub_stmt[0], which is
> initialized along with the txn.
> 
> So I think that some work needs to be done to move 
> transaction state which can participate in a savepoint to
> either an existing object representing a piece of the transaction
> history (struct txn_stmt) or a new object (txn_stmt_chain,
> txn_savepoint_state or something similar), the transaction is
> represented as a chain of such objects, the first
> object in the chain is a member of struct txn and is created when
> the transaction starts, and the savepoint can continue to be 
> just a link in that chain.
> 
> Let's discuss this when you have a chance.