From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 517EE26EC7 for ; Tue, 17 Jul 2018 17:05:11 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id THz_e-Qpe4Ey for ; Tue, 17 Jul 2018 17:05:11 -0400 (EDT) Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 009BD26EA2 for ; Tue, 17 Jul 2018 17:05:10 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 1/5] sql: prohibit creation of FK on unexisting tables References: <32f36b207de86bf144b16260c9ed434b45cf094c.1531443603.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Wed, 18 Jul 2018 00:05:08 +0300 MIME-Version: 1.0 In-Reply-To: <32f36b207de86bf144b16260c9ed434b45cf094c.1531443603.git.korablev@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, Nikita Pettik Thanks for the patch! See 12 comments below and a separate commit on the branch. On 13/07/2018 05:04, Nikita Pettik wrote: > Originally, SQLite allows to create table with foreign keys contraint 1. contraint -> constraint. And two the same typos below. > which refers to yet not created parent table. For instance: > > CREATE TABLE child(id INT PRIMARY KEY REFERENCES parent); > CREATE TABLE parent(id INT PRIMARY KEY); > > This patch bans such ability since it contradicts SQL ANSI. > Moreover, SQLite allows to drop parent table if deletion of all rows > wouldn't result in FK contraint violations. This feature has been > removed since in such situation child table would become inconsistent. > > Finally, within current patch ability to create FK contraints on VIEWs > is banned as well. > > Part of #3271 > --- > src/box/sql/build.c | 41 ++++++++++++---- > src/box/sql/fkey.c | 95 +++--------------------------------- > src/box/sql/sqliteInt.h | 1 - > src/box/sql/vdbe.c | 30 ------------ > test/sql-tap/alter.test.lua | 6 +-- > test/sql-tap/fkey1.test.lua | 18 +++---- > test/sql-tap/fkey2.test.lua | 25 ++++------ > test/sql-tap/fkey3.test.lua | 4 +- > test/sql-tap/suite.ini | 1 + > test/sql-tap/table.test.lua | 1 + > test/sql-tap/tkt-b1d3a2e531.test.lua | 2 +- > 11 files changed, 67 insertions(+), 157 deletions(-) > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 0072f842e..0c762fac9 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -2256,10 +2256,14 @@ sql_drop_table(struct Parse *parse_context, struct SrcList *table_name_list, > * removing indexes from _index space and eventually > * tuple with corresponding space_id from _space. > */ > - > - sql_clear_stat_spaces(parse_context, space_name, NULL); > struct Table *tab = sqlite3HashFind(&db->pSchema->tblHash, space_name); > - sqlite3FkDropTable(parse_context, table_name_list, tab); > + struct FKey *fk = sqlite3FkReferences(tab); > + if (fk != NULL && strcmp(fk->pFrom->def->name, tab->def->name) != 0) { 2. Is it sufficient to compare space ids in lieu of names? > + sqlite3ErrorMsg(parse_context, "can't drop parent table %s when " > + "child table refers to it", space_name); 3. How about ER_DROP_SPACE? Now we have merely < 140 sqlite3ErrorMsg calls, and if we did not use the latter in new code, we would gradually get rid of it, nErr, redundant errmsg in Parse. > + goto exit_drop_table; > + } > + sql_clear_stat_spaces(parse_context, space_name, NULL); > sql_code_drop_table(parse_context, space, is_view); > > exit_drop_table: > @@ -2301,6 +2305,26 @@ sqlite3CreateForeignKey(Parse * pParse, /* Parsing context */ > char *z; > > assert(pTo != 0); > + char *normilized_name = strndup(pTo->z, pTo->n); 4. normilized -> normalized? 5. Can we involve Parser.region here? > + if (normilized_name == NULL) { > + diag_set(OutOfMemory, pTo->n, "strndup", "normalized name"); > + goto fk_end; > + } > + sqlite3NormalizeName(normilized_name); > + uint32_t parent_id = box_space_id_by_name(normilized_name, > + strlen(normilized_name)); 6. It is possible to use pTo->n here in lieu of strlen? > + if (parent_id == BOX_ID_NIL && > + strcmp(normilized_name, p->def->name) != 0) { > + sqlite3ErrorMsg(pParse, "foreign key constraint references "\ > + "nonexistent table: %s", normilized_name); 7. Lets move ER_CREATE_FK_CONSTRAINT into this patch from the next ones and use it. Also I dream we can move into this patch all the refactoring about FKey -> fkey, fkey_def, fkey_parse, and other non-functional changes, but it is almost impossible, as I understand(( > + goto fk_end; > + } > + struct space *parent_space = space_by_id(parent_id); > + if (parent_space != NULL && parent_space->def->opts.is_view) { > + sqlite3ErrorMsg(pParse, "can't create foreign key constraint "\ > + "referencing view: %s", normilized_name); > + goto fk_end; > + } > if (p == 0) > goto fk_end; > if (pFromCol == 0) { > @@ -2322,8 +2346,8 @@ sqlite3CreateForeignKey(Parse * pParse, /* Parsing context */ > } else { > nCol = pFromCol->nExpr; > } > - nByte = > - sizeof(*pFKey) + (nCol - 1) * sizeof(pFKey->aCol[0]) + pTo->n + 1; > + nByte = sizeof(*pFKey) + (nCol - 1) * sizeof(pFKey->aCol[0]) + > + strlen(normilized_name) + 1; 8. Why strlen()? I thought length of the normalized name is equal to pTo->n, it is not? You had created the normalized name as strndup of pTo->n bytes. Same about the next hunk. > if (pToCol) { > for (i = 0; i < pToCol->nExpr; i++) { > nByte += sqlite3Strlen30(pToCol->a[i].zName) + 1; > @@ -2337,10 +2361,8 @@ sqlite3CreateForeignKey(Parse * pParse, /* Parsing context */ > pFKey->pNextFrom = p->pFKey; > z = (char *)&pFKey->aCol[nCol]; > pFKey->zTo = z; > - memcpy(z, pTo->z, pTo->n); > - z[pTo->n] = 0; > - sqlite3NormalizeName(z); > - z += pTo->n + 1; > + memcpy(z, normilized_name, strlen(normilized_name) + 1); > + z += strlen(normilized_name) + 1; > pFKey->nCol = nCol; > if (pFromCol == 0) { > pFKey->aCol[0].iFrom = p->def->field_count - 1;> diff --git a/test/sql-tap/alter.test.lua b/test/sql-tap/alter.test.lua > index cfe280121..3e5c6102b 100755 > --- a/test/sql-tap/alter.test.lua > +++ b/test/sql-tap/alter.test.lua > @@ -313,9 +313,9 @@ test:do_execsql_test( > DROP TABLE IF EXISTS t1; > DROP TABLE IF EXISTS t2; > DROP TABLE IF EXISTS t3; > - CREATE TABLE t1(a PRIMARY KEY, b, c, FOREIGN KEY(b) REFERENCES t2(id), FOREIGN KEY(c) REFERENCES t3(id)); > - CREATE TABLE t2(id PRIMARY KEY); > - CREATE TABLE t3(id PRIMARY KEY); > + CREATE TABLE t2(id INT PRIMARY KEY); > + CREAte TABLE t3(id INT PRIMARY KEY); > + CREATE TABLE t1(a PRIMARY KEY, b, c, FOREIGN KEY(b) REFERENCES t2(id), FOREIGN KEY(c) REFERENCES t3(id)); 9. Some problems with indentation and capitalization. > INSERT INTO t2 VALUES(1); > INSERT INTO t3 VALUES(2); > INSERT INTO t1 VALUES(1, 1, 2); > diff --git a/test/sql-tap/suite.ini b/test/sql-tap/suite.ini > index 0637cffc1..e9c3d65ed 100644 > --- a/test/sql-tap/suite.ini > +++ b/test/sql-tap/suite.ini > @@ -3,6 +3,7 @@ core = app > description = Database tests with #! using TAP > disabled = > reindex.test.lua ; This test is banned in scope of #2174 > + gh-2953-drop-table-with-FK.test.lua 10. Leading white space. 11. Why did you disable it? If it can not be fixed, then just delete. > lua_libs = lua/sqltester.lua ../sql/lua/sql_tokenizer.lua ../box/lua/identifier.lua > is_parallel = True > release_disabled = debug_mode_only.test.lua > diff --git a/test/sql-tap/table.test.lua b/test/sql-tap/table.test.lua > index 31330a5a0..6aa290742 100755 > --- a/test/sql-tap/table.test.lua > +++ b/test/sql-tap/table.test.lua > @@ -730,6 +730,7 @@ test:do_catchsql_test( > "table-10.2", > [[ > DROP TABLE t6; > + CREATE TABLE t4(a INT PRIMARY KEY); 12. Indentation. Something is wrong. > CREATE TABLE t6(a REFERENCES t4(a) MATCH PARTIAL primary key); > ]], { > --