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 566CE203B9 for ; Wed, 18 Jul 2018 12:40:20 -0400 (EDT) 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 xEwlLJ7gp8De for ; Wed, 18 Jul 2018 12:40:20 -0400 (EDT) Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [94.100.177.103]) (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 A4368202DC for ; Wed, 18 Jul 2018 12:40:19 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: introduce TRUNCATE TABLE operation From: "n.pettik" In-Reply-To: <606f657b-26df-35b9-ddf1-d5bae5654d82@tarantool.org> Date: Wed, 18 Jul 2018 19:40:14 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <70326bb69fbe25215df79e2d5e01043f93ff7c5a.1531902074.git.kshcherbatov@tarantool.org> <797d52e5-1387-dd3d-1fb2-0fafbbbdcf56@tarantool.org> <606f657b-26df-35b9-ddf1-d5bae5654d82@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: Kirill Shcherbatov > On 18 Jul 2018, at 16:01, Kirill Shcherbatov = wrote: >=20 >> 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=E2=80=99t 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/290a5c9a08d469be9954df9e7d6d= 33f5743826c9 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=E2=80=99t fire ON DELETE triggers (but fires ON TRUNCATE ones). In = your implementation it might fire, if TRUNCATE launched inside transaction.=20 Either, it can=E2=80=99t 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=E2=80=99t 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); > } >=20 > /** > 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, >=20 > 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 =E2=80=98is_=E2=80=99 or =E2=80=98if_=E2=80=99= prefixes. > { > struct sqlite3 *db =3D parse->db; > if (parse->nErr || db->mallocFailed) > @@ -185,9 +185,10 @@ sql_table_delete_from(struct Parse *parse, struct = SrcList *tab_list, > */ > if (where =3D=3D NULL && !is_complex) { > assert(!is_view); > - > - sqlite3VdbeAddOp1(v, OP_Clear, space_id); > - > + if (!allow_truncate || space_is_system(space) || = in_txn() !=3D 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 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=E2=80=99t 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=3D1;"); > -- Verify > box.sql.execute("SELECT * FROM t1;"); >=20 > +-- > +-- gh-2201: TRUNCATE TABLE operation > +-- > +box.sql.execute("TRUNCATE TABLE t1;") > +box.sql.execute("SELECT * FROM t1;=E2=80=9D) 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=E2=80=99t check that OP_Truncate opcode is = really executed.