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

[Konstantin Osipov <kostja@tarantool.org>]

* 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.


> When reverting to a savepoint inside a DDL transaction, apart from
> undoing changes done by the DDL statements to the system spaces, we also
> have to
> 
>  - Run rollback triggers installed after the savepoint was set, because
>    otherwise changes done to the schema by DDL won't be undone.
>  - Remove commit triggers installed after the savepoint, because they
>    are not relevant anymore, apparently.
> 
> To achieve that, let's remember not only the last transaction statement
> at the time when a savepoint is created, but also the state of commit
> and rollback triggers. Since in contrast to txn statements, commit and
> rollback triggers can actually be deleted from the list, we create dummy
> triggers per each savepoint and use them as markers in the trigger
> lists. We don't however create dummy triggers if no commit/rollback
> triggers is installed, because in that case we would simply need to
> clear the trigger lists.
> 
> While we are at it, let's also make sure that a rollback trigger doesn't
> an already freed tuple. To do that, we release statement tuples after
> running rollback triggers, not before as we used to.
> 
> Closes #4364
> Closes #4365
> ---
>  src/box/txn.c                 | 74 ++++++++++++++++++++++++++++++++++++++-----
>  src/box/txn.h                 | 11 +++++++
>  test/box/transaction.result   | 39 +++++++++++++++++++++++
>  test/box/transaction.test.lua | 23 ++++++++++++++
>  4 files changed, 139 insertions(+), 8 deletions(-)
> 
> diff --git a/src/box/txn.c b/src/box/txn.c
> index 3f0017f8..47afcaa0 100644
> --- a/src/box/txn.c
> +++ b/src/box/txn.c
> @@ -46,20 +46,33 @@ txn_on_stop(struct trigger *trigger, void *event);
>  static void
>  txn_on_yield(struct trigger *trigger, void *event);
>  
> +static void
> +txn_run_triggers(struct txn *txn, struct rlist *trigger);
> +
>  static inline void
>  fiber_set_txn(struct fiber *fiber, struct txn *txn)
>  {
>  	fiber->storage.txn = txn;
>  }
>  
> +static void
> +dummy_trigger_cb(struct trigger *trigger, void *event)
> +{
> +	(void)trigger;
> +	(void)event;
> +}
> +
>  /**
>   * A savepoint that points to the beginning of a transaction.
>   * Use it to rollback all statements of any transaction.
>   */
>  static struct txn_savepoint zero_svp = {
>  	/* .in_sub_stmt = */ 0,
> +	/* .has_triggers = */ false,
>  	/* .stmt = */ NULL,
>  	/* .fk_deferred_count = */ 0,
> +	/* .on_commit = */ {RLIST_LINK_INITIALIZER, 0, NULL, NULL},
> +	/* .on_rollback = */ {RLIST_LINK_INITIALIZER, 0, NULL, NULL},
>  };
>  
>  /**
> @@ -71,10 +84,31 @@ txn_create_svp(struct txn *txn, struct txn_savepoint *svp)
>  {
>  	svp->in_sub_stmt = txn->in_sub_stmt;
>  	svp->stmt = stailq_last(&txn->stmts);
> +	svp->has_triggers = txn->has_triggers;
> +	if (svp->has_triggers) {
> +		trigger_create(&svp->on_commit, dummy_trigger_cb, NULL, NULL);
> +		trigger_add(&txn->on_commit, &svp->on_commit);
> +		trigger_create(&svp->on_rollback, dummy_trigger_cb, NULL, NULL);
> +		trigger_add(&txn->on_rollback, &svp->on_rollback);
> +	}
>  	if (txn->psql_txn != NULL)
>  		svp->fk_deferred_count = txn->psql_txn->fk_deferred_count;
>  }
>  
> +/**
> + * Destroy a savepoint that isn't going to be used anymore.
> + * This function clears dummy commit and rollback triggers
> + * installed by the savepoint.
> + */
> +static void
> +txn_destroy_svp(struct txn_savepoint *svp)
> +{
> +	if (svp->has_triggers) {
> +		trigger_clear(&svp->on_commit);
> +		trigger_clear(&svp->on_rollback);
> +	}
> +}
> +
>  static int
>  txn_add_redo(struct txn *txn, struct txn_stmt *stmt, struct request *request)
>  {
> @@ -144,6 +178,11 @@ txn_stmt_unref_tuples(struct txn_stmt *stmt)
>  static void
>  txn_rollback_to_svp(struct txn *txn, struct txn_savepoint *svp)
>  {
> +	/*
> +	 * Undo changes done to the database by the rolled back
> +	 * statements. Don't release the tuples as rollback triggers
> +	 * might still need them.
> +	 */
>  	struct txn_stmt *stmt;
>  	struct stailq rollback;
>  	stailq_cut_tail(&txn->stmts, svp->stmt, &rollback);
> @@ -161,10 +200,31 @@ txn_rollback_to_svp(struct txn *txn, struct txn_savepoint *svp)
>  			assert(txn->n_applier_rows > 0);
>  			txn->n_applier_rows--;
>  		}
> -		txn_stmt_unref_tuples(stmt);
>  		stmt->space = NULL;
>  		stmt->row = NULL;
>  	}
> +	/*
> +	 * Remove commit and rollback triggers installed after
> +	 * the savepoint was set and run the rollback triggers.
> +	 */
> +	RLIST_HEAD(on_commit);
> +	RLIST_HEAD(on_rollback);
> +	if (svp->has_triggers) {
> +		rlist_cut_before(&on_commit, &txn->on_commit,
> +				 &svp->on_commit.link);
> +		rlist_cut_before(&on_rollback, &txn->on_rollback,
> +				 &svp->on_rollback.link);
> +	} else if (txn->has_triggers) {
> +		rlist_splice(&on_commit, &txn->on_commit);
> +		rlist_splice(&on_rollback, &txn->on_rollback);
> +		txn->has_triggers = false;
> +	}
> +	txn_run_triggers(txn, &on_rollback);
> +
> +	/* Release the tuples. */
> +	stailq_foreach_entry(stmt, &rollback, next)
> +		txn_stmt_unref_tuples(stmt);
> +
>  	if (txn->psql_txn != NULL)
>  		txn->psql_txn->fk_deferred_count = svp->fk_deferred_count;
>  }
> @@ -362,6 +422,7 @@ txn_commit_stmt(struct txn *txn, struct request *request)
>  			goto fail;
>  	}
>  	--txn->in_sub_stmt;
> +	txn_destroy_svp(&txn->sub_stmt_begin[txn->in_sub_stmt]);
>  	return 0;
>  fail:
>  	txn_rollback_stmt(txn);
> @@ -371,7 +432,7 @@ fail:
>  /*
>   * A helper function to process on_commit/on_rollback triggers.
>   */
> -static inline void
> +static void
>  txn_run_triggers(struct txn *txn, struct rlist *trigger)
>  {
>  	/* Rollback triggers must not throw. */
> @@ -589,8 +650,9 @@ txn_rollback_stmt(struct txn *txn)
>  {
>  	if (txn == NULL || txn->in_sub_stmt == 0)
>  		return;
> -	txn->in_sub_stmt--;
> -	txn_rollback_to_svp(txn, &txn->sub_stmt_begin[txn->in_sub_stmt]);
> +	struct txn_savepoint *svp = &txn->sub_stmt_begin[--txn->in_sub_stmt];
> +	txn_rollback_to_svp(txn, svp);
> +	txn_destroy_svp(svp);
>  }
>  
>  void
> @@ -790,9 +852,5 @@ txn_on_yield(struct trigger *trigger, void *event)
>  	struct txn *txn = in_txn();
>  	assert(txn != NULL && !txn->can_yield);
>  	txn_rollback_to_svp(txn, &zero_svp);
> -	if (txn->has_triggers) {
> -		txn_run_triggers(txn, &txn->on_rollback);
> -		txn->has_triggers = false;
> -	}
>  	txn->is_aborted_by_yield = true;
>  }
> diff --git a/src/box/txn.h b/src/box/txn.h
> index a1685b5f..80463ff0 100644
> --- a/src/box/txn.h
> +++ b/src/box/txn.h
> @@ -89,6 +89,8 @@ struct txn_savepoint {
>  	 * creation.
>  	 */
>  	int in_sub_stmt;
> +	/** True if commit/rollback triggers are set. */
> +	bool has_triggers;
>  	/**
>  	 * Statement, on which a savepoint is created. On rollback
>  	 * to this savepoint all newer statements are rolled back.
> @@ -106,6 +108,15 @@ struct txn_savepoint {
>  	 * state violating any number of deferred FK constraints.
>  	 */
>  	uint32_t fk_deferred_count;
> +	/**
> +	 * Dummy commit and rollback triggers installed when this
> +	 * savepoint was created. They are used on rollback to this
> +	 * savepoint in order to remove triggers set after this
> +	 * savepoint was created. We don't set the fake triggers
> +	 * if the transaction doesn't have any triggers, because
> +	 * in that case we have to remove all triggers on rollback.
> +	 */
> +	struct trigger on_commit, on_rollback;
>  };
>  
>  extern double too_long_threshold;
> diff --git a/test/box/transaction.result b/test/box/transaction.result
> index 9a48f2f7..1ed649d2 100644
> --- a/test/box/transaction.result
> +++ b/test/box/transaction.result
> @@ -778,3 +778,42 @@ box.begin() drop() box.rollback()
>  box.begin() drop() box.commit()
>  ---
>  ...
> +--
> +-- gh-4364: DDL doesn't care about savepoints.
> +--
> +test_run:cmd("setopt delimiter ';'")
> +---
> +- true
> +...
> +box.begin()
> +s1 = box.schema.create_space('test1')
> +save = box.savepoint()
> +s2 = box.schema.create_space('test2')
> +check1 = box.space.test1 ~= nil
> +check2 = box.space.test2 ~= nil
> +box.rollback_to_savepoint(save)
> +check3 = box.space.test1 ~= nil
> +check4 = box.space.test2 ~= nil
> +box.commit();
> +---
> +...
> +test_run:cmd("setopt delimiter ''");
> +---
> +- true
> +...
> +check1, check2, check3, check4
> +---
> +- true
> +- true
> +- true
> +- false
> +...
> +--
> +-- gh-4365: DDL reverted by yield triggers crash.
> +--
> +box.begin() box.execute([[CREATE TABLE test(id INTEGER PRIMARY KEY AUTOINCREMENT)]]) fiber.yield()
> +---
> +...
> +box.rollback()
> +---
> +...
> diff --git a/test/box/transaction.test.lua b/test/box/transaction.test.lua
> index 0792cc3c..a0014c9f 100644
> --- a/test/box/transaction.test.lua
> +++ b/test/box/transaction.test.lua
> @@ -415,3 +415,26 @@ box.begin() create() box.rollback()
>  box.begin() create() box.commit()
>  box.begin() drop() box.rollback()
>  box.begin() drop() box.commit()
> +
> +--
> +-- gh-4364: DDL doesn't care about savepoints.
> +--
> +test_run:cmd("setopt delimiter ';'")
> +box.begin()
> +s1 = box.schema.create_space('test1')
> +save = box.savepoint()
> +s2 = box.schema.create_space('test2')
> +check1 = box.space.test1 ~= nil
> +check2 = box.space.test2 ~= nil
> +box.rollback_to_savepoint(save)
> +check3 = box.space.test1 ~= nil
> +check4 = box.space.test2 ~= nil
> +box.commit();
> +test_run:cmd("setopt delimiter ''");
> +check1, check2, check3, check4
> +
> +--
> +-- gh-4365: DDL reverted by yield triggers crash.
> +--
> +box.begin() box.execute([[CREATE TABLE test(id INTEGER PRIMARY KEY AUTOINCREMENT)]]) fiber.yield()
> +box.rollback()
> -- 
> 2.11.0
> 

-- 
Konstantin Osipov, Moscow, Russia