[tarantool-patches] Re: [PATCH v3 0/3] box: run checks on insertions in LUA spaces

[Kirill Shcherbatov]

Hello everybody,

The discussion has grown in many threads. I've gathered together
everything that we have agreed and what has been done.
Looks like we have a conception with this patch series with
@korablev now.
I'll resend whole patch a bit later.
(careful: now it is based on kshch/gh-3691-sql-flags-in-parser branch)

1. @korablev
   [tarantool-patches] Re: [PATCH v3 2/3] box: run check constraint tests
                       on space alter 14.05.2019, 19:49:
> > One unpleasant “feature” that I’ve noticed:
> >
> > create table t4 (id int primary key check(id > id*id))
> > insert into t4 values(0.5)
> > - error: 'Failed to execute SQL statement: Check constraint failed ''CK_CONSTRAINT_1_T4'':
> >    id > id*id’
>
> Please, document this “feature”.

At first, we've discussed this problem with @kyukhin, @kostya, @pgulutzan again
	[dev] Implicit casting from FP -> INT 15.05.2019, 11:44
	Re: [dev] cast 15.05.2019, 21:44
	#4216
and decided that it's OK and we should live with it.

I've included the following description in the last commit:
    Note: this patch changes the CK constraints execution order for
    SQL. Previously check of CK constraints integrity was fired before
    tuple is formed; meanwhile now they are implemented as NoSQL before
    replace triggers, which are fired right before tuple insertion.
    In turn, type casts are performed earlier than msgpack
    serialization. You should be careful with functions that use
    field types in your check constrains (like typeof()).

    Consider following situation:
    ```
     box.execute("CREATE TABLE t2(id  INT primary key,
                                  x INTEGER CHECK (x > 1));")
     box.execute("INSERT INTO t2 VALUES(3, 1.1)")
    ```
    the last operation would fail because 1.1 is silently
    casted to integer 1 which is not greater than 1.


2. @kostya
   [tarantool-patches] Re: [PATCH v3 1/3] schema: add new system space
                       for CHECK constraints 14.05.2019, 21:29

> Please make sure at least the space definition contains all the
> fields necessary for information_schema, even if this patch
> doesn't fill in some of the fields yet.
>
> Does it? What's in information_schema.check_constraints? What do
> other vendors store in this system space (like deferrable,
> initially deferred).

https://docs.microsoft.com/en-us/sql/relational-databases/system-information-schema-views/check-constraints-transact-sql?view=sql-server-2017

https://dev.mysql.com/doc/refman/8.0/en/check-constraints-table.html

https://mariadb.com/kb/en/library/information-schema-check_constraints-table/

https://www.postgresql.org/docs/9.1/infoschema-check-constraints.html

Have really similar structure:
| constraint_catalog - Name of the database containing the constraint (always the current database)
| constraint_schema - Name of the schema containing the constraint
| constraint_name	- Name of the constraint
| check_clause - The check expression of the check constraint

Looks like current format of _ck_constraint comprises all of these fields.

3. @kostya
   [tarantool-patches] Re: [PATCH v3 3/3] box: user-friendly interface
                       to manage ck constraints 14.05.2019, 23:09

> Imagine you have stored Lua functions. Don't you want to be able
> to define Lua check constraints from Lua?
>
> Which begs a question, should _ck_constraint space contain
> 'language' field just in case you'd want to add this inthe future?
>
> Looks a bit weird that you use Lua to define an SQL constraint,
> doesn't it?
>
> But overall this is pretty cool already.

Ok, then we are going to extend space format to make it
hold language of expression.

4. @kostya
   [tarantool-patches] Re: [PATCH v3 2/3] box: run check constraint
                       tests on space alter 07.05.2019, 19:39

> > +	/* Compile VDBE with default sql parameters. */
> > +	struct session *user_session = current_session();
> > +	uint32_t sql_flags = user_session->sql_flags;
> > +	user_session->sql_flags = default_flags;
>
> This is a time bomb from technical debt point of view. Please pass
> the flags  to sql_parser_create instead, which will then pass them
> to vdbe.

I agree with your position, so that I implemented this change in
a separate commit series. Now ck constraint(s) branch is based on it.


5. @kostya
   [tarantool-patches] Re: [PATCH v3 2/3] box: run check constraint
                       tests on space alter 07.05.2019, 19:39
> This looks like a pessimization to me. Depending on the code flow,
> some of the tuple fields may not be accessed at all. Is it really
> necessary to decode them so agressibvely here? Especially since
> you encode *all* space fields.

   @kshcherbatov
   [tarantool-patches] Re: [tarantool-patches] Re: [PATCH v3 2/3]
                       box: run check constraint tests on space
                       alter 07.05.2019, 20:47
> I'll try to walk though the AST tree and prepare the map of fields that
> are involved in expression (on check compile operation); Here there
> would be binding of **used** fields.
> What do you think?

   @korablev
   [tarantool-patches] Re: [PATCH v3 0/3] box: run checks on
                       insertions in LUA spaces 14.05.2019, 21:22

> Add to ck_constraint array of used field numbers - that’s done
> during ck_constraint_program_compile() while we have struct
> Expr by traversing AST. Then, we emit OP_Variable ck_field_count
> times, where ck_field_count is length of array of used field
> numbers.

I've solved this problem using smart column mask for AST-required
fields. It is a separate patch now. It's good enough, I think.

Regards,
Kirill Shcherbatov