Re: [tarantool-patches] [PATCH v3 3/5] Require for single statement not autocommit in case of ddl

[Vladimir Davydov <vdavydov.dev@gmail.com>]

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();