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

["n.pettik" <korablev@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 3092 bytes --]


> On 20 Mar 2018, at 13:54, Kirill Yukhin <kyukhin@tarantool.org> 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.


[-- Attachment #2: Type: text/html, Size: 15087 bytes --]