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

[n.pettik]

>> Nit: ck_constraint_def_sizeof() return uint32_t
>> 
>> Also why do we not use vdbe_emit_halt_with_presence_test()
>> during creation of check constraints?
>> 
>> tarantool> create table t2(id int primary key, constraint ck1 check(id > 0), constraint ck1 check(id < 0))
>> ---
>> - error: Duplicate key exists in unique index 'primary' in space '_ck_constraint'
>> ...
>> tarantool> create table t2(id int primary key, constraint fk1 foreign key(id) references t2, constraint fk1 foreign key(id) references t2)
>> ---
>> - error: Constraint FK1 already exists
>> ...
>> 
> 
> And this is a bug in FK
> https://github.com/tarantool/tarantool/issues/4183

Still, we have an option to fix it: display proper error message and
process clean-up. Please, add a mention that in scope of issue we
should also use vdbe_emit_halt_with_presence_test for CK constraints

>>> +	if (rlist_next(dst_ck_link) != &dest->ck_constraint)
>>> +		return 0;
>> 
>> After next patch this check could be removed: VDBE program for checking
>> consistency of check constraints would be generated automatically.
>> In SQLite xFer allows to avoid generation of this program. On the other hand,
>> if you added way to temporarily disable check constraints, we would able
>> to leave this check and re-enable them after query is executed. So, consider
>> way of disabling/enabling check constraints - it might be useful for other
>> cases as well.
> 
> At first, there is no way to control this state as you propose.

Ok, then we should come up with machinery which will allow us
to disable CK constraints, but not other NoSQL triggers.

> Next, disabling/enabling triggers is required for upgrade() functionality
> and is not a part of public API.

From user’s point of view checks are constraints. AFAIR Konstantin
asked for a handle to disable check and foreign key constraints.

> As in further patches ck constraint are rely
> on trigger machinery, we don't need a separate controller.

We shouldn’t disable all NoSQL triggers, only check constraints.

> So, I've reject xfer optimization when source or destination space has
> ck constraints.

This optimisation doesn’t seem to be vital. On the other hand, if we can
use it, why not to do so?

> 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]

Nit: you forgot about is_deferred field.

> Because space alter, index alter and space truncate operations
> cause space recreation, introduced RebuildCkConstrains object
> that compile

-> compiles

> new ck constraint objects, replace and remove

-> replaces and removes

> existent instances atomically(when some compilation fails,
> nothing changed).

-> is 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

-> are going to

> The main motivation for these changes is the ability to support
> ADD CHECK CONSTRAINT operation in the future. CK constraints are
> are easier to manage as self-sustained objects: we mustn’t

What to do?

> the tuple describing target space to do it(unlike the current
> architecture).

Can’t parse this sentence. Re-phrase it please.

> Disabled xfer optimization when some space have ck constraints
> because in the following patches this xfer optimisation becomes
> impossible. No reason to rewrite this code.
> 
> Needed for #3691
> —

Please, don’t send the whole patch again, I expect only diff
between versions. It takes a while to review such huge patch again.