* [tarantool-patches] [PATCH] sql: fix row count calculation for DELETE optimization
@ 2018-11-19 12:12 Nikita Pettik
2018-11-19 12:50 ` [tarantool-patches] " Vladislav Shpilevoy
2018-11-23 5:51 ` Kirill Yukhin
0 siblings, 2 replies; 3+ messages in thread
From: Nikita Pettik @ 2018-11-19 12:12 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: fix row count calculation for DELETE optimization
2018-11-19 12:12 [tarantool-patches] [PATCH] sql: fix row count calculation for DELETE optimization Nikita Pettik
@ 2018-11-19 12:50 ` Vladislav Shpilevoy
2018-11-23 5:51 ` Kirill Yukhin
1 sibling, 0 replies; 3+ messages in thread
From: Vladislav Shpilevoy @ 2018-11-19 12:50 UTC (permalink / raw)
To: tarantool-patches, 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.
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* [tarantool-patches] Re: [PATCH] sql: fix row count calculation for DELETE optimization
2018-11-19 12:12 [tarantool-patches] [PATCH] sql: fix row count calculation for DELETE optimization Nikita Pettik
2018-11-19 12:50 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-11-23 5:51 ` Kirill Yukhin
1 sibling, 0 replies; 3+ messages in thread
From: Kirill Yukhin @ 2018-11-23 5:51 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Nikita Pettik
Hello,
On 19 Nov 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
I've pushed the patch into 2.1 branch.
--
Regards, Kirill Yukhin
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-11-23 5:51 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-19 12:12 [tarantool-patches] [PATCH] sql: fix row count calculation for DELETE optimization Nikita Pettik
2018-11-19 12:50 ` [tarantool-patches] " Vladislav Shpilevoy
2018-11-23 5:51 ` Kirill Yukhin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox