[tarantool-patches] Re: [PATCH v2 5/9] schema: add new system space for CHECK constraints

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

Hi! Thank you for review.
I didn't quote all your comments because the code is differ now and many of
them are outdated or trivial(that are fixed).
But I've answered all questions below.

>>>> +	++schema_version;
>>>
>>> 7. We have UpdateSchemaVersion for that. It can be touched
>>> direcrtly only by _space triggers.
>> I don't know how make it for now. I don't use alter object in on_dd_...replace_trigger
>> for _ck_constraint space; maybe it worth to introduce it. Do it later with review
>> on whole patch.
>>
> 
> See no fixes to this comment.

I've discussed it with Vlad. We don't need to update epoch here at all.

> 
> I’ve found way to make Tarantool hang:
> 
> box.sql.execute("create table t(id int primary key check (id > 5))")
> drop_ck = function() box.space._ck_constraint:delete({'CK_CONSTRAINT_1_T', box.space.T.id}) end
> fiber = require('fiber')
> box.error.injection.set("ERRINJ_WAL_DELAY", true)
> f2 = fiber.create(drop_ck)
> box.sql.execute("INSERT INTO t VALUES(1)")
> box.error.injection.set("ERRINJ_WAL_DELAY", false)
> 
> Please, find the reason of that and add several tests like this:
> insert/update + create/drop CK + delayed wal injection.

I've created a bug reproduced without CK constraints. But it is a feature =)
https://github.com/tarantool/tarantool/issues/4100#issuecomment-479846703

>> After insertion into this space, a new instance describing check
>> constraint is created. Check constraint hold Expr tree.
>> While space features check constraints, it isn't allowed to
>> be dropped. The :drop() space method firstly deletes all check
>> constraints and then removes entry from _space.
> 
> It would be nice to see motivation for this change.

Believe my new commit message a little better:

    schema: add new system space for CHECK constraints
    
    This patch introduces a new system space to persist check
    constraints. Format of the space:
    
    _ck_constraint (space id = 357)
    [<constraint name> STR, <space id> UINT, <expression string>STR]
    
    CK constraint is local to space, so every pair <CK name, space id>
    is unique (and it is PK in _ck_constraint space).
    
    After insertion into this space, a new instance describing check
    constraint is created. Check constraint holds Expr AST tree.
    While space features check constraints, it isn't allowed to
    be dropped. The :drop() space method firstly deletes all check
    constraints and then removes entry from _space.
    
    Because space alter, index alter and space truncate operations
    case space recreation, introduced RebuildCkConstrains object
    that compile new ck constraint objects, replace and remove
    existent instances atomically(when some compilation fails,
    nothing changed). In fact, in scope of this patch we don't
    really need to recreate ck_constraint object in such situations
    (patch space_def pointer in AST like we did it before is enough
    now, but we would recompile VDBE that represents ck constraint
    in future that can fail).
    
    The main motivation for these changes is the ability to support
    ADD CHECK CONSTRAINT operation in the future.
    
    Needed for #3691

>> 	alter_space_move_indexes(alter, iid + 1, old_space->index_id_max + 1);
>> +	(void) new BuildCkConstraints(alter);
> 
> Why do we need to rebuild check constraints on index alter?
>
>> +	(void) new BuildCkConstraints(alter);
> 
> Looks like a nonsense: why do we need to rebuild ck constraints on truncate?
Consider my new comment:

/**
 * As ck_constraint object depends on space_def we must rebuild
 * all ck constraints on space alter.
 *
 * To perform it transactionally, we create a list of a new ck
 * constraints objects in ::prepare method that is fault-tolerant.
 * Finally in ::alter or ::rollback methods we only swap thouse
 * lists securely.
 */
class RebuildCkConstraints: public AlterSpaceOp

We must rebuild CK constraints when space definition is recreated.
Factually, we must use RebuildCkConstraints on each alter_space_do()
call: on_replace_dd_truncate, on_replace_dd_index, on_replace_dd_space.
But I don't see any way to make it a logical part of alter_space_do.

> Rename it to ck_constraint_def_new_from_tuple() to make name be
> consistent with the rest of similar functions:
> - space_def_new_from_tuple
> - index_def_new_from_tuple
> - func_def_new_from_tuple
> ...
> etc
Ok.
>> +		if (ck_constraint == NULL)
>> +			return;
> 
> But in on_replace_dd_ck_constraint() you have next check:
> 
> assert(old_ck_constraint != NULL);
> …
> on_rollback->data = old_ck_constraint;
> 
> How ck_constraint could be NULL?
You are right.

>> +	access_check_ddl(space->def->name, space->def->id, space->def->uid,
>> +			 SC_SPACE, PRIV_A);
>> +
> 
> See comments to previous patch concerning privileges.
I've drop this code. We don't need to check privileges. Everything with _fk_constraint
and _trigger spaces is ok.


> Why do you need to place implementation and definition of CK in
> the same memory chunk? Yep, it simplifies deallocation a bit, but
> instead you have to copy def and call def_sizeof() 4 times. I suggest
> to allocate def using malloc right during tuple parsing.
I've reworked ck constraints structures. Now ck_constraint_def is
simultaneous allocation.

> You can make link be first member of struct and place
> name  at the end of struct (like in space_def or fk_def).
But I have two strings here. Don't see any reason to distinguish them.
Moreover, now this structure is slightly different.

> For constraints with auto-generated names it would be helpful
> to see failed condition IMHO. Otherwise, using only error message like
> 
> "Check constraint failed: CK_CONSTRAINT_1_T1”
> 
> it’s quite complicated to understand which constraint failed.
> So, I suggest to add to this error string of failed condition.
Done.


> I suppose this should be part of upgrade to 2_2_0 script.
I've implemented it as a part of upgrade_to_2_1_3 script.
Feature  #4007 that has 2.2.0 milestone is already there.

>> +		int all_ok = sqlVdbeMakeLabel(v);
>> +		sqlExprIfTrue(parse_context, expr, all_ok, SQL_JUMPIFNULL);
>> +		if (on_conflict == ON_CONFLICT_ACTION_IGNORE) {
>> +			sqlVdbeGoto(v, ignore_label);
>> 			sqlVdbeResolveLabel(v, all_ok);
> 
> Are you sure you need to resolve label here?
> I see in code below one more same label resolution.
You are right. It is redundant.
> +	/*
>> +	 * Dissallow the transfer optimization if the destination
>> +	 * table contains any check constraints.
>> +	 */
>> +	if (!rlist_empty(&dest->ck_constraint))
>> 		return 0;
> 
> Why did you change condition of optimisation?
It was easier to implement.. =)
Ok; now we have equivalent code. And tests.

>> +	parser->parsed_ast.expr = sqlExprDup(parser->db, expr_list->a->pExpr, 0);
> 
> Is this change related to patch-set?
Described in comment.

>> +			static const char * const azType[] =
>> +				{"NOT NULL", "UNIQUE", "Check", "FOREIGN KEY" };
> 
> Do we really need this change?
Now it is useless