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

[Kirill Yukhin <kyukhin@tarantool.org>]

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?

> +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.

> @@ -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.

--
Kirill