[tarantool-patches] [PATCH v3 3/5] Require for single statement not autocommit in case of ddl
Vladimir Davydov
vdavydov.dev at gmail.com
Wed Mar 27 13:49:12 MSK 2019
On Fri, Mar 22, 2019 at 03:06:08PM +0300, Georgy Kirichenko wrote:
> Allow single statement transactions within begin/commit in case of an
> ddl operation instead of auto commit requirements. This is essential
> for a transactional applier.
>
> Needed for: #2798
> ---
> src/box/memtx_engine.c | 20 ++------
> src/box/txn.c | 9 +++-
> src/box/txn.h | 5 ++
> test/box/ddl.result | 8 +--
> test/box/ddl.test.lua | 4 +-
> test/box/transaction.result | 53 +++++---------------
> test/box/transaction.test.lua | 27 ++++------
> test/engine/ddl.result | 92 ++++++++++++++++++++++++++++++++++
> test/engine/ddl.test.lua | 55 ++++++++++++++++++++
> test/engine/truncate.result | 3 +-
> test/sql-tap/trigger1.test.lua | 12 ++---
> test/sql/delete.result | 8 ++-
> test/sql/delete.test.lua | 3 +-
> 13 files changed, 204 insertions(+), 95 deletions(-)
>
> diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
> index d468d1cd8..924f8bbc4 100644
> --- a/src/box/memtx_engine.c
> +++ b/src/box/memtx_engine.c
> @@ -384,15 +384,7 @@ static int
> memtx_engine_begin(struct engine *engine, struct txn *txn)
> {
> (void)engine;
> - /*
> - * Register a trigger to rollback transaction on yield.
> - * This must be done in begin(), since it's
> - * the first thing txn invokes after txn->n_stmts++,
> - * to match with trigger_clear() in rollbackStatement().
> - */
> - if (txn->is_autocommit == false) {
> - memtx_init_txn(txn);
> - }
> + (void)txn;
> return 0;
> }
>
> @@ -404,15 +396,9 @@ memtx_engine_begin_statement(struct engine *engine, struct txn *txn)
> if (txn->engine_tx == NULL) {
> struct space *space = txn_last_stmt(txn)->space;
>
> - if (space->def->id > BOX_SYSTEM_ID_MAX &&
> - ! rlist_empty(&space->on_replace)) {
> - /**
> - * A space on_replace trigger may initiate
> - * a yield.
> - */
> - assert(txn->is_autocommit);
> + if (space->def->id > BOX_SYSTEM_ID_MAX)
> + /* Setup triggers for non-ddl transactions. */
> memtx_init_txn(txn);
> - }
> }
> return 0;
> }
> diff --git a/src/box/txn.c b/src/box/txn.c
> index deb4fac47..31e19951f 100644
> --- a/src/box/txn.c
> +++ b/src/box/txn.c
> @@ -147,6 +147,7 @@ txn_begin(bool is_autocommit)
> txn->n_local_rows = 0;
> txn->n_remote_rows = 0;
> txn->is_autocommit = is_autocommit;
> + txn->is_single_statement = false;
> txn->has_triggers = false;
> txn->is_aborted = false;
> txn->in_sub_stmt = 0;
> @@ -191,6 +192,11 @@ txn_begin_stmt(struct space *space)
> diag_set(ClientError, ER_SUB_STMT_MAX);
> return NULL;
> }
> + if (txn->is_single_statement && !stailq_empty(&txn->stmts)) {
> + diag_set(ClientError, ER_UNSUPPORTED,
> + "DDL", "multi-statement transactions");
> + return NULL;
> + }
>
> struct txn_stmt *stmt = txn_stmt_new(txn);
> if (stmt == NULL) {
> @@ -430,11 +436,12 @@ txn_abort(struct txn *txn)
> int
> txn_check_singlestatement(struct txn *txn, const char *where)
> {
> - if (!txn->is_autocommit || !txn_is_first_statement(txn)) {
> + if (!txn_is_first_statement(txn)) {
> diag_set(ClientError, ER_UNSUPPORTED,
> where, "multi-statement transactions");
> return -1;
> }
> + txn->is_single_statement = true;
> return 0;
> }
Judging by the function name and protocol, txn_check_singlestatement
isn't supposed to update txn fields. Relying on the fact that it is
called by any DDL operation (and only by DDL) is cumbersome IMO.
May be, better mark a transaction as DDL in begin_statement callback and
fail subsequent statements if the transaction is already marked?
> diff --git a/test/engine/ddl.test.lua b/test/engine/ddl.test.lua
> index cdaf7a5bf..636f6c3b9 100644
> --- a/test/engine/ddl.test.lua
> +++ b/test/engine/ddl.test.lua
> @@ -759,6 +759,9 @@ i3:select()
>
> -- Check that recovery works.
> inspector:cmd("restart server default")
> +test_run = require('test_run')
> +inspector = test_run.new()
> +engine = inspector:get_cfg('engine')
These two should remain set after restart. No need to reset them.
>
> s = box.space.test
> s.index.i1:select()
> @@ -775,3 +778,55 @@ s.index.i1:select()
> box.snapshot()
>
> s:drop()
> +
> +-- test ddl operation within begin/commit/rollback
> +-- acquire free space id
> +space = box.schema.space.create('ddl_test', {engine = engine})
> +id = space.id
> +space:drop()
> +
> +inspector:cmd("setopt delimiter ';'")
> +box.begin()
> +s = box.schema.space.create('ddl_test', {engine = engine, id = id})
> +box.rollback();
> +
> +box.begin()
> +s = box.schema.space.create('ddl_test', {engine = engine, id = id})
> +box.commit();
> +
> +box.begin()
> +s:create_index('pk')
> +box.rollback();
> +
> +box.begin()
> +s:create_index('pk')
> +box.commit();
> +
> +s:replace({1});
> +s:replace({2});
> +s:replace({3});
> +
> +box.begin()
> +s:truncate()
> +box.commit();
> +s:select();
> +
> +box.begin()
> +box.schema.user.grant('guest', 'write', 'space', 'ddl_test')
> +box.rollback();
> +
> +box.begin()
> +box.schema.user.grant('guest', 'write', 'space', 'ddl_test')
> +box.commit();
> +
> +box.begin()
> +box.schema.user.revoke('guest', 'write', 'space', 'ddl_test')
> +box.rollback();
> +
> +box.begin()
> +box.schema.user.revoke('guest', 'write', 'space', 'ddl_test')
> +box.commit();
Shouldn't we also check that mixing DDL with DML and combining several
DDL operations in one transaction still fail?
> +
> +-- index and space drop are not currently supported (because of truncate)
What do you mean by 'truncate'? Dropping indexes and grants? Please fix
the comment.
> +s.index.pk:drop();
> +s:drop();
More information about the Tarantool-patches
mailing list