[tarantool-patches] Re: [PATCH 2/4] txn: reverse commit trigger list only before running commit triggers
Vladimir Davydov
vdavydov.dev at gmail.com
Thu Jul 25 17:56:42 MSK 2019
On Thu, Jul 25, 2019 at 12:35:28PM +0300, Vladimir Davydov wrote:
> On Thu, Jul 25, 2019 at 12:29:48PM +0300, Konstantin Osipov wrote:
> > * Vladimir Davydov <vdavydov.dev at gmail.com> [19/07/25 12:28]:
> > > On Thu, Jul 25, 2019 at 01:48:11AM +0300, Konstantin Osipov wrote:
> > > > * Vladimir Davydov <vdavydov.dev at gmail.com> [19/07/19 21:09]:
> > > > > Commit triggers must be run in the same order they are added, see commit
> > > > > 013432641283 ("txn: fix execution order of commit triggers"). To achieve
> > > > > that we added a new trigger method, trigger_add_tail(), which adds new
> > > > > triggers to the trigger list tail rather than to the head, and now we
> > > > > use this new method for adding commit triggers.
> > > >
> > > > Commit triggers are a hot path.
> > >
> > > I wouldn't say they are - they are only used by DDL.
> >
> > commit path is always a hot path, and if the server itself invokes
> > these triggers on DDL only, it doesn't mean users never invoke
> > them on a hot path.
> >
> > But the server itself will use a lot of commit triggers when
> > materialized views are implemented.
>
> Reversing a list of a few triggers won't make much difference from
> perfromance pov IMO as we iterate over them anyway. Iterating over
> the list in the reverse order would require a bit of refactoring and
> adding rlist_foreach_entry_reverse_safe method. I don't really care,
> to tell the truth, and will rework as you wish, no problem.
Reworked the patch to use a reverse iterator over the list of commit
triggers rather than reversing the list explicitly. Please take a look.
>From 7bf0567dae27304fda315363b0930de452db96d6 Mon Sep 17 00:00:00 2001
From: Vladimir Davydov <vdavydov.dev at gmail.com>
Date: Fri, 19 Jul 2019 17:09:45 +0300
Subject: [PATCH] txn: reverse commit trigger list only before running commit
triggers
Commit triggers must be run in the same order they are added, see commit
013432641283 ("txn: fix execution order of commit triggers"). To achieve
that we added a new trigger method, trigger_add_tail(), which adds new
triggers to the trigger list tail rather than to the head, and now we
use this new method for adding commit triggers.
Come to think of it now, that solution was rather confusing. First,
commit triggers are still added to the head of the list from Lua.
Second, to revert triggers on rollback-to-savepoint it would be really
more convenient to have both commit and rollback trigger lists have the
same order.
So this patch reverts the above-mentioned commit and instead simply
uses a reverse iterator to run commit triggers.
diff --git a/src/box/txn.c b/src/box/txn.c
index 73eaee73..cb5a06cc 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -344,20 +344,41 @@ fail:
}
/*
- * A helper function to process on_commit/on_rollback triggers.
+ * A helper function to process on_commit triggers.
*/
static inline void
-txn_run_triggers(struct txn *txn, struct rlist *trigger)
+txn_run_commit_triggers(struct txn *txn)
{
- /* Rollback triggers must not throw. */
- if (trigger_run(trigger, txn) != 0) {
+ /*
+ * Commit triggers must be run in the same order they
+ * were added so that a trigger sees the changes done
+ * by previous triggers (this is vital for DDL).
+ */
+ if (trigger_run_reverse(&txn->on_commit, txn) != 0) {
/*
* As transaction couldn't handle a trigger error so
* there is no option except panic.
*/
diag_log();
unreachable();
- panic("commit/rollback trigger failed");
+ panic("commit trigger failed");
+ }
+}
+
+/*
+ * A helper function to process on_rollback triggers.
+ */
+static inline void
+txn_run_rollback_triggers(struct txn *txn)
+{
+ if (trigger_run(&txn->on_rollback, txn) != 0) {
+ /*
+ * As transaction couldn't handle a trigger error so
+ * there is no option except panic.
+ */
+ diag_log();
+ unreachable();
+ panic("rollback trigger failed");
}
}
@@ -376,13 +397,13 @@ txn_complete(struct txn *txn)
if (txn->engine)
engine_rollback(txn->engine, txn);
if (txn->has_triggers)
- txn_run_triggers(txn, &txn->on_rollback);
+ txn_run_rollback_triggers(txn);
} else {
/* Commit the transaction. */
if (txn->engine != NULL)
engine_commit(txn->engine, txn);
if (txn->has_triggers)
- txn_run_triggers(txn, &txn->on_commit);
+ txn_run_commit_triggers(txn);
double stop_tm = ev_monotonic_now(loop());
if (stop_tm - txn->start_tm > too_long_threshold) {
@@ -763,7 +784,7 @@ txn_on_yield(struct trigger *trigger, void *event)
assert(txn != NULL && !txn->can_yield);
txn_rollback_to_svp(txn, NULL);
if (txn->has_triggers) {
- txn_run_triggers(txn, &txn->on_rollback);
+ txn_run_rollback_triggers(txn);
txn->has_triggers = false;
}
txn->is_aborted_by_yield = true;
diff --git a/src/box/txn.h b/src/box/txn.h
index 3996a01f..2d5dea21 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -201,16 +201,7 @@ struct txn {
* in case a fiber stops (all engines).
*/
struct trigger fiber_on_stop;
- /**
- * Commit and rollback triggers.
- *
- * Note, we commit triggers are added to the tail of
- * the list while rollback triggers are added to the
- * head, see txn_on_commit() and txn_on_rollback().
- * This ensures that the triggers are fired in the
- * same order as statements that added them, both on
- * commit and on rollback.
- */
+ /** Commit and rollback triggers. */
struct rlist on_commit, on_rollback;
struct sql_txn *psql_txn;
};
@@ -280,7 +271,7 @@ static inline void
txn_on_commit(struct txn *txn, struct trigger *trigger)
{
txn_init_triggers(txn);
- trigger_add_tail(&txn->on_commit, trigger);
+ trigger_add(&txn->on_commit, trigger);
}
static inline void
diff --git a/src/lib/core/trigger.cc b/src/lib/core/trigger.cc
index cd567ba1..4a43151e 100644
--- a/src/lib/core/trigger.cc
+++ b/src/lib/core/trigger.cc
@@ -45,3 +45,15 @@ trigger_run(struct rlist *list, void *event)
return 0;
}
+int
+trigger_run_reverse(struct rlist *list, void *event)
+{
+ try {
+ struct trigger *trigger, *tmp;
+ rlist_foreach_entry_safe_reverse(trigger, list, link, tmp)
+ trigger->run(trigger, event);
+ } catch (Exception *e) {
+ return -1;
+ }
+ return 0;
+}
diff --git a/src/lib/core/trigger.h b/src/lib/core/trigger.h
index 4bc4da1a..76fa6345 100644
--- a/src/lib/core/trigger.h
+++ b/src/lib/core/trigger.h
@@ -84,17 +84,6 @@ trigger_add(struct rlist *list, struct trigger *trigger)
rlist_add_entry(list, trigger, link);
}
-/**
- * Same as trigger_add(), but adds a trigger to the tail of the list
- * rather than to the head. Use it when you need to ensure a certain
- * execution order among triggers.
- */
-static inline void
-trigger_add_tail(struct rlist *list, struct trigger *trigger)
-{
- rlist_add_tail_entry(list, trigger, link);
-}
-
static inline void
trigger_add_unique(struct rlist *list, struct trigger *trigger)
{
@@ -124,9 +113,24 @@ trigger_destroy(struct rlist *list)
}
}
+/**
+ * Run registered triggers. Stop and return an error if
+ * a trigger fails.
+ *
+ * Note, since triggers are added to the list head, this
+ * function first runs triggers that were added last
+ */
int
trigger_run(struct rlist *list, void *event);
+/**
+ * Same as trigger_run(), but runs triggers in the reverse
+ * order, i.e. it runs triggers in the same order they were
+ * added.
+ */
+int
+trigger_run_reverse(struct rlist *list, void *event);
+
#if defined(__cplusplus)
} /* extern "C" */
diff --git a/test/engine/transaction.result b/test/engine/transaction.result
index b18b025c..910abb58 100644
--- a/test/engine/transaction.result
+++ b/test/engine/transaction.result
@@ -377,10 +377,10 @@ inspector:cmd("setopt delimiter ''");
-- Yes, the order is reversed. Like all other Lua triggers.
call_list
---
-- - on_commit_3
+- - on_commit_1
+ - on_commit_3
- on_commit_3
- on_commit_3
- - on_commit_1
...
funcs
---
More information about the Tarantool-patches
mailing list