[tarantool-patches] Re: [PATCH v7 7/7] sql: remove Checks to server

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the fixes! I force pushed new ones on the branch. Now the
patchset LGTM.

Nikita, please, review it.

On 28/05/2018 17:59, Kirill Shcherbatov wrote:
>> 1. I do not see a test on it
>> 3. One test is not enough. You did not test my example with 'exp' above, you did not test 'expr'
>> type mismatch, unexpected options in a check (expr + name + some unexpected one) etc.
> --- a/test/sql/checks.test.lua
> +++ b/test/sql/checks.test.lua
> @@ -18,4 +18,27 @@ t = {513, 1, 'test', 'memtx', 0, opts, format}
>   s = box.space._space:insert(t)
>   box.space._space:delete(513)
>   
> +opts = {checks = {{expr = 'X>5', name = 'ONE'}}}
> +format = {{name = 'X', type = 'unsigned'}}
> +t = {513, 1, 'test', 'memtx', 0, opts, format}
> +s = box.space._space:insert(t)
> +box.space._space:delete(513)
> +
> +-- extra invlalid field name
> +opts = {checks = {{expr = 'X>5', name = 'ONE', extra = 'TWO'}}}
> +format = {{name = 'X', type = 'unsigned'}}
> +t = {513, 1, 'test', 'memtx', 0, opts, format}
> +s = box.space._space:insert(t)
> +
> +opts = {checks = {{expr_invalid_label = 'X>5'}}}
> +format = {{name = 'X', type = 'unsigned'}}
> +t = {513, 1, 'test', 'memtx', 0, opts, format}
> +s = box.space._space:insert(t)
> +
> +-- invalid field type
> +opts = {checks = {{name = 123}}}
> +format = {{name = 'X', type = 'unsigned'}}
> +t = {513, 1, 'test', 'memtx', 0, opts, format}
> +s = box.space._space:insert(t)
> 
> 
>>> We don't replicate checks in SQL. At the end of sqlite3EndTable checks are released and the only checks collection present on the server side.
>> 2. Are you sure? I added assert(def->checks == NULL) in deleteTable() and it fails:
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index bd99f20..8acd01e 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -408,10 +408,11 @@ deleteTable(sqlite3 * db, Table * pTable)
>          sqlite3SelectDelete(db, pTable->pSelect);
>          assert(pTable->def != NULL);
>          /* Do not delete pTable->def allocated on region. */
> -       if (!pTable->def->opts.temporary)
> +       if (!pTable->def->opts.temporary) {
>                  space_def_delete(pTable->def);
> -       else
> -               assert(pTable->def->opts.checks == NULL);
> +       } else {
> +               sql_expr_list_delete(db, pTable->def->opts.checks);
> +       }
>          sqlite3DbFree(db, pTable);
>