[tarantool-patches] Re: [PATCH] sql: fix row count calculation for DELETE optimization

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

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.
>