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

[Vladislav Shpilevoy]

Thanks for the fixes! See my 3 comments below.

And I have pushed several review fix commits. Look at them
and squash.

> 
> 
>> box.cfg{}
>> opts = {checks = {{exp = 'X<5'}}}
>> format = {{name = 'X', type = 'unsigned'}}
>> t = {513, 1, 'test', 'memtx', 0, opts, format}
>> box.space._space:insert(t)

1. I do not see a test on it.
> 
>> 10. deleteTable leaks with the same reason: you delete space_def only when
>> opts.is_temporary flag is set. Including checks, that are on malloc.
>> 11. Why do you space->def->opts.checks here? I scanned sql_view_column_names,
>> and looks like it does not require compiled check. It needs only check names
>> and check span. Table.def has both, it is not?
>>> +	/*
>>> +	 * As rebuild creates a new ExpList tree and old_def
>>> +	 * is allocated on region release old tree manually.
>>> +	 */
>>> +	struct ExprList *old_checks = p->def->opts.checks;
>>>    	if (sql_table_def_rebuild(db, p) != 0)
>>>    		return;
> 
>>> +	/* Get server checks. */
>>> +	ExprList *checks = space_checks_expr_list(table->def->id);
>>
> 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:

box.sql.execute("CREATE TABLE t1(x INTEGER CHECK( x<5 ), y REAL CHECK( y>x ), z primary key, invalid:parameter);")

Assertion failed: (pTable->def->opts.checks == NULL), function deleteTable, file /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/build.c, line 414.
Process 24676 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
     frame #0: 0x00007fff5eed8b6e libsystem_kernel.dylib`__pthread_kill + 10
libsystem_kernel.dylib`__pthread_kill:
->  0x7fff5eed8b6e <+10>: jae    0x7fff5eed8b78            ; <+20>
     0x7fff5eed8b70 <+12>: movq   %rax, %rdi
     0x7fff5eed8b73 <+15>: jmp    0x7fff5eecfb00            ; cerror_nocancel
     0x7fff5eed8b78 <+20>: retq

> I've wrote comments about this in  sqlite3EndTable:
> 
> /*
> * As rebuild creates a new ExpList tree and old_def
> * allocated on region release old tree manually.
> */
> /*
> * Checks are useless for now as all operations process with
>   * the server checks instance. Remove to do not consume extra memory,
>   * don't require make a copy on space_def_dup and to improve
>   * debuggability.
>   */
>>
>> 12. Please, write tests on checks insertion from Lua. I know such checks will
>> not work, but at least you should do sanity test. You should test, that a user
>> can insert a check with no name, that a check can be compiled even if no SQL
>> Table, that a check expr can not be number or bool and the same about name.
>> In the same tests you can check errors on non-existing column names.

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.