[tarantool-patches] Re: [PATCH v1 1/1] sql: make sql checks on server side

[n.pettik]

Firstly, sql-tap/selectG.test.lua fails on your branch.

> To make SQL CHECKS on space insertion in LUA we have to
> generate a temporal VDBE and execute it with a tuple
> mapped in VDBE memory with new opcode OP_FieldMapMemory as
> an argument. These actions are performed by a new special trigger
> set for the space on server side.
> The CHECKS are carried out twice for SQL requests because of
> context and affinity changes to which the inserting tuple is
> subjected.
> At first, user could specify IGNORE keyword for SQL UPDATE that
> is a part of PARSER CONTEXT and couldn't be handled on
> server-side for now.

If you mean ‘pragma ignore_checks’, then we are going to
remove this option at all. So this shouldn’t be a problem.
https://github.com/tarantool/tarantool/issues/3696

> The other point is that tuple represented as msgpack on
> server-side may have already changed field data type.
> 
> E.g.
> CREATE TABLE t2(id primary key,
>                z TEXT CONSTRAINT three
>                  CHECK(typeof(coalesce(z,'')) == 'text'));
> INSERT INTO t2 VALUES(1, 3.14159);
> 
> Before the tuple for insertion into the server-side space was
> generated, it's z field was of type REAL, which did not pass
> the CHECK test. But then new compatible affinity BLOB is applied,
> so making check on server-side no problems would be triggered
> and tuple would be inserted successfully.

This will be fixed when static types hit the trunk. Currently, patch is under
review. Hence, this shouldn’t be issue as well.

To be honest, I think that triggers, foreign keys and checks should
work in the same way. So you need kind of interface (or framework) to run them.

Personally mine point of view is following:
we should compile check constraints once, and reuse the same VDBE program -
it would significantly speed up execution time. The same for triggers and foreign keys.

Eventually, it should look like (IMHO):

	vdbe_run(get_compiled_triggers(space));
	vdbe_run(get_compiled_checks(space));
	vdbe_run(get_compiled_fk(space));

Check can’t be changed now in any way (at least until we introduce
ALTER TABLE ADD/DROP CONSTRAINT CHECK), so this program
can’t change. The same for FK: until constraint exists, it can’t be changed, ergo
VDBE program implementing FK checks can’t be modified as well.
As for triggers - now we can refer to inexistent spaces in trigger’s body,
but I guess we may ban this opportunity without any regrets.

To sump up, I suggest you to completely remove code generation for check
constraints from SQL parser and move it to tnt trigger. Moreover, investigate
how we can store compiled program and then run it instead of parsing
each time the same expression.

Your approach works when it comes for checks - the easiest part of integrity
constraints. But I have doubts that it would work the same way for triggers
and foreign keys.

One more general nit - comment your code, please.

> Closes #3691.
> ---
> Branch: https://github.com/tarantool/tarantool/tree/kshch/gh-3691-checks-on-server-side
> Issue: https://github.com/tarantool/tarantool/issues/3691
> 
> src/box/alter.cc         | 37 +++++++++++++++++++++
> src/box/sql.c            | 67 +++++++++++++++++++++++++++++++++++++
> src/box/sql.h            | 15 +++++++++
> src/box/sql/build.c      |  2 +-
> src/box/sql/insert.c     | 86 +++++++++++++++++++++++++++---------------------
> src/box/sql/sqliteInt.h  | 27 ++++++++++++---
> src/box/sql/vdbe.c       | 20 ++++++++++-
> test/sql/checks.result   | 47 ++++++++++++++++++++++++++
> test/sql/checks.test.lua | 17 ++++++++++
> 9 files changed, 275 insertions(+), 43 deletions(-)
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 1757332..4684fc6 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -39,6 +39,7 @@
> #include "coll_id_def.h"
> #include "txn.h"
> #include "tuple.h"
> +#include "trigger.h"
> #include "fiber.h" /* for gc_pool */
> #include "scoped_guard.h"
> #include "third_party/base64.h"
> @@ -1425,6 +1426,27 @@ on_drop_space_rollback(struct trigger *trigger, void *event)
> }
> 
> /**
> + * SQL-specific actions for space.
> + */

Too poor comment. it doesn’t even look like comment, sorry.
I didn’t review the rest of code since it may change significantly
after reworking patch.

> +static void
> +on_replace_dd_space_sql(struct trigger *trigger, void *event)
> +{