[tarantool-patches] Re: [PATCH v1 1/1] sql: introduce TRUNCATE TABLE operation

[n.pettik]

> On 18 Jul 2018, at 16:01, Kirill Shcherbatov <kshcherbatov at tarantool.org> wrote:
> 
>> Hello. I do not see, where do you call box_truncate, and where is a
>> test that it is called.
> I've discussed this problem with Nikita and we have decided to make separate OP_Truncate code.

Should we document somehow this feature (new truncate statement)?
I have heard about doc-bot (but still have never used it tho).

Well, personally I would implement this feature in other way.
I stick to the point that TRUNCATE should be executed only if
user asks for TRUNCATE (using TRUNCATE statement) and vice versa:
DELETE mustn’t result in TRUNCATE.
Not so long ago we deliberately removed truncate from clearing routine.
In fact, you return back previous behaviour which was disabled by this commit:
https://github.com/tarantool/tarantool/commit/290a5c9a08d469be9954df9e7d6d33f5743826c9

Lets ask smb (Vlad or Kirill) for advice, but that is my point.

Actually, TRUNCATE is quite sophisticated (or at least dubious) feature in its behaviour.
I would also ask Peter for his expert opinion, since TRUNCATE is ANSI extension and
is not described there. For instance, TRUNCATE in Postgres and MySQL
doesn’t fire ON DELETE triggers (but fires ON TRUNCATE ones). In your implementation it
might fire, if TRUNCATE launched inside transaction. 
Either, it can’t be executed on referenced table:

create table t1(id int primary key, a int) \\
create table t2(id int primary key references t1) \\
insert into t1 values (1, 2) \\
insert into t2 values (1) \\
truncate t1 \\

0A000: cannot truncate a table referenced in a foreign key constraint

How does TRUNCATE work with IPROTO requests? In MySQL TRUNCATE
doesn’t return number of deleted rows, for example. At least add tests on this case.

> /* Number of keywords */
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index a64d723..be4c860 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1953,7 +1953,7 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
> 	 * On memory allocation error sql_table delete_from
> 	 * releases memory for its own.
> 	 */
> -	sql_table_delete_from(parse, src_list, where);
> +	sql_table_delete_from(parse, src_list, where, false);
> }
> 
> /**
> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> index f9d3498..ab6771d 100644
> --- a/src/box/sql/delete.c
> +++ b/src/box/sql/delete.c
> @@ -73,7 +73,7 @@ sql_materialize_view(struct Parse *parse, const char *name, struct Expr *where,
> 
> void
> sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
> -		      struct Expr *where)
> +		      struct Expr *where, bool allow_truncate)

For bool flags we usually use ‘is_’ or ‘if_’ prefixes.

> {
> 	struct sqlite3 *db = parse->db;
> 	if (parse->nErr || db->mallocFailed)
> @@ -185,9 +185,10 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
> 	 */
> 	if (where == NULL && !is_complex) {
> 		assert(!is_view);
> -
> -		sqlite3VdbeAddOp1(v, OP_Clear, space_id);
> -
> +		if (!allow_truncate || space_is_system(space) || in_txn() != NULL)

It unlikely to be OK checking active transaction in parser.
When we will have cache for prepared statements, these stmts
might be compiled firstly and then executed together (without re-compilcation):

BEGIN; // Only generate OP_StartTransaction, but not execute it.
DELETE FROM t1; // It would generate OP_Truncate which in turn would fail during execution.

Also, fit code into 80 chars (this <if> is 83 chars long).

> +			sqlite3VdbeAddOp1(v, OP_Clear, space_id);
> +		else
> +			sqlite3VdbeAddOp1(v, OP_Truncate, space_id);

Furthermore, I see no reason to introduce separate opcode.
When I suggested to do so, I thought it wouldn’t mess with OP_Clear.
In your approach it is enough to pass flag to OP_Clear.

> diff --git a/test/sql/delete.test.lua b/test/sql/delete.test.lua
> index 1721989..7132302 100644
> --- a/test/sql/delete.test.lua
> +++ b/test/sql/delete.test.lua
> @@ -21,6 +21,12 @@ box.sql.execute("DELETE FROM t1 WHERE a=1;");
> -- Verify
> box.sql.execute("SELECT * FROM t1;");
> 
> +--
> +-- gh-2201: TRUNCATE TABLE operation
> +--
> +box.sql.execute("TRUNCATE TABLE t1;")
> +box.sql.execute("SELECT * FROM t1;”)

I would add wider range of tests checking TRUNCATE work.
At least truncate inside transaction, truncate on system space,
truncate on view, truncate on space with FK constraints, triggers etc.
Moreover, this test doesn’t check that OP_Truncate opcode is really executed.