[tarantool-patches] Re: [PATCH 3/4] sql: allow SAVEPOINT statement outside transaction

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hello. Thanks for contributing!

See below 3 comments.

On 03/05/2018 21:49, Nikita Pettik wrote:
> Before this patch, usage of SAVEPOINT statement outside transaction or
> inside transaction started in Lua, led to assertion fault.
> Now, failed assert is substituted with checks to test transaction status.
> 
> Closes #3313
> ---
>   src/box/sql/vdbe.c                               |  8 +++++-
>   test/sql/gh-3313-savepoints-outside-txn.result   | 32 ++++++++++++++++++++++++
>   test/sql/gh-3313-savepoints-outside-txn.test.lua | 18 +++++++++++++
>   3 files changed, 57 insertions(+), 1 deletion(-)
>   create mode 100644 test/sql/gh-3313-savepoints-outside-txn.result
>   create mode 100644 test/sql/gh-3313-savepoints-outside-txn.test.lua
> 
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 1192fc399..6ea04901c 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -2867,7 +2867,13 @@ case OP_Savepoint: {
>   	Savepoint *pTmp;
>   	struct sql_txn *psql_txn = p->psql_txn;
>   
> -	assert(psql_txn);
> +	if (psql_txn == NULL) {
> +		assert(!box_txn());
> +		sqlite3VdbeError(p, "cannot process savepoint: "
> +				    "there is no active transaction");
> +		rc = SQLITE_ERROR;
> +		goto abort_due_to_error;
> +	}

1. Lets use diag_set(ClientError, ER_SAVEPOINT_NO_TRANSACTION) and return TARANTOOL_ERROR.
It must behave like box.savepoint.
> diff --git a/test/sql/gh-3313-savepoints-outside-txn.result b/test/sql/gh-3313-savepoints-outside-txn.result
> new file mode 100644
> index 000000000..702d3e815
> --- /dev/null
> +++ b/test/sql/gh-3313-savepoints-outside-txn.result
> @@ -0,0 +1,32 @@
> +test_run = require('test_run').new()
> +---
> +...
> +test_run:cmd("setopt delimiter ';'")

2. Why do you need here a special delimiter? Your test does not
contain multi-line statements.

> +---
> +- true
> +...
> +-- These tests check that SQL savepoints properly work outside
> +-- transactions as well as inside transactions started in Lua.
> +--
> +-- box.cfg()

3. Garbage comment.