From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: =?utf-8?B?0JPQtdC+0YDQs9C40Lkg0JrQuNGA0LjRh9C10L3QutC+?= Subject: Re: [tarantool-patches] [PATCH] txn: don't unref stmt tuples before rollback triggers are run Date: Wed, 24 Jul 2019 15:30:47 +0300 Message-ID: <6520487.SKVt4zzWEJ@home.lan> In-Reply-To: References: MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart2010689.tenYudaoSU"; micalg="pgp-sha256"; protocol="application/pgp-signature" To: tarantool-patches@freelists.org Cc: Vladimir Davydov , kostja@tarantool.org List-ID: --nextPart2010689.tenYudaoSU Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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() --nextPart2010689.tenYudaoSU Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part. Content-Transfer-Encoding: 7Bit -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEFB+nbqWGnp59Rk9ZFSyY70x8X3sFAl04T3cACgkQFSyY70x8 X3u2QAf/b/Avvsx5LQU3bt1ItryV8uNGIvsKLwplj76QEp0LLyJ5n8EqUa6h6MA5 mXY4oHAxJilpUiAX4opH3gdL//DFmFqj6aGlfLqMadCSPVVctzRyOMtxfgswlmHw JmOtfhbAJpImi4zu8LIeZvHRlqdzJaG2wVKkSiB8jnpN393y1m51rjmpNHML8CfO lAa8Enp0E6/9bCIuHDnaPI9nBUJmJnm+1NKOi0EubC7qX3QyZd3JCeoameRxvrli PA1ycncxGWMeGbByUDc5Yr5TSnSJt1/tkrM6rCwuySFSJzEqh1Kow2uWVGR/bmFF AYoOa6xpzyJ1L2ret7fp5xuHjj+fMg== =h36D -----END PGP SIGNATURE----- --nextPart2010689.tenYudaoSU--