* [PATCH 1/4] Update small library
2019-07-19 18:08 [PATCH 0/4] Support savepoints in DDL transactions Vladimir Davydov
@ 2019-07-19 18:08 ` Vladimir Davydov
2019-07-24 22:48 ` [tarantool-patches] " Konstantin Osipov
2019-07-19 18:08 ` [PATCH 2/4] txn: reverse commit trigger list only before running commit triggers Vladimir Davydov
` (2 subsequent siblings)
3 siblings, 1 reply; 23+ messages in thread
From: Vladimir Davydov @ 2019-07-19 18:08 UTC (permalink / raw)
To: tarantool-patches
To bring new rlist methods - rlist_cut_before and rlist_reverse.
Also, remove unit/rlist test as it is now a part of the small suite.
---
src/lib/small | 2 +-
test/unit/CMakeLists.txt | 2 -
test/unit/rlist.c | 137 -----------------------------------------------
test/unit/rlist.result | 88 ------------------------------
4 files changed, 1 insertion(+), 228 deletions(-)
delete mode 100644 test/unit/rlist.c
delete mode 100644 test/unit/rlist.result
diff --git a/src/lib/small b/src/lib/small
index 020716d8..0f029a60 160000
--- a/src/lib/small
+++ b/src/lib/small
@@ -1 +1 @@
-Subproject commit 020716d8a1337118a2afbfaaf4bd13210e640d5c
+Subproject commit 0f029a60d21aef7e47e500b2d12931447279bebd
diff --git a/test/unit/CMakeLists.txt b/test/unit/CMakeLists.txt
index 3b31ddfa..73ee0a97 100644
--- a/test/unit/CMakeLists.txt
+++ b/test/unit/CMakeLists.txt
@@ -17,8 +17,6 @@ add_executable(heap.test heap.c)
target_link_libraries(heap.test unit)
add_executable(heap_iterator.test heap_iterator.c)
target_link_libraries(heap_iterator.test unit)
-add_executable(rlist.test rlist.c)
-target_link_libraries(rlist.test unit)
add_executable(stailq.test stailq.c)
target_link_libraries(stailq.test unit)
add_executable(uri.test uri.c unit.c)
diff --git a/test/unit/rlist.c b/test/unit/rlist.c
deleted file mode 100644
index c0c29a3f..00000000
--- a/test/unit/rlist.c
+++ /dev/null
@@ -1,137 +0,0 @@
-#include "small/rlist.h"
-#include <stdio.h>
-#include <stdarg.h>
-#include "unit.h"
-
-
-#define PLAN 87
-
-#define ITEMS 7
-
-struct test {
- char ch;
- int no;
- struct rlist list;
-};
-
-static struct test items[ITEMS];
-
-static RLIST_HEAD(head);
-static RLIST_HEAD(head2);
-
-int
-main(void)
-{
- int i;
- struct test *it;
- struct rlist *rlist;
-
- plan(PLAN);
- ok(rlist_empty(&head), "list is empty");
- for (i = 0; i < ITEMS; i++) {
- items[i].no = i;
- rlist_add_tail(&head, &(items[i].list));
- }
- RLIST_HEAD(empty_list);
- ok(rlist_empty(&empty_list), "rlist_nil is empty");
- ok(rlist_empty(&head2), "head2 is empty");
- rlist_swap(&head2, &empty_list);
- ok(rlist_empty(&empty_list), "rlist_nil is empty after swap");
- ok(rlist_empty(&head2), "head2 is empty after swap");
- rlist_swap(&head, &head2);
- ok(rlist_empty(&head), "head is empty after swap");
- is(rlist_first(&head2), &items[0].list, "first item");
- is(rlist_last(&head2), &items[ITEMS - 1].list, "last item");
- i = 0;
- rlist_foreach(rlist, &head2) {
- is(rlist, &items[i].list, "element (foreach) %d", i);
- i++;
- }
- rlist_foreach_reverse(rlist, &head2) {
- i--;
- is(rlist, &items[i].list, "element (foreach_reverse) %d", i);
- }
- rlist_swap(&head2, &head);
-
-
- is(rlist_first(&head), &items[0].list, "first item");
- isnt(rlist_first(&head), &items[ITEMS - 1].list, "first item");
-
- is(rlist_last(&head), &items[ITEMS - 1].list, "last item");
- isnt(rlist_last(&head), &items[0].list, "last item");
-
- is(rlist_next(&head), &items[0].list, "rlist_next");
- is(rlist_prev(&head), &items[ITEMS - 1].list, "rlist_prev");
-
- i = 0;
- rlist_foreach(rlist, &head) {
- is(rlist, &items[i].list, "element (foreach) %d", i);
- i++;
- }
- rlist_foreach_reverse(rlist, &head) {
- i--;
- is(rlist, &items[i].list, "element (foreach_reverse) %d", i);
- }
-
-
- is(rlist_entry(&items[0].list, struct test, list), &items[0],
- "rlist_entry");
- is(rlist_first_entry(&head, struct test, list), &items[0],
- "rlist_first_entry");
- is(rlist_next_entry(&items[0], list), &items[1], "rlist_next_entry");
- is(rlist_prev_entry(&items[2], list), &items[1], "rlist_prev_entry");
-
-
- i = 0;
- rlist_foreach_entry(it, &head, list) {
- is(it, items + i, "element (foreach_entry) %d", i);
- i++;
- }
- rlist_foreach_entry_reverse(it, &head, list) {
- i--;
- is(it, items + i, "element (foreach_entry_reverse) %d", i);
- }
-
- rlist_del(&items[2].list);
- ok(rlist_empty(&head2), "head2 is empty");
- rlist_move(&head2, &items[3].list);
- ok(!rlist_empty(&head2), "head2 isnt empty");
- is(rlist_first_entry(&head2, struct test, list),
- &items[3], "Item was moved");
- rlist_move_tail(&head2, &items[4].list);
- rlist_foreach_entry(it, &head, list) {
- is(it, items + i, "element (second deleted) %d", i);
- i++;
- if (i == 2)
- i += 3;
- }
- rlist_foreach_entry_reverse(it, &head, list) {
- i--;
- if (i == 4)
- i -= 3;
- is(it, items + i, "element (second deleted) %d", i);
- }
-
-
- rlist_create(&head);
- ok(rlist_empty(&head), "list is empty");
- for (i = 0; i < ITEMS; i++) {
- items[i].no = i;
- rlist_add(&head, &(items[i].list));
- }
- i = 0;
- rlist_foreach_entry_reverse(it, &head, list) {
- is(it, items + i, "element (foreach_entry_reverse) %d", i);
- i++;
- }
- rlist_foreach_entry(it, &head, list) {
- i--;
- is(it, items + i, "element (foreach_entry) %d", i);
- }
- rlist_create(&head);
- rlist_add_entry(&head, &items[0], list);
- ok(rlist_prev_entry_safe(&items[0], &head, list) == NULL,
- "prev is null");
- return check_plan();
-}
-
diff --git a/test/unit/rlist.result b/test/unit/rlist.result
deleted file mode 100644
index fa99a87c..00000000
--- a/test/unit/rlist.result
+++ /dev/null
@@ -1,88 +0,0 @@
-1..87
-ok 1 - list is empty
-ok 2 - rlist_nil is empty
-ok 3 - head2 is empty
-ok 4 - rlist_nil is empty after swap
-ok 5 - head2 is empty after swap
-ok 6 - head is empty after swap
-ok 7 - first item
-ok 8 - last item
-ok 9 - element (foreach) 0
-ok 10 - element (foreach) 1
-ok 11 - element (foreach) 2
-ok 12 - element (foreach) 3
-ok 13 - element (foreach) 4
-ok 14 - element (foreach) 5
-ok 15 - element (foreach) 6
-ok 16 - element (foreach_reverse) 6
-ok 17 - element (foreach_reverse) 5
-ok 18 - element (foreach_reverse) 4
-ok 19 - element (foreach_reverse) 3
-ok 20 - element (foreach_reverse) 2
-ok 21 - element (foreach_reverse) 1
-ok 22 - element (foreach_reverse) 0
-ok 23 - first item
-ok 24 - first item
-ok 25 - last item
-ok 26 - last item
-ok 27 - rlist_next
-ok 28 - rlist_prev
-ok 29 - element (foreach) 0
-ok 30 - element (foreach) 1
-ok 31 - element (foreach) 2
-ok 32 - element (foreach) 3
-ok 33 - element (foreach) 4
-ok 34 - element (foreach) 5
-ok 35 - element (foreach) 6
-ok 36 - element (foreach_reverse) 6
-ok 37 - element (foreach_reverse) 5
-ok 38 - element (foreach_reverse) 4
-ok 39 - element (foreach_reverse) 3
-ok 40 - element (foreach_reverse) 2
-ok 41 - element (foreach_reverse) 1
-ok 42 - element (foreach_reverse) 0
-ok 43 - rlist_entry
-ok 44 - rlist_first_entry
-ok 45 - rlist_next_entry
-ok 46 - rlist_prev_entry
-ok 47 - element (foreach_entry) 0
-ok 48 - element (foreach_entry) 1
-ok 49 - element (foreach_entry) 2
-ok 50 - element (foreach_entry) 3
-ok 51 - element (foreach_entry) 4
-ok 52 - element (foreach_entry) 5
-ok 53 - element (foreach_entry) 6
-ok 54 - element (foreach_entry_reverse) 6
-ok 55 - element (foreach_entry_reverse) 5
-ok 56 - element (foreach_entry_reverse) 4
-ok 57 - element (foreach_entry_reverse) 3
-ok 58 - element (foreach_entry_reverse) 2
-ok 59 - element (foreach_entry_reverse) 1
-ok 60 - element (foreach_entry_reverse) 0
-ok 61 - head2 is empty
-ok 62 - head2 isnt empty
-ok 63 - Item was moved
-ok 64 - element (second deleted) 0
-ok 65 - element (second deleted) 1
-ok 66 - element (second deleted) 5
-ok 67 - element (second deleted) 6
-ok 68 - element (second deleted) 6
-ok 69 - element (second deleted) 5
-ok 70 - element (second deleted) 1
-ok 71 - element (second deleted) 0
-ok 72 - list is empty
-ok 73 - element (foreach_entry_reverse) 0
-ok 74 - element (foreach_entry_reverse) 1
-ok 75 - element (foreach_entry_reverse) 2
-ok 76 - element (foreach_entry_reverse) 3
-ok 77 - element (foreach_entry_reverse) 4
-ok 78 - element (foreach_entry_reverse) 5
-ok 79 - element (foreach_entry_reverse) 6
-ok 80 - element (foreach_entry) 6
-ok 81 - element (foreach_entry) 5
-ok 82 - element (foreach_entry) 4
-ok 83 - element (foreach_entry) 3
-ok 84 - element (foreach_entry) 2
-ok 85 - element (foreach_entry) 1
-ok 86 - element (foreach_entry) 0
-ok 87 - prev is null
--
2.11.0
^ permalink raw reply [flat|nested] 23+ messages in thread* [tarantool-patches] Re: [PATCH 1/4] Update small library
2019-07-19 18:08 ` [PATCH 1/4] Update small library Vladimir Davydov
@ 2019-07-24 22:48 ` Konstantin Osipov
2019-07-25 9:23 ` Vladimir Davydov
0 siblings, 1 reply; 23+ messages in thread
From: Konstantin Osipov @ 2019-07-24 22:48 UTC (permalink / raw)
To: tarantool-patches
* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/19 21:09]:
> To bring new rlist methods - rlist_cut_before and rlist_reverse.
> Also, remove unit/rlist test as it is now a part of the small suite.
Why do you need rlist_cut_before, what's wrong with rlist_splice?
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [tarantool-patches] Re: [PATCH 1/4] Update small library
2019-07-24 22:48 ` [tarantool-patches] " Konstantin Osipov
@ 2019-07-25 9:23 ` Vladimir Davydov
0 siblings, 0 replies; 23+ messages in thread
From: Vladimir Davydov @ 2019-07-25 9:23 UTC (permalink / raw)
To: Konstantin Osipov; +Cc: tarantool-patches
On Thu, Jul 25, 2019 at 01:48:53AM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/19 21:09]:
> > To bring new rlist methods - rlist_cut_before and rlist_reverse.
> > Also, remove unit/rlist test as it is now a part of the small suite.
>
> Why do you need rlist_cut_before, what's wrong with rlist_splice?
rlist_splice splices two lists (merges them in one) while rlist_cut cuts
a list (split it in two).
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/4] txn: reverse commit trigger list only before running commit triggers
2019-07-19 18:08 [PATCH 0/4] Support savepoints in DDL transactions Vladimir Davydov
2019-07-19 18:08 ` [PATCH 1/4] Update small library Vladimir Davydov
@ 2019-07-19 18:08 ` Vladimir Davydov
2019-07-24 22:48 ` [tarantool-patches] " Konstantin Osipov
2019-07-19 18:08 ` [PATCH 3/4] txn: use savepoints to roll back statements on yield or error Vladimir Davydov
2019-07-19 18:08 ` [PATCH 4/4] txn: undo commit/rollback triggers when reverting to savepoint Vladimir Davydov
3 siblings, 1 reply; 23+ messages in thread
From: Vladimir Davydov @ 2019-07-19 18:08 UTC (permalink / raw)
To: tarantool-patches
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
applies rlist_reverse to the commit trigger list before running them.
---
src/box/txn.c | 10 +++++++++-
src/box/txn.h | 13 ++-----------
src/lib/core/trigger.h | 11 -----------
test/engine/transaction.result | 4 ++--
4 files changed, 13 insertions(+), 25 deletions(-)
diff --git a/src/box/txn.c b/src/box/txn.c
index 73eaee73..0a42d440 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -381,8 +381,16 @@ txn_complete(struct txn *txn)
/* Commit the transaction. */
if (txn->engine != NULL)
engine_commit(txn->engine, txn);
- if (txn->has_triggers)
+ if (txn->has_triggers) {
+ /*
+ * Commit triggers must be run in the same
+ * order they were added. Since triggers
+ * are always added to the list head, we
+ * need to reverse the list.
+ */
+ rlist_reverse(&txn->on_commit);
txn_run_triggers(txn, &txn->on_commit);
+ }
double stop_tm = ev_monotonic_now(loop());
if (stop_tm - txn->start_tm > too_long_threshold) {
diff --git a/src/box/txn.h b/src/box/txn.h
index baeaa0ed..df173924 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -203,16 +203,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;
};
@@ -282,7 +273,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.h b/src/lib/core/trigger.h
index 4bc4da1a..d3ffb9a8 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)
{
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
---
--
2.11.0
^ permalink raw reply [flat|nested] 23+ messages in thread* [tarantool-patches] Re: [PATCH 2/4] txn: reverse commit trigger list only before running commit triggers
2019-07-19 18:08 ` [PATCH 2/4] txn: reverse commit trigger list only before running commit triggers Vladimir Davydov
@ 2019-07-24 22:48 ` Konstantin Osipov
2019-07-25 9:24 ` Vladimir Davydov
0 siblings, 1 reply; 23+ messages in thread
From: Konstantin Osipov @ 2019-07-24 22:48 UTC (permalink / raw)
To: tarantool-patches
* Vladimir Davydov <vdavydov.dev@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.
Since this is a *double linked list* you don't need to reverse it
to *iterate over it in reverse order*.
Thanks :(
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [tarantool-patches] Re: [PATCH 2/4] txn: reverse commit trigger list only before running commit triggers
2019-07-24 22:48 ` [tarantool-patches] " Konstantin Osipov
@ 2019-07-25 9:24 ` Vladimir Davydov
2019-07-25 9:29 ` Konstantin Osipov
0 siblings, 1 reply; 23+ messages in thread
From: Vladimir Davydov @ 2019-07-25 9:24 UTC (permalink / raw)
To: Konstantin Osipov; +Cc: tarantool-patches
On Thu, Jul 25, 2019 at 01:48:11AM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@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.
>
> Since this is a *double linked list* you don't need to reverse it
> to *iterate over it in reverse order*.
I can of course do that, but IMO this wouldn't make any difference.
^ permalink raw reply [flat|nested] 23+ messages in thread* [tarantool-patches] Re: [PATCH 2/4] txn: reverse commit trigger list only before running commit triggers
2019-07-25 9:24 ` Vladimir Davydov
@ 2019-07-25 9:29 ` Konstantin Osipov
2019-07-25 9:35 ` Vladimir Davydov
0 siblings, 1 reply; 23+ messages in thread
From: Konstantin Osipov @ 2019-07-25 9:29 UTC (permalink / raw)
To: tarantool-patches
* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/25 12:28]:
> On Thu, Jul 25, 2019 at 01:48:11AM +0300, Konstantin Osipov wrote:
> > * Vladimir Davydov <vdavydov.dev@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.
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [tarantool-patches] Re: [PATCH 2/4] txn: reverse commit trigger list only before running commit triggers
2019-07-25 9:29 ` Konstantin Osipov
@ 2019-07-25 9:35 ` Vladimir Davydov
2019-07-25 14:56 ` Vladimir Davydov
0 siblings, 1 reply; 23+ messages in thread
From: Vladimir Davydov @ 2019-07-25 9:35 UTC (permalink / raw)
To: Konstantin Osipov; +Cc: tarantool-patches
On Thu, Jul 25, 2019 at 12:29:48PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/25 12:28]:
> > On Thu, Jul 25, 2019 at 01:48:11AM +0300, Konstantin Osipov wrote:
> > > * Vladimir Davydov <vdavydov.dev@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.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [tarantool-patches] Re: [PATCH 2/4] txn: reverse commit trigger list only before running commit triggers
2019-07-25 9:35 ` Vladimir Davydov
@ 2019-07-25 14:56 ` Vladimir Davydov
2019-07-26 19:25 ` Konstantin Osipov
0 siblings, 1 reply; 23+ messages in thread
From: Vladimir Davydov @ 2019-07-25 14:56 UTC (permalink / raw)
To: Konstantin Osipov; +Cc: tarantool-patches
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@gmail.com> [19/07/25 12:28]:
> > > On Thu, Jul 25, 2019 at 01:48:11AM +0300, Konstantin Osipov wrote:
> > > > * Vladimir Davydov <vdavydov.dev@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@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
---
^ permalink raw reply [flat|nested] 23+ messages in thread* [tarantool-patches] Re: [PATCH 2/4] txn: reverse commit trigger list only before running commit triggers
2019-07-25 14:56 ` Vladimir Davydov
@ 2019-07-26 19:25 ` Konstantin Osipov
2019-07-29 8:45 ` Vladimir Davydov
0 siblings, 1 reply; 23+ messages in thread
From: Konstantin Osipov @ 2019-07-26 19:25 UTC (permalink / raw)
To: tarantool-patches
* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/25 17:57]:
LGTM
>
> 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.
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [tarantool-patches] Re: [PATCH 2/4] txn: reverse commit trigger list only before running commit triggers
2019-07-26 19:25 ` Konstantin Osipov
@ 2019-07-29 8:45 ` Vladimir Davydov
0 siblings, 0 replies; 23+ messages in thread
From: Vladimir Davydov @ 2019-07-29 8:45 UTC (permalink / raw)
To: Konstantin Osipov; +Cc: tarantool-patches
On Fri, Jul 26, 2019 at 10:25:56PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/25 17:57]:
>
> LGTM
> >
> > 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.
Pushed to master along with the small library update.
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 3/4] txn: use savepoints to roll back statements on yield or error
2019-07-19 18:08 [PATCH 0/4] Support savepoints in DDL transactions Vladimir Davydov
2019-07-19 18:08 ` [PATCH 1/4] Update small library Vladimir Davydov
2019-07-19 18:08 ` [PATCH 2/4] txn: reverse commit trigger list only before running commit triggers Vladimir Davydov
@ 2019-07-19 18:08 ` Vladimir Davydov
2019-07-24 22:55 ` [tarantool-patches] " Konstantin Osipov
2019-07-19 18:08 ` [PATCH 4/4] txn: undo commit/rollback triggers when reverting to savepoint Vladimir Davydov
3 siblings, 1 reply; 23+ messages in thread
From: Vladimir Davydov @ 2019-07-19 18:08 UTC (permalink / raw)
To: tarantool-patches
Currently, txn_savepoint objects are only used for savepoints created by
the user while internally we use stailq_entry instead. This is okay now,
because txn_savepoint is equivalent to a stailq_entry in most cases, but
in order to properly deal with commit/rollback triggers, we will need to
maintain extra information in each savepoint. So this patch makes txn
use txn_savepoint for internal needs.
Note that this patch increases txn::sub_stmt_begin array size by 1,
because we could actually write beyond the array bounds - it didn't
lead to any problems before, because it only overwrote txn::signature.
With the increased array entry size, it can overwrite more vital parts
of the txn struct.
---
src/box/txn.c | 44 ++++++++++++++++++++++++++++++++------------
src/box/txn.h | 7 ++++---
2 files changed, 36 insertions(+), 15 deletions(-)
diff --git a/src/box/txn.c b/src/box/txn.c
index 0a42d440..3f0017f8 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -52,6 +52,29 @@ fiber_set_txn(struct fiber *fiber, struct txn *txn)
fiber->storage.txn = txn;
}
+/**
+ * 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,
+ /* .stmt = */ NULL,
+ /* .fk_deferred_count = */ 0,
+};
+
+/**
+ * Create a savepoint that can be used to rollback to
+ * the current transaction state.
+ */
+static void
+txn_create_svp(struct txn *txn, struct txn_savepoint *svp)
+{
+ svp->in_sub_stmt = txn->in_sub_stmt;
+ svp->stmt = stailq_last(&txn->stmts);
+ if (txn->psql_txn != NULL)
+ svp->fk_deferred_count = txn->psql_txn->fk_deferred_count;
+}
+
static int
txn_add_redo(struct txn *txn, struct txn_stmt *stmt, struct request *request)
{
@@ -102,7 +125,7 @@ txn_stmt_new(struct txn *txn)
stmt->row = NULL;
/* Set the savepoint for statement rollback. */
- txn->sub_stmt_begin[txn->in_sub_stmt] = stailq_last(&txn->stmts);
+ txn_create_svp(txn, &txn->sub_stmt_begin[txn->in_sub_stmt]);
txn->in_sub_stmt++;
stailq_add_tail_entry(&txn->stmts, stmt, next);
@@ -119,11 +142,11 @@ txn_stmt_unref_tuples(struct txn_stmt *stmt)
}
static void
-txn_rollback_to_svp(struct txn *txn, struct stailq_entry *svp)
+txn_rollback_to_svp(struct txn *txn, struct txn_savepoint *svp)
{
struct txn_stmt *stmt;
struct stailq rollback;
- stailq_cut_tail(&txn->stmts, svp, &rollback);
+ stailq_cut_tail(&txn->stmts, svp->stmt, &rollback);
stailq_reverse(&rollback);
stailq_foreach_entry(stmt, &rollback, next) {
if (txn->engine != NULL && stmt->space != NULL)
@@ -142,6 +165,8 @@ txn_rollback_to_svp(struct txn *txn, struct stailq_entry *svp)
stmt->space = NULL;
stmt->row = NULL;
}
+ if (txn->psql_txn != NULL)
+ txn->psql_txn->fk_deferred_count = svp->fk_deferred_count;
}
/*
@@ -565,7 +590,7 @@ 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]);
+ txn_rollback_to_svp(txn, &txn->sub_stmt_begin[txn->in_sub_stmt]);
}
void
@@ -702,10 +727,7 @@ box_txn_savepoint()
"region", "struct txn_savepoint");
return NULL;
}
- svp->stmt = stailq_last(&txn->stmts);
- svp->in_sub_stmt = txn->in_sub_stmt;
- if (txn->psql_txn != NULL)
- svp->fk_deferred_count = txn->psql_txn->fk_deferred_count;
+ txn_create_svp(txn, svp);
return svp;
}
@@ -731,9 +753,7 @@ box_txn_rollback_to_savepoint(box_txn_savepoint_t *svp)
diag_set(ClientError, ER_NO_SUCH_SAVEPOINT);
return -1;
}
- txn_rollback_to_svp(txn, svp->stmt);
- if (txn->psql_txn != NULL)
- txn->psql_txn->fk_deferred_count = svp->fk_deferred_count;
+ txn_rollback_to_svp(txn, svp);
return 0;
}
@@ -769,7 +789,7 @@ txn_on_yield(struct trigger *trigger, void *event)
(void) event;
struct txn *txn = in_txn();
assert(txn != NULL && !txn->can_yield);
- txn_rollback_to_svp(txn, NULL);
+ txn_rollback_to_svp(txn, &zero_svp);
if (txn->has_triggers) {
txn_run_triggers(txn, &txn->on_rollback);
txn->has_triggers = false;
diff --git a/src/box/txn.h b/src/box/txn.h
index df173924..a1685b5f 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -182,7 +182,7 @@ struct txn {
* First statement at each statement-level.
* Needed to rollback sub statements.
*/
- struct stailq_entry *sub_stmt_begin[TXN_SUB_STMT_MAX];
+ struct txn_savepoint sub_stmt_begin[TXN_SUB_STMT_MAX + 1];
/** LSN of this transaction when written to WAL. */
int64_t signature;
/** Engine involved in multi-statement transaction. */
@@ -390,8 +390,9 @@ txn_current_stmt(struct txn *txn)
{
if (txn->in_sub_stmt == 0)
return NULL;
- struct stailq_entry *stmt = txn->sub_stmt_begin[txn->in_sub_stmt - 1];
- stmt = stmt != NULL ? stailq_next(stmt) : stailq_first(&txn->stmts);
+ struct txn_savepoint *svp = &txn->sub_stmt_begin[txn->in_sub_stmt - 1];
+ struct stailq_entry *stmt = svp->stmt != NULL ?
+ stailq_next(svp->stmt) : stailq_first(&txn->stmts);
return stailq_entry(stmt, struct txn_stmt, next);
}
--
2.11.0
^ permalink raw reply [flat|nested] 23+ messages in thread* [tarantool-patches] Re: [PATCH 3/4] txn: use savepoints to roll back statements on yield or error
2019-07-19 18:08 ` [PATCH 3/4] txn: use savepoints to roll back statements on yield or error Vladimir Davydov
@ 2019-07-24 22:55 ` Konstantin Osipov
2019-07-24 23:19 ` Konstantin Osipov
2019-07-25 11:57 ` Vladimir Davydov
0 siblings, 2 replies; 23+ messages in thread
From: Konstantin Osipov @ 2019-07-24 22:55 UTC (permalink / raw)
To: tarantool-patches
* Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/19 21:09]:
> Currently, txn_savepoint objects are only used for savepoints created by
> the user while internally we use stailq_entry instead. This is okay now,
> because txn_savepoint is equivalent to a stailq_entry in most cases, but
> in order to properly deal with commit/rollback triggers, we will need to
> maintain extra information in each savepoint. So this patch makes txn
> use txn_savepoint for internal needs.
>
> Note that this patch increases txn::sub_stmt_begin array size by 1,
> because we could actually write beyond the array bounds - it didn't
> lead to any problems before, because it only overwrote txn::signature.
> With the increased array entry size, it can overwrite more vital parts
> of the txn struct.
Please add a fix for txn->signature overwrite to 1.10 in a separate patch,
this part is LGTM.
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 23+ messages in thread
* [tarantool-patches] Re: [PATCH 3/4] txn: use savepoints to roll back statements on yield or error
2019-07-24 22:55 ` [tarantool-patches] " Konstantin Osipov
@ 2019-07-24 23:19 ` Konstantin Osipov
2019-07-25 9:28 ` Vladimir Davydov
2019-07-25 11:57 ` Vladimir Davydov
1 sibling, 1 reply; 23+ messages in thread
From: Konstantin Osipov @ 2019-07-24 23:19 UTC (permalink / raw)
To: tarantool-patches
* Konstantin Osipov <kostja@tarantool.org> [19/07/25 01:59]:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/19 21:09]:
> > Currently, txn_savepoint objects are only used for savepoints created by
> > the user while internally we use stailq_entry instead. This is okay now,
> > because txn_savepoint is equivalent to a stailq_entry in most cases, but
> > in order to properly deal with commit/rollback triggers, we will need to
> > maintain extra information in each savepoint. So this patch makes txn
> > use txn_savepoint for internal needs.
> >
> > Note that this patch increases txn::sub_stmt_begin array size by 1,
> > because we could actually write beyond the array bounds - it didn't
> > lead to any problems before, because it only overwrote txn::signature.
> > With the increased array entry size, it can overwrite more vital parts
> > of the txn struct.
>
> Please add a fix for txn->signature overwrite to 1.10 in a separate patch,
> this part is LGTM.
As to other parts, I don't understand the commit comment to begin
with.
Can there be a DDL in a sub-statement? What is the point of having
a full blown savepoint for sub-statement boundaries?
--
Konstantin Osipov, Moscow, Russia
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [tarantool-patches] Re: [PATCH 3/4] txn: use savepoints to roll back statements on yield or error
2019-07-24 23:19 ` Konstantin Osipov
@ 2019-07-25 9:28 ` Vladimir Davydov
0 siblings, 0 replies; 23+ messages in thread
From: Vladimir Davydov @ 2019-07-25 9:28 UTC (permalink / raw)
To: Konstantin Osipov; +Cc: tarantool-patches
On Thu, Jul 25, 2019 at 02:19:51AM +0300, Konstantin Osipov wrote:
> * Konstantin Osipov <kostja@tarantool.org> [19/07/25 01:59]:
> > * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/19 21:09]:
> > > Currently, txn_savepoint objects are only used for savepoints created by
> > > the user while internally we use stailq_entry instead. This is okay now,
> > > because txn_savepoint is equivalent to a stailq_entry in most cases, but
> > > in order to properly deal with commit/rollback triggers, we will need to
> > > maintain extra information in each savepoint. So this patch makes txn
> > > use txn_savepoint for internal needs.
> > >
> > > Note that this patch increases txn::sub_stmt_begin array size by 1,
> > > because we could actually write beyond the array bounds - it didn't
> > > lead to any problems before, because it only overwrote txn::signature.
> > > With the increased array entry size, it can overwrite more vital parts
> > > of the txn struct.
> >
> > Please add a fix for txn->signature overwrite to 1.10 in a separate patch,
> > this part is LGTM.
>
> As to other parts, I don't understand the commit comment to begin
> with.
>
> Can there be a DDL in a sub-statement?
Yes, there's nothing that prevents one from doing that.
Besides, one can install on_commit/on_rollback triggers from Lua.
> What is the point of having a full blown savepoint for sub-statement
> boundaries?
To remove/run on_commit/on_rollback triggers in txn_rollback_stmt.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [tarantool-patches] Re: [PATCH 3/4] txn: use savepoints to roll back statements on yield or error
2019-07-24 22:55 ` [tarantool-patches] " Konstantin Osipov
2019-07-24 23:19 ` Konstantin Osipov
@ 2019-07-25 11:57 ` Vladimir Davydov
1 sibling, 0 replies; 23+ messages in thread
From: Vladimir Davydov @ 2019-07-25 11:57 UTC (permalink / raw)
To: Konstantin Osipov; +Cc: tarantool-patches
On Thu, Jul 25, 2019 at 01:55:05AM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/19 21:09]:
> > Currently, txn_savepoint objects are only used for savepoints created by
> > the user while internally we use stailq_entry instead. This is okay now,
> > because txn_savepoint is equivalent to a stailq_entry in most cases, but
> > in order to properly deal with commit/rollback triggers, we will need to
> > maintain extra information in each savepoint. So this patch makes txn
> > use txn_savepoint for internal needs.
> >
> > Note that this patch increases txn::sub_stmt_begin array size by 1,
> > because we could actually write beyond the array bounds - it didn't
> > lead to any problems before, because it only overwrote txn::signature.
> > With the increased array entry size, it can overwrite more vital parts
> > of the txn struct.
>
> Please add a fix for txn->signature overwrite to 1.10 in a separate patch,
> this part is LGTM.
Done.
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 4/4] txn: undo commit/rollback triggers when reverting to savepoint
2019-07-19 18:08 [PATCH 0/4] Support savepoints in DDL transactions Vladimir Davydov
` (2 preceding siblings ...)
2019-07-19 18:08 ` [PATCH 3/4] txn: use savepoints to roll back statements on yield or error Vladimir Davydov
@ 2019-07-19 18:08 ` Vladimir Davydov
2019-07-19 19:36 ` [tarantool-patches] " Vladislav Shpilevoy
` (2 more replies)
3 siblings, 3 replies; 23+ messages in thread
From: Vladimir Davydov @ 2019-07-19 18:08 UTC (permalink / raw)
To: tarantool-patches
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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [tarantool-patches] [PATCH 4/4] txn: undo commit/rollback triggers when reverting to savepoint
2019-07-19 18:08 ` [PATCH 4/4] txn: undo commit/rollback triggers when reverting to savepoint Vladimir Davydov
@ 2019-07-19 19:36 ` Vladislav Shpilevoy
2019-07-19 19:42 ` Vladimir Davydov
2019-07-26 8:56 ` Vladimir Davydov
2019-07-29 13:11 ` [tarantool-patches] " Konstantin Osipov
2 siblings, 1 reply; 23+ messages in thread
From: Vladislav Shpilevoy @ 2019-07-19 19:36 UTC (permalink / raw)
To: tarantool-patches, Vladimir Davydov
On 19/07/2019 20:08, Vladimir Davydov wrote:
> 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
Nit: 'rollback trigger doesn't an already freed'. Perhaps I am wrong,
but shouldn't be here a verb after 'doesn't'?
> running rollback triggers, not before as we used to.
>
> Closes #4364
> Closes #4365
> ---
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [tarantool-patches] [PATCH 4/4] txn: undo commit/rollback triggers when reverting to savepoint
2019-07-19 19:36 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2019-07-19 19:42 ` Vladimir Davydov
0 siblings, 0 replies; 23+ messages in thread
From: Vladimir Davydov @ 2019-07-19 19:42 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
On Fri, Jul 19, 2019 at 09:36:08PM +0200, Vladislav Shpilevoy wrote:
>
>
> On 19/07/2019 20:08, Vladimir Davydov wrote:
> > 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
>
> Nit: 'rollback trigger doesn't an already freed'. Perhaps I am wrong,
> but shouldn't be here a verb after 'doesn't'?
Yep, I missed word 'access'. Fixed. Thanks.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/4] txn: undo commit/rollback triggers when reverting to savepoint
2019-07-19 18:08 ` [PATCH 4/4] txn: undo commit/rollback triggers when reverting to savepoint Vladimir Davydov
2019-07-19 19:36 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2019-07-26 8:56 ` Vladimir Davydov
2019-07-29 13:11 ` [tarantool-patches] " Konstantin Osipov
2 siblings, 0 replies; 23+ messages in thread
From: Vladimir Davydov @ 2019-07-26 8:56 UTC (permalink / raw)
To: Konstantin Osipov; +Cc: tarantool-patches
On Fri, Jul 19, 2019 at 09:08:42PM +0300, Vladimir Davydov wrote:
> 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
As discussed f2f, this and the previous patch are also needed to make
savepoints handle user-defined commit/rollback triggers correctly, i.e.
on rollback to a savepoint, remove commit triggers and run rollback
triggers installed after the savepoint. I added a test case to check
this behavior:
diff --git a/test/engine/transaction.result b/test/engine/transaction.result
index 910abb58..0b3fccf1 100644
--- a/test/engine/transaction.result
+++ b/test/engine/transaction.result
@@ -527,3 +527,77 @@ stmts
s:drop()
---
...
+--
+-- Check that on rollback to a savepoint we remove all commit
+-- triggers and run all rollback triggers installed after the
+-- savepoint.
+--
+s = box.schema.space.create('test', {engine = engine})
+---
+...
+_ = s:create_index('pk')
+---
+...
+commit = {}
+---
+...
+rollback = {}
+---
+...
+on_commit1 = function() table.insert(commit, 'on_commit_1') end
+---
+...
+on_commit2 = function() table.insert(commit, 'on_commit_2') end
+---
+...
+on_commit3 = function() table.insert(commit, 'on_commit_3') end
+---
+...
+on_rollback1 = function() table.insert(rollback, 'on_rollback_1') end
+---
+...
+on_rollback2 = function() table.insert(rollback, 'on_rollback_2') end
+---
+...
+on_rollback3 = function() table.insert(rollback, 'on_rollback_3') end
+---
+...
+inspector:cmd("setopt delimiter ';'")
+---
+- true
+...
+box.begin()
+box.on_commit(on_commit1)
+box.on_rollback(on_rollback1)
+sv = box.savepoint()
+s:insert{1}
+box.on_commit(on_commit2)
+box.on_rollback(on_rollback2)
+s:insert{2}
+box.on_commit(on_commit3)
+box.on_rollback(on_rollback3)
+box.rollback_to_savepoint(sv)
+s:insert{3}
+box.commit();
+---
+...
+inspector:cmd("setopt delimiter ''");
+---
+- true
+...
+commit -- on_commit_1
+---
+- - on_commit_1
+...
+rollback -- on_rollback_3, on_rollback_2
+---
+- - on_rollback_3
+ - on_rollback_2
+...
+s:select() -- 3
+---
+- - [3]
+...
+s:drop()
+---
+...
diff --git a/test/engine/transaction.test.lua b/test/engine/transaction.test.lua
index 51756f93..75bb5f66 100644
--- a/test/engine/transaction.test.lua
+++ b/test/engine/transaction.test.lua
@@ -228,3 +228,42 @@ box.begin() box.on_commit(on_commit) box.commit()
stmts
s:drop()
+
+--
+-- Check that on rollback to a savepoint we remove all commit
+-- triggers and run all rollback triggers installed after the
+-- savepoint.
+--
+s = box.schema.space.create('test', {engine = engine})
+_ = s:create_index('pk')
+
+commit = {}
+rollback = {}
+on_commit1 = function() table.insert(commit, 'on_commit_1') end
+on_commit2 = function() table.insert(commit, 'on_commit_2') end
+on_commit3 = function() table.insert(commit, 'on_commit_3') end
+on_rollback1 = function() table.insert(rollback, 'on_rollback_1') end
+on_rollback2 = function() table.insert(rollback, 'on_rollback_2') end
+on_rollback3 = function() table.insert(rollback, 'on_rollback_3') end
+
+inspector:cmd("setopt delimiter ';'")
+box.begin()
+box.on_commit(on_commit1)
+box.on_rollback(on_rollback1)
+sv = box.savepoint()
+s:insert{1}
+box.on_commit(on_commit2)
+box.on_rollback(on_rollback2)
+s:insert{2}
+box.on_commit(on_commit3)
+box.on_rollback(on_rollback3)
+box.rollback_to_savepoint(sv)
+s:insert{3}
+box.commit();
+inspector:cmd("setopt delimiter ''");
+
+commit -- on_commit_1
+rollback -- on_rollback_3, on_rollback_2
+s:select() -- 3
+
+s:drop()
^ permalink raw reply [flat|nested] 23+ messages in thread* [tarantool-patches] Re: [PATCH 4/4] txn: undo commit/rollback triggers when reverting to savepoint
2019-07-19 18:08 ` [PATCH 4/4] txn: undo commit/rollback triggers when reverting to savepoint Vladimir Davydov
2019-07-19 19:36 ` [tarantool-patches] " Vladislav Shpilevoy
2019-07-26 8:56 ` Vladimir Davydov
@ 2019-07-29 13:11 ` Konstantin Osipov
2019-07-30 10:54 ` Vladimir Davydov
2 siblings, 1 reply; 23+ messages in thread
From: Konstantin Osipov @ 2019-07-29 13:11 UTC (permalink / raw)
To: tarantool-patches
* 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
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [tarantool-patches] Re: [PATCH 4/4] txn: undo commit/rollback triggers when reverting to savepoint
2019-07-29 13:11 ` [tarantool-patches] " Konstantin Osipov
@ 2019-07-30 10:54 ` Vladimir Davydov
0 siblings, 0 replies; 23+ messages in thread
From: Vladimir Davydov @ 2019-07-30 10:54 UTC (permalink / raw)
To: Konstantin Osipov; +Cc: tarantool-patches
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.
^ permalink raw reply [flat|nested] 23+ messages in thread