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 887DE2CC03 for ; Mon, 19 Nov 2018 07:50:24 -0500 (EST) 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 hbGRMopP12Kf for ; Mon, 19 Nov 2018 07:50:24 -0500 (EST) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 2E60D2CC01 for ; Mon, 19 Nov 2018 07:50:24 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH] sql: fix row count calculation for DELETE optimization References: <20181119121256.8154-1-korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <98a013cf-1c67-a5e9-0006-8384e57c4917@tarantool.org> Date: Mon, 19 Nov 2018 15:50:21 +0300 MIME-Version: 1.0 In-Reply-To: <20181119121256.8154-1-korablev@tarantool.org> 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: tarantool-patches@freelists.org, Nikita Pettik Thanks for the patch! LGTM. On 19/11/2018 15:12, Nikita Pettik wrote: > When SQL DELETE statement comes in most primitive from without WHERE > clause and foreign key constraints, it is optimized and processed with > one VDBE instruction (instead of several OP_Delete). However, it was > forgotten to account affected tuples by row counter. Current patch fixes > this obvious defect. > > Closes #3816 > --- > Branch: https://github.com/tarantool/tarantool/tree/np/gh-3816-account-changes-for-simple-delete > Issue: https://github.com/tarantool/tarantool/issues/3816 > > src/box/sql.c | 5 ++++- > src/box/sql/delete.c | 1 + > src/box/sql/tarantoolInt.h | 2 +- > src/box/sql/vdbe.c | 10 +++++++-- > test/sql/row-count.result | 54 +++++++++++++++++++++++++++++++++++++++++++++ > test/sql/row-count.test.lua | 20 +++++++++++++++++ > 6 files changed, 88 insertions(+), 4 deletions(-) > > diff --git a/src/box/sql.c b/src/box/sql.c > index d7df84874..c3418008c 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -579,8 +579,10 @@ int tarantoolSqlite3EphemeralClearTable(BtCursor *pCur) > * Removes all instances from table. > * Iterate through the space and delete one by one all tuples. > */ > -int tarantoolSqlite3ClearTable(struct space *space) > +int tarantoolSqlite3ClearTable(struct space *space, uint32_t *tuple_count) > { > + assert(tuple_count != NULL); > + *tuple_count = 0; > uint32_t key_size; > box_tuple_t *tuple; > int rc; > @@ -602,6 +604,7 @@ int tarantoolSqlite3ClearTable(struct space *space) > iterator_delete(iter); > return SQL_TARANTOOL_DELETE_FAIL; > } > + (*tuple_count)++; > } > iterator_delete(iter); > > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c > index 116832f90..f9c42fdec 100644 > --- a/src/box/sql/delete.c > +++ b/src/box/sql/delete.c > @@ -210,6 +210,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, > assert(!is_view); > > sqlite3VdbeAddOp1(v, OP_Clear, space->def->id); > + sqlite3VdbeChangeP5(v, OPFLAG_NCHANGE); > > /* Do not start Tarantool's transaction in case of > * truncate optimization. This is workaround until > diff --git a/src/box/sql/tarantoolInt.h b/src/box/sql/tarantoolInt.h > index 1ba8476fc..3ff14d53a 100644 > --- a/src/box/sql/tarantoolInt.h > +++ b/src/box/sql/tarantoolInt.h > @@ -58,7 +58,7 @@ int > sql_delete_by_key(struct space *space, uint32_t iid, char *key, > uint32_t key_size); > > -int tarantoolSqlite3ClearTable(struct space *space); > +int tarantoolSqlite3ClearTable(struct space *space, uint32_t *tuple_count); > > /** > * Rename the table in _space. Update tuple with corresponding id > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index b6afe9184..ff8bf2afc 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -4644,7 +4644,7 @@ case OP_IdxGE: { /* jump */ > break; > } > > -/* Opcode: Clear P1 P2 * * * > +/* Opcode: Clear P1 P2 * * P5 > * Synopsis: space id = P1 > * If P2 is not 0, use Truncate semantics. > * > @@ -4652,6 +4652,9 @@ case OP_IdxGE: { /* jump */ > * in P1 argument. It is worth mentioning, that clearing routine > * doesn't involve truncating, since it features completely > * different mechanism under hood. > + * > + * If the OPFLAG_NCHANGE flag is set, then the row change count > + * is incremented by the number of deleted tuples. > */ > case OP_Clear: { > assert(pOp->p1 > 0); > @@ -4663,7 +4666,10 @@ case OP_Clear: { > if (box_truncate(space_id) != 0) > rc = SQL_TARANTOOL_ERROR; > } else { > - rc = tarantoolSqlite3ClearTable(space); > + uint32_t tuple_count; > + rc = tarantoolSqlite3ClearTable(space, &tuple_count); > + if (rc == 0 && (pOp->p5 & OPFLAG_NCHANGE) != 0) > + p->nChange += tuple_count; > } > if (rc) goto abort_due_to_error; > break; > diff --git a/test/sql/row-count.result b/test/sql/row-count.result > index 7577d2795..d4c86ac2b 100644 > --- a/test/sql/row-count.result > +++ b/test/sql/row-count.result > @@ -100,6 +100,60 @@ box.sql.execute("SELECT ROW_COUNT();") > --- > - - [3] > ... > +-- gh-3816: DELETE optimization returns valid number of > +-- deleted tuples. > +-- > +box.sql.execute("DELETE FROM t3 WHERE 0 = 0;") > +--- > +... > +box.sql.execute("SELECT ROW_COUNT();") > +--- > +- - [3] > +... > +box.sql.execute("INSERT INTO t3 VALUES (1, 1), (2, 2), (3, 3);") > +--- > +... > +box.sql.execute("DELETE FROM t3") > +--- > +... > +box.sql.execute("SELECT ROW_COUNT();") > +--- > +- - [3] > +... > +-- But triggers still should't be accounted. > +-- > +box.sql.execute("CREATE TABLE tt1 (id INT PRIMARY KEY);") > +--- > +... > +box.sql.execute("CREATE TABLE tt2 (id INT PRIMARY KEY);") > +--- > +... > +box.sql.execute("CREATE TRIGGER tr1 AFTER DELETE ON tt1 BEGIN DELETE FROM tt2; END;") > +--- > +... > +box.sql.execute("INSERT INTO tt1 VALUES (1), (2), (3);") > +--- > +... > +box.sql.execute("INSERT INTO tt2 VALUES (1), (2), (3);") > +--- > +... > +box.sql.execute("DELETE FROM tt1 WHERE id = 2;") > +--- > +... > +box.sql.execute("SELECT ROW_COUNT();") > +--- > +- - [1] > +... > +box.sql.execute("SELECT * FROM tt2;") > +--- > +- [] > +... > +box.sql.execute("DROP TABLE tt1;") > +--- > +... > +box.sql.execute("DROP TABLE tt2;") > +--- > +... > -- All statements which are not accounted as DML should > -- return 0 (zero) as a row count. > -- > diff --git a/test/sql/row-count.test.lua b/test/sql/row-count.test.lua > index c29e1d051..45a39d19a 100644 > --- a/test/sql/row-count.test.lua > +++ b/test/sql/row-count.test.lua > @@ -31,6 +31,26 @@ box.sql.execute("SELECT ROW_COUNT();") > box.sql.execute("INSERT INTO t3 VALUES (1, 1), (2, 2), (3, 3);") > box.sql.execute("UPDATE t3 SET i2 = 666;") > box.sql.execute("SELECT ROW_COUNT();") > +-- gh-3816: DELETE optimization returns valid number of > +-- deleted tuples. > +-- > +box.sql.execute("DELETE FROM t3 WHERE 0 = 0;") > +box.sql.execute("SELECT ROW_COUNT();") > +box.sql.execute("INSERT INTO t3 VALUES (1, 1), (2, 2), (3, 3);") > +box.sql.execute("DELETE FROM t3") > +box.sql.execute("SELECT ROW_COUNT();") > +-- But triggers still should't be accounted. > +-- > +box.sql.execute("CREATE TABLE tt1 (id INT PRIMARY KEY);") > +box.sql.execute("CREATE TABLE tt2 (id INT PRIMARY KEY);") > +box.sql.execute("CREATE TRIGGER tr1 AFTER DELETE ON tt1 BEGIN DELETE FROM tt2; END;") > +box.sql.execute("INSERT INTO tt1 VALUES (1), (2), (3);") > +box.sql.execute("INSERT INTO tt2 VALUES (1), (2), (3);") > +box.sql.execute("DELETE FROM tt1 WHERE id = 2;") > +box.sql.execute("SELECT ROW_COUNT();") > +box.sql.execute("SELECT * FROM tt2;") > +box.sql.execute("DROP TABLE tt1;") > +box.sql.execute("DROP TABLE tt2;") > > -- All statements which are not accounted as DML should > -- return 0 (zero) as a row count. >