From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 27 Mar 2019 13:49:12 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] [PATCH v3 3/5] Require for single statement not autocommit in case of ddl Message-ID: <20190327104912.bnid4qyzweko7gcz@esperanza> References: <6073eba9d945e40086b6bd2cd72e49f6a0bbc8db.1553255718.git.georgy@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6073eba9d945e40086b6bd2cd72e49f6a0bbc8db.1553255718.git.georgy@tarantool.org> To: Georgy Kirichenko Cc: tarantool-patches@freelists.org List-ID: 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();