[tarantool-patches] Re: [PATCH 3/5] sql: introduce ADD CONSTRAINT statement

[Vladislav Shpilevoy]

Thanks for the patch! The whole patchset LGTM except two
comments below about the tests.

On 06/08/2018 03:28, n.pettik wrote:
> I squashed your fixes. However, personally I don’t understand why you
> decided to refactor endTable() routine, for instance. Ofc refactoring
> is always appreciated but I would prefer separating functional part of patch and
> cosmetic/codestyle changes. Initial version of this patch AFAIR included 1k +- lines.
> Now (due to codestyle fixes) it is about 1.8k + and 1.6k -.. It would be quite
> complicated to find smth in such giant diff.

But new code amount was reduced actually. Many of the new code
(added during the review) is the tests on the bugs found during
the review.

> 
>>> diff --git a/src/box/alter.cc b/src/box/alter.cc
>>> index 5b55bfd7a..6b9e29470 100644
>>> --- a/src/box/alter.cc
>>> +++ b/src/box/alter.cc
>>> @@ -3660,6 +3660,50 @@ fkey_grab_by_name(struct rlist *list, const char *fkey_name)
>>>   	return NULL;
>>>   }
>>>   +static void
>>
>> 1. No comment.
> 
> Comment for static inline two-lines function? Seriously?
> Ok, I will add it, but I think we should consider making an exception
> in our developing guide for instance for static functions less than
> 10 lines or sort of.

You can propose it in a chat.

>>> +	/* Delete all child FK constraints. */
>>> +	struct fkey *child_fk;
>>> +	rlist_foreach_entry (child_fk, &space->child_fkey, child_link) {
>>> +		const char *fk_name_dup = sqlite3DbStrDup(v->db,
>>> +							  child_fk->def->name);
>>> +		if (fk_name_dup == NULL)
>>> +			return;
>>> +		vdbe_emit_fkey_drop(parse_context, fk_name_dup, space_id);
>>
>> 5. Can we use P4_STATIC and do not duplicate?
> 
> No, unfortunately we can’t. P4_STATIC attribute is usable only when the same
> memory is used twice during VDBE execution. STATIC attribute prevents from
> memory releasing (even at the end of execution AFAIK). If we released memory
> right after generating opcode (or at the end of parsing, it doesn’t matter), we will
> probably get corrupted memory.

How can child_fk->def->name be freed before VDBE execution? It is drop table
code generation, so here the child_fk exists out of the parser context in a
space from the space cache.

> 
>> I want to say, that if we
>> do not duplicate the memory here, then only 2 situations are possible:
>> 1) Until the VDBE is executed nothing is happened, the memory is ok and
>> can be used.
> 
> Can be used, but wouldn’t be freed (or I misunderstand what you initially meant).

Yes, it will not be freed because it will not be duplicated. Free of the
original name together with the child_fk will occur inside alter.cc. Vdbe
after drop of the space will not touch this freed memory.

But do not mind. I see, that vdbe_emit_fkey_drop is used both to malloced
normalized names from the parser and for struct fkey names. So it is not
worth to refactor this function to use both static and dynamic memory. We
need a more comprehensive way to compact these numerous mallocs during
parsing, later.

> diff --git a/test/sql-tap/fkey2.test.lua b/test/sql-tap/fkey2.test.lua
> index 062597e9b..7cb315fcc 100755
> --- a/test/sql-tap/fkey2.test.lua
> +++ b/test/sql-tap/fkey2.test.lua
> @@ -755,10 +753,13 @@ test:do_catchsql_test(
>          DROP TABLE IF EXISTS p;
>          CREATE TABLE p(a, b, PRIMARY KEY(a, b));
>          CREATE TABLE c(x PRIMARY KEY REFERENCES p);
> +<<<<<<< HEAD
>          INSERT INTO c DEFAULT VALUES;
> +=======
> +>>>>>>> 323e96ee2... sql: introduce ADD CONSTRAINT statement

1. ???

>      ]], {
>          -- <fkey2-7.4>
> -        1, "foreign key mismatch - \"C\" referencing \"P\""
> +        1, "Failed to create foreign key constraint 'FK_CONSTRAINT_1_C': number of columns in foreign key does not match the number of columns in the primary index of referenced table"
>          -- </fkey2-7.4>
>      })
>  
> diff --git a/test/sql/foreign-keys.result b/test/sql/foreign-keys.result
> index c2ec429c3..2580221d3 100644
> --- a/test/sql/foreign-keys.result
> +++ b/test/sql/foreign-keys.result
> @@ -332,5 +332,109 @@ box.space.CHILD:drop()
>  box.space.PARENT:drop()
>  ---
>  ...
> --- Clean-up SQL DD hash.
> -test_run:cmd('restart server default with cleanup=1')
> +-- Check that parser correctly handles MATCH, ON DELETE and
> +-- ON UPDATE clauses.
> +--
> +box.sql.execute('CREATE TABLE tp (id INT PRIMARY KEY, a INT UNIQUE)')
> +---
> +...
> +box.sql.execute('CREATE TABLE tc (id INT PRIMARY KEY, a INT REFERENCES tp(a) ON DELETE SET NULL MATCH FULL)')
> +---
> +...
> +box.sql.execute('ALTER TABLE tc ADD CONSTRAINT fk1 FOREIGN KEY (id) REFERENCES tp(id) MATCH PARTIAL ON DELETE CASCADE ON UPDATE SET NULL')
> +---
> +...
> +box.space._fk_constraint:select{}
> +---
> +- - ['FK1', 518, 517, false, 'partial', 'cascade', 'set_null', [0], [0]]
> +  - ['FK_CONSTRAINT_1_TC', 518, 517, false, 'full', 'set_null', 'no_action', [1],
> +    [1]]
> +...
> +box.sql.execute('DROP TABLE tc')
> +---
> +...
> +box.sql.execute('DROP TABLE tp')
> +---
> +...
> +-- Tests which are aimed at verifying work of commit/rollback
> +-- triggers on _fk_constraint space.
> +--
> +box.sql.execute("CREATE TABLE t3 (id PRIMARY KEY, a REFERENCES t3, b INT UNIQUE);")
> +---
> +...
> +t = box.space._fk_constraint:select{}[1]:totable()
> +---
> +...
> +errinj = box.error.injection

2. Please, do not use errinj in regular tests. Only
in release-disabled. Errinj structures and code are
defined to nothing in C when NDEBUG is set, so they
do not work.