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 EF5C02C39A for ; Mon, 19 Nov 2018 07:13:10 -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 frS7BvePWgK7 for ; Mon, 19 Nov 2018 07:13:10 -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 451992C33F for ; Mon, 19 Nov 2018 07:13:10 -0500 (EST) From: Nikita Pettik Subject: [tarantool-patches] [PATCH] sql: fix row count calculation for DELETE optimization Date: Mon, 19 Nov 2018 15:12:56 +0300 Message-Id: <20181119121256.8154-1-korablev@tarantool.org> 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 Cc: v.shpilevoy@tarantool.org, Nikita Pettik 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. -- 2.15.1