[tarantool-patches] Re: [PATCH v4 1/4] schema: add new system space for CHECK constraints

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

On 19.05.2019 19:01, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patchset!
> 
> Great! See 17 comments below!
> 
> 1. Nikita in the email 13.05 02:04 asked you to create
> an issue "sql: add handle to disable constraints". Where
> is it? - I can't find.
https://github.com/tarantool/tarantool/issues/4244

>> _ck_constraint (space id = 357)
>> [<constraint name> STR, <space id> UINT,
>>  <is_deferred>BOOL, <code>STR, <language>STR]
> 
> 2. As I understand, now we want to allow stored functions in Lua, C,
> and in future in SQL. But how checks are different? Why there is no
> an option like 'language' or something? Lua and C checks are going to
> be much faster than SQL ones. Also I see, that Kostja asked you to add
> a language at 14.05 23:09 email, but you just ignored it. Why?
In fact, you've reviewed an outdated branch. As you see, there was
such code in the letter and moreover it is in may new letter for you.

> 
> 3. Please, change the space format to [space_id, ck_name, ...] and the
> primary index parts to [space_id, ck_name]. Otherwise you can't
> lookup all checks of one space having space_id. On the other hand lookup
> by name without a space id is useless. This is why _index system space
> has such a format and a pk.
Done.

>> are easier to manage as self-sustained objects: such change is
> 
> 4. Double 'are'.
Done.

>> + * To perform it transactionally, we create a list of a new ck
> 
> 5. 'a' article is never used with plural.
It is not plural here. A list ...

> 
> 6. I do not see a language on the branch. Either the mail, or the
> branch, are outdated.
See 2.

>> +	}  else if (stmt->new_tuple != NULL && stmt->old_tuple == NULL) {
> 
> 7. Double whitespace prior to 'else'.
Fixed.

>> +		/*
>> +		 * FIXME: Ck constraint creation on non-empty
>> +		 * space must be implemented as preparatory
>> +		 * step for ALTER SPACE ADD CONSTRAINT feature.
> 
> 8. We already have ADD CONSTRAINT. It works for FK, UNIQUE, PK. The
> problem is that we can't call it on a non-empty space.
https://github.com/tarantool/tarantool/issues/4243

>> +const char *ck_constraint_language_strs[] = {"SQL"};
> 
> 9. I do not see that on the branch.
See 2.

>> + * Resolve space_def references for check constraint via AST
>> + * tree traversal.
>> + * @param ck_constraint Check constraint object to update.
> 
> 10. There is no such parameter.
Fixed.

>> + * Find check constraint object in space by given name and
>> + * name_len.
>> + * @param space The space to lookup check constraint.
>> + * @param name The check constraint name.
>> + * @param name_len The length of the name.
>> + * @retval not NULL Check constrain if exists, NULL otherwise.
> 
> 11. constrain -> constraint.
fixed.

> 12. Is there any reason why you do not use field access by '.'?
> You could use
> 
>     local _space = box.space._space
>     local _index = box.space._index
>     local _ck_constraint = box.space._ck_constraint
>     ...
>     if flags.checks then
>     ...
>     flags.checks = nil
> 
> instead of
> 
>     local _space = box.space[box.schema.SPACE_ID]
>     local _index = box.space[box.schema.INDEX_ID]
>     local _ck_constraint = box.space[box.schema.CK_CONSTRAINT_ID]
>     ...
>     if flags['checks'] ~= nil then
>     ...
>     flags['checks'] = nil
> 
> IMO the former looks simpler.
No reason. I did it looking on some legacy code. Done.


>> +            _space:replace(box.tuple.new({space.id, space.owner, space.name,
>> +                                          space.engine, space.field_count,
>> +                                          flags, space.format}))
> 
> 13. Why do you create tuple to replace? Anyway it is not inserted as
> is, and :replace() creates another tuple internally.
Fixed,

>> -	/** SQL Checks expressions list. */
>> -	struct ExprList *checks;
> 
> 14. Now you can drop 'struct ExprList;' announcement
> from this header.
Ok.

>> +static void
>> +trim_space_snprintf(char *wptr, const char *str, uint32_t str_len)
> 
> 15. You can't just drop all the whitespaces, new lines, and tabs from
> an SQL expression, and expect its behaviour unchanged.
> 
> 	tarantool> box.execute("CREATE TABLE test (id INT PRIMARY KEY, a TEXT, CONSTRAINT CK1 CHECK(a != '   '))")
> 	---
> 	- row_count: 1
> 	...
> 
> 	tarantool> box.execute("INSERT INTO test VALUES (1, '   ')")
> 	---
> 	- row_count: 1
> 	...
> 
> 	tarantool> box.space._ck_constraint:select{}
> 	---
> 	- - ['CK1', 512, false, 'a != '' ''']
> 	...
> 
> I've forbidden to insert '   ', but you trimmed the string, and
> changed the expression logic. Please, either remove
> trim_space_snprintf(), or make it smarter and do not touch
> space symbols inside quotes. I vote for the second option. As Nikita
> fairly noticed, otherwise CHECK bodies are unreadable.

You are right. Fixed. New "smart" routine is on branch.

>>  	struct alter_entity_def *alter_def =
>>  		(struct alter_entity_def *) &parser->create_ck_def;
> 
> 16. Here you can cast 'create_ck_def' without taking it from
> &parser->create_ck_def again.
Ok.

>> +	uint32_t expr_str_len = (uint32_t)(create_ck_def->expr->zEnd -
>> +					   create_ck_def->expr->zStart);
> 
> 17. create_ck_def->expr is expr_span, which you obtained above.
> 
>> +	const char *expr_str = create_ck_def->expr->zStart;
Ok.