Tarantool development patches archive
 help / color / mirror / Atom feed
From: "n.pettik" <korablev@tarantool.org>
To: tarantool-patches@freelists.org
Cc: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH 3/5] sql: introduce ADD CONSTRAINT statement
Date: Tue, 7 Aug 2018 02:43:38 +0300	[thread overview]
Message-ID: <1EF951A7-A7E7-4EC3-BA78-DB3A5E66A461@tarantool.org> (raw)
In-Reply-To: <17680404-70f3-93bb-1f7c-024ff45a3530@tarantool.org>

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

  reply	other threads:[~2018-08-06 23:43 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-13  2:04 [tarantool-patches] [PATCH 0/5] Move FK constraints to server Nikita Pettik
2018-07-13  2:04 ` [tarantool-patches] [PATCH 1/5] sql: prohibit creation of FK on unexisting tables Nikita Pettik
2018-07-17 21:05   ` [tarantool-patches] " Vladislav Shpilevoy
2018-07-25 10:03     ` n.pettik
2018-07-26 20:12       ` Vladislav Shpilevoy
2018-08-01 20:54         ` n.pettik
2018-08-02 22:15           ` Vladislav Shpilevoy
2018-08-06  0:27             ` n.pettik
2018-07-13  2:04 ` [tarantool-patches] [PATCH 2/5] schema: add new system space for FK constraints Nikita Pettik
2018-07-17 21:05   ` [tarantool-patches] " Vladislav Shpilevoy
2018-07-25 10:03     ` n.pettik
2018-07-26 20:12       ` Vladislav Shpilevoy
2018-08-01 20:54         ` n.pettik
2018-08-02 22:15           ` Vladislav Shpilevoy
2018-08-06  0:28             ` n.pettik
2018-08-06 18:24               ` Vladislav Shpilevoy
2018-07-13  2:04 ` [tarantool-patches] [PATCH 3/5] sql: introduce ADD CONSTRAINT statement Nikita Pettik
2018-07-17 21:05   ` [tarantool-patches] " Vladislav Shpilevoy
2018-07-25 10:03     ` n.pettik
2018-07-26 20:12       ` Vladislav Shpilevoy
2018-08-01 20:54         ` n.pettik
2018-08-02 22:15           ` Vladislav Shpilevoy
2018-08-06  0:28             ` n.pettik
2018-08-06 18:24               ` Vladislav Shpilevoy
2018-08-06 23:43                 ` n.pettik [this message]
2018-07-13  2:04 ` [tarantool-patches] [PATCH 4/5] sql: display error on FK creation and drop failure Nikita Pettik
2018-07-17 21:04   ` [tarantool-patches] " Vladislav Shpilevoy
2018-07-25 10:03     ` n.pettik
2018-07-26 20:11       ` Vladislav Shpilevoy
2018-07-13  2:04 ` [tarantool-patches] [PATCH 5/5] sql: remove SQLITE_OMIT_FOREIGN_KEY define guard Nikita Pettik
2018-07-17 21:04 ` [tarantool-patches] Re: [PATCH 0/5] Move FK constraints to server Vladislav Shpilevoy
2018-08-07 14:57 ` Kirill Yukhin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1EF951A7-A7E7-4EC3-BA78-DB3A5E66A461@tarantool.org \
    --to=korablev@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='[tarantool-patches] Re: [PATCH 3/5] sql: introduce ADD CONSTRAINT statement' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox