[tarantool-patches] Re: [PATCH 2/4] sql: rework OP_Clear internals

  • From: "n.pettik" <korablev@xxxxxxxxxxxxx>
  • To: tarantool-patches@xxxxxxxxxxxxx
  • Date: Tue, 20 Mar 2018 15:27:05 +0300


On 20 Mar 2018, at 13:54, Kirill Yukhin <kyukhin@xxxxxxxxxxxxx> wrote:

Hello,

My comments are inlined.

On 19 мар 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              | 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 == 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 “comments for the sake of comments”:
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 = txn_begin_stmt(space);
+    if (txn == NULL)
+            return -1;
+    struct tuple *unused = NULL;
+    if (space_execute_dml(space, txn, request, &unused) != 0) {
+            txn_rollback_stmt();
+            return -1;
+    }
+    if (txn_commit_stmt(txn, request) != 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 “integrated” system
of privilege checks and separate (from box) metrics:

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


@@ -670,30 +689,31 @@ int tarantoolSqlite3ClearTable(int iTable)
                     }
             }
-            box_iterator_free(iter);
-    } else if (box_truncate(space_id) != 0) {
+            iterator_delete(iter);
+    } else if (box_truncate(space->def->id) != 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.

Other related posts:

  1. Home
  2. tarantool-patches
  3. Archive
  4. 03-2018