From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 4807E23A47 for ; Fri, 4 May 2018 10:12:36 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id dq8asgPkRmtH for ; Fri, 4 May 2018 10:12:36 -0400 (EDT) Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 01A2D23926 for ; Fri, 4 May 2018 10:12:35 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 3/4] sql: allow SAVEPOINT statement outside transaction References: From: Vladislav Shpilevoy Message-ID: Date: Fri, 4 May 2018 17:12:32 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Nikita Pettik , tarantool-patches@freelists.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.