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 accesschecks? I think, this won't work. The only concern I see here is thatprocess_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 thisDDL 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.