[tarantool-patches] Re: [PATCH v2 9/9] sql: run check constraint tests on space alter

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

> Mention that format is required condition to use check
> constraints: otherwise, name of field can’t be resolved.
> Now it leads to assertion fault:
> 
> s = box.schema.create_space('test’)
> s:create_index('pk’)
> box.space._ck_constraint:insert({'physics', s.id, 'X<Y’})
> Assertion failed: (space_def->field_count > 0), function lookupName, file tarantool/src/box/sql/resolve.c, line 242.
Thank you. Fixed it in previous patch.

> 
>> box.space._ck_constraint:insert({'physics', s.id, 'X<Y'})
>> box.space.test:insert({6, 5})
>> - error: 'Check constraint failed: physics’
> 
> To finish this patch-set I suggest to add Lua-wrapper to create
> check constraints on any space using NoSQL interface and introduce
> ALTER TABLE ADD CONSTRAINT CHECK().
> Last issue you can implement in a separate patch or delegate its
> implementation to smb else.
Consider my new commit.

>> +	vdbe_emit_ck_constraint(&parser, expr, ck_constraint->def->name,
>> +				new_tuple_reg);
>> +	sql_finish_coding(&parser);
> 
> Do we need to call this function at all?
Yes. I've tried to drop it.

> 
>> +	if (parser.is_aborted) {
>> +		diag_set(ClientError, ER_CREATE_CK_CONSTRAINT,
>> +			 ck_constraint->def->name,
>> +			 "can not compile expression”);
> 
> This error will re-set original parsing error. I suggest to
> concatenate them.
Done.


>> +	sql_reset(ck_constraint->stmt);
> 
> This function returns error code, don’t ignore it.
Now I use it's result to determine execution state at the end of trigger.

>> +	parser->ckBase = new_tuple_reg;
>> +	struct Vdbe *v = sqlGetVdbe(parser);
>> +	const char *ck_constraint_name = sqlDbStrDup(parser->db, name);
> 
> Where's this pointer released?
> 
>> +	VdbeNoopComment((v, "BEGIN: ck constraint %s test", name));
>> +	/* Skip check when it is turned off. */
Now this string is a part of VdbeNoopComment so VDBE must release it.
> When it can be turned off?
Outdated comment.

> Please, add descent set of tests verifying that check constraints
> work in any possible scenario.
> Make sure that check occurs before replace action.

Many new good tests are the part of previous patch now.