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

[n.pettik]

Also I’ve rebased patch-set on fresh 2.0.

>>>> +	/* 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

Yep, I meant exactly this usage.

>> 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. ???

Oops, sorry, ‘haste makes waste’...

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

Forgot about that. So simply moved these tests to sql/erring.test.lua