Could we consider the possibility to assign triggers on a statement itself? In this case you shouldn't implement all that machinery with savepoints. On Thursday, July 18, 2019 6:45:56 PM MSK Vladimir Davydov wrote: > Rollback triggers expect to see the database state after reverting all > changes done by the transaction. At the same time they may use tuples > referenced by reverted statements. > > As a result, using txn_rollback_to_svp(), which both reverts changes and > releases tuples, on yield in a DDL transaction may result in a crash. > The crash can only be reproduced using SQL, because Lua implicitly > references all tuples anyway. > > Thanks to @Gerold103 for reporting this issue and carrying out > preliminary investigation. > > Closes #4365 > --- > https://github.com/tarantool/tarantool/issues/4365 > https://github.com/tarantool/tarantool/commits/dv/gh-4365-fix-tx-ddl-crash-o > n-yield > > src/box/txn.c | 22 +++++++++++++++++++++- > test/box/transaction.result | 9 +++++++++ > test/box/transaction.test.lua | 6 ++++++ > 3 files changed, 36 insertions(+), 1 deletion(-) > > diff --git a/src/box/txn.c b/src/box/txn.c > index 73eaee73..6d844af2 100644 > --- a/src/box/txn.c > +++ b/src/box/txn.c > @@ -761,10 +761,30 @@ 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); > + /* > + * Rollback triggers expect to see the database state > + * after reverting all changes done by the transaction. > + * At the same time they may use tuples referenced by > + * reverted statements. So we rollback statements before > + * running the triggers, but postpone tuple release until > + * after the triggers have been invoked. > + */ > + struct txn_stmt *stmt; > + stailq_foreach_entry(stmt, &txn->stmts, next) { > + if (stmt->space != NULL) > + engine_rollback_statement(txn->engine, txn, stmt); > + stmt->space = NULL; > + stmt->row = NULL; > + } > if (txn->has_triggers) { > txn_run_triggers(txn, &txn->on_rollback); > txn->has_triggers = false; > } > + stailq_foreach_entry(stmt, &txn->stmts, next) > + txn_stmt_unref_tuples(stmt); > + stailq_create(&txn->stmts); > + txn->n_new_rows = 0; > + txn->n_local_rows = 0; > + txn->n_applier_rows = 0; > txn->is_aborted_by_yield = true; > } > diff --git a/test/box/transaction.result b/test/box/transaction.result > index 9a48f2f7..771ad96e 100644 > --- a/test/box/transaction.result > +++ b/test/box/transaction.result > @@ -778,3 +778,12 @@ box.begin() drop() box.rollback() > box.begin() drop() box.commit() > --- > ... > +-- > +-- gh-4365: DDL revered 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..d8a6e60a 100644 > --- a/test/box/transaction.test.lua > +++ b/test/box/transaction.test.lua > @@ -415,3 +415,9 @@ box.begin() create() box.rollback() > box.begin() create() box.commit() > box.begin() drop() box.rollback() > box.begin() drop() box.commit() > + > +-- > +-- gh-4365: DDL revered by yield triggers crash. > +-- > +box.begin() box.execute([[CREATE TABLE test(id INTEGER PRIMARY KEY > AUTOINCREMENT)]]) fiber.yield() +box.rollback()