[tarantool-patches] Re: [PATCH v7 7/7] sql: remove Checks to server
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Mon May 28 21:50:08 MSK 2018
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);
>
More information about the Tarantool-patches
mailing list