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 E03792B0E8 for ; Tue, 20 Mar 2018 08:27:10 -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 Cc1X1hgH4pX7 for ; Tue, 20 Mar 2018 08:27:10 -0400 (EDT) Received: from smtp37.i.mail.ru (smtp37.i.mail.ru [94.100.177.97]) (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 0FEB62B088 for ; Tue, 20 Mar 2018 08:27:09 -0400 (EDT) From: "n.pettik" Message-Id: <1712F845-FB6E-4E19-9294-160A036F69B5@tarantool.org> Content-Type: multipart/alternative; boundary="Apple-Mail=_FA05C6B6-BABA-4AE6-811C-9F1A5A0AF6C4" Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH 2/4] sql: rework OP_Clear internals Date: Tue, 20 Mar 2018 15:27:05 +0300 In-Reply-To: <20180320105452.2oln3bfvc4uhkuhg@tarantool.org> References: <48d971ed5d65bb116a6b6719cbf5f13b6328b136.1521479592.git.korablev@tarantool.org> <20180320105452.2oln3bfvc4uhkuhg@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: =?utf-8?B?0JrQuNGA0LjQu9C7INCu0YXQuNC9?= --Apple-Mail=_FA05C6B6-BABA-4AE6-811C-9F1A5A0AF6C4 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On 20 Mar 2018, at 13:54, Kirill Yukhin wrote: >=20 > Hello, >=20 > My comments are inlined. >=20 > On 19 =D0=BC=D0=B0=D1=80 21:10, Nikita Pettik wrote: >> This patch makes OP_Clear use pointer to space instead of space id. >> Moreover, in order to avoid excess function calls (such as = box_delete() -> >> box_process1(), which in its turn makes additional space lookup), >> sql_execute_dml() function is introduced. It is an analogue of >> process_rw() from BOX internals. Its purpose is to handle transaction >> routine and call DML executor. This function will be also used later = as >> well during insertion and deletion. >>=20 >> Part of #3122 >> --- >> src/box/sql.c | 54 = +++++++++++++++++++++++++++++++--------------- >> src/box/sql/opcodes.c | 2 +- >> src/box/sql/opcodes.h | 2 +- >> src/box/sql/tarantoolInt.h | 2 +- >> src/box/sql/vdbe.c | 28 ++++++++++-------------- >> 5 files changed, 51 insertions(+), 37 deletions(-) >>=20 >> diff --git a/src/box/sql.c b/src/box/sql.c >> index 256985793..4f8b39810 100644 >> --- a/src/box/sql.c >> +++ b/src/box/sql.c >> @@ -191,6 +191,27 @@ is_tarantool_error(int rc) >> rc =3D=3D SQL_TARANTOOL_INSERT_FAIL); >> } >>=20 >> +/** >> + * This function is analogue of process_rw() from box module. >> + * Apart of calling space_execute_dml(), it handles transaction >> + * routine. >> + */ > Could you pls describe params as well? As you wish, but it would look like =E2=80=9Ccomments for the sake of = comments=E2=80=9D: function is 10 lines long and I stick to the point that the purpose of=20= arguments is obvious. =20 >=20 >> +static int >> +sql_execute_dml(struct request *request, struct space *space) >> +{ >> + struct txn *txn =3D txn_begin_stmt(space); >> + if (txn =3D=3D NULL) >> + return -1; >> + struct tuple *unused =3D NULL; >> + if (space_execute_dml(space, txn, request, &unused) !=3D 0) { >> + txn_rollback_stmt(); >> + return -1; >> + } >> + if (txn_commit_stmt(txn, request) !=3D 0) >> + return -1; >> + return 0; >> +} > So, you've copied process_rw, removing statitics udpate and access > checks? I think, this won't work. The only concern I see here is that > process_rw returns a tuple, which is useless for SQL so far. Ok, I have returned these two functions (as it happens in process_rw). But I thought that in SQL we would have more =E2=80=9Cintegrated=E2=80=9D = system of privilege checks and separate (from box) metrics: + rmean_collect(rmean_box, request->type, 1); + if (access_check_space(space, PRIV_W) !=3D 0) + return -1; >=20 >> @@ -670,30 +689,31 @@ int tarantoolSqlite3ClearTable(int iTable) >> } >> } >> - box_iterator_free(iter); >> - } else if (box_truncate(space_id) !=3D 0) { >> + iterator_delete(iter); >> + } else if (box_truncate(space->def->id) !=3D 0) { > I think we should block box_truncate sp far: too much complications = this > DDL operation introduce. We'll re-implement it as optimization in = furture. > Also, we'll support ANSI SQL's TRUNCATE TABLE. >=20 Fixed on branch. Also, updated comments for OP_Clear. --Apple-Mail=_FA05C6B6-BABA-4AE6-811C-9F1A5A0AF6C4 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8
On 20 Mar 2018, at 13:54, Kirill Yukhin <kyukhin@tarantool.org> wrote:

Hello,
My comments are inlined.
On 19 =D0=BC=D0=B0=D1=80 21:10, Nikita Pettik = wrote:
This patch makes OP_Clear use pointer to space instead of = space id.
Moreover, in order to avoid excess function = calls (such as box_delete() ->
box_process1(), which in = its turn makes additional space lookup),
sql_execute_dml() = function is introduced. It is an analogue of
process_rw() = from BOX internals. Its purpose is to handle transaction
routine and call DML executor. This function will be also = used later as
well during insertion and deletion.

Part of #3122
---
src/box/sql.c =             &n= bsp;| 54 +++++++++++++++++++++++++++++++---------------
src/box/sql/opcodes.c      |  2 = +-
src/box/sql/opcodes.h      | =  2 +-
src/box/sql/tarantoolInt.h |  2 +-
src/box/sql/vdbe.c =         | 28 = ++++++++++--------------
5 files changed, 51 = insertions(+), 37 deletions(-)

diff --git = a/src/box/sql.c b/src/box/sql.c
index 256985793..4f8b39810 = 100644
--- a/src/box/sql.c
+++ = b/src/box/sql.c
@@ -191,6 +191,27 @@ = is_tarantool_error(int rc)
rc =3D=3D = SQL_TARANTOOL_INSERT_FAIL);
}

+/**
+ * This function is analogue of = process_rw() from box module.
+ * Apart of calling = space_execute_dml(), it handles transaction
+ * = routine.
+ */
Could you pls describe params as = well?

As you wish, but it would look like =E2=80=9Ccomment= s for the sake of comments=E2=80=9D:
function is 10 lines long = and I stick to the point that the purpose of 
arguments = is obvious.  


+static int
+sql_execute_dml(struct request *request, struct space = *space)
+{
+ struct txn *txn =3D = txn_begin_stmt(space);
+ if (txn =3D=3D NULL)
+ = = return -1;
+ struct tuple *unused =3D NULL;
+ = if (space_execute_dml(space, txn, request, &unused) !=3D 0) = {
+ txn_rollback_stmt();
+ return = -1;
+ }
+ if (txn_commit_stmt(txn, request) = !=3D 0)
+ return -1;
+ return = 0;
+}
So, you've copied process_rw, removing = statitics udpate and access
checks? I think, this won't = work. The only concern I see here is that
process_rw returns a tuple, = which is useless for SQL so far.

Ok, I have = returned these two functions (as it happens in = process_rw).
But I thought that in SQL we would have more = =E2=80=9Cintegrated=E2=80=9D system
of privilege checks = and separate (from box) metrics:

+ rmean_collect(rmean_box, = request->type, 1);
+ if (access_check_space(space, = PRIV_W) !=3D 0)
+ return -1;


@@ -670,30 +689,31 @@ int = tarantoolSqlite3ClearTable(int iTable)
}
= = }
- box_iterator_free(iter);
- = } else if (box_truncate(space_id) !=3D 0) {
+ = iterator_delete(iter);
+ } else if = (box_truncate(space->def->id) !=3D 0) {
I think we should block box_truncate sp = far: too much complications this
DDL operation introduce. We'll = re-implement it as optimization in furture.
Also, we'll support ANSI SQL's TRUNCATE = TABLE.


Fixed on = branch. Also, updated comments for OP_Clear.

= --Apple-Mail=_FA05C6B6-BABA-4AE6-811C-9F1A5A0AF6C4--