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

  • From: "n.pettik" <korablev@xxxxxxxxxxxxx>
  • To: tarantool-patches@xxxxxxxxxxxxx
  • Date: Sun, 12 May 2019 16:45:30 +0300


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.


Other related posts:

  1. Home
  2. tarantool-patches
  3. Archive
  4. 05-2019