[tarantool-patches] Re: [PATCH 4/4] txn: undo commit/rollback triggers when reverting to savepoint
Konstantin Osipov
kostja at tarantool.org
Mon Jul 29 16:11:50 MSK 2019
* Vladimir Davydov <vdavydov.dev at 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
More information about the Tarantool-patches
mailing list