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 C4DF422C2B for ; Wed, 25 Jul 2018 06:03:57 -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 QgQs5lw2ES-V for ; Wed, 25 Jul 2018 06:03:57 -0400 (EDT) Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [94.100.177.103]) (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 E6C1222A9E for ; Wed, 25 Jul 2018 06:03:56 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH 1/5] sql: prohibit creation of FK on unexisting tables From: "n.pettik" In-Reply-To: Date: Wed, 25 Jul 2018 13:03:45 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <0176472E-E0CD-41F9-B794-45E02FA54238@tarantool.org> References: <32f36b207de86bf144b16260c9ed434b45cf094c.1531443603.git.korablev@tarantool.org> 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 Cc: Vladislav Shpilevoy >> Originally, SQLite allows to create table with foreign keys contraint >=20 > 1. contraint -> constraint. And two the same typos below. Fixed. >> --- 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 =3D sqlite3HashFind(&db->pSchema->tblHash, = space_name); >> - sqlite3FkDropTable(parse_context, table_name_list, tab); >> + struct FKey *fk =3D sqlite3FkReferences(tab); >> + if (fk !=3D NULL && strcmp(fk->pFrom->def->name, tab->def->name) = !=3D 0) { >=20 > 2. Is it sufficient to compare space ids in lieu of names? Seems to be enough: +++ b/src/box/sql/build.c @@ -2258,9 +2258,11 @@ sql_drop_table(struct Parse *parse_context, = struct SrcList *table_name_list, */ struct Table *tab =3D sqlite3HashFind(&db->pSchema->tblHash, = space_name); struct FKey *fk =3D sqlite3FkReferences(tab); - if (fk !=3D NULL && strcmp(fk->pFrom->def->name, tab->def->name) = !=3D 0) { - sqlite3ErrorMsg(parse_context, "can't drop parent table = %s when " - "child table refers to it", space_name); + if (fk !=3D NULL && (fk->pFrom->def->id !=3D tab->def->id)) { + diag_set(ClientError, ER_DROP_SPACE, space_name, + "other objects depend on it"); + parse_context->rc =3D SQL_TARANTOOL_ERROR; + parse_context->nErr++; >=20 >> + sqlite3ErrorMsg(parse_context, "can't drop parent table = %s when " >> + "child table refers to it", space_name); >=20 > 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. Ok, see diff above. >=20 >> + 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 !=3D 0); >> + char *normilized_name =3D strndup(pTo->z, pTo->n); >=20 > 4. normilized -> normalized? Surely, it is a typo. >=20 > 5. Can we involve Parser.region here? It doesn=E2=80=99t really matter: in further patches I reworked this = function significantly and region is used there. >=20 >> + if (normilized_name =3D=3D NULL) { >> + diag_set(OutOfMemory, pTo->n, "strndup", "normalized = name"); >> + goto fk_end; >> + } >> + sqlite3NormalizeName(normilized_name); >> + uint32_t parent_id =3D box_space_id_by_name(normilized_name, >> + = strlen(normilized_name)); >=20 > 6. It is possible to use pTo->n here in lieu of strlen? After =E2=80=99normalisation=E2=80=99 name can become shorter: =E2=80=9Ctab=E2=80=9D -> tab So, I guess strlen is right choice. >=20 >> + if (parent_id =3D=3D BOX_ID_NIL && >> + strcmp(normilized_name, p->def->name) !=3D 0) { >> + sqlite3ErrorMsg(pParse, "foreign key constraint = references "\ >> + "nonexistent table: %s", = normilized_name); >=20 > 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(( Particularly this moment doesn=E2=80=99t require = ER_CREATE_FK_CONSTRAINT: + diag_set(ClientError, ER_DROP_SPACE, space_name, + "other objects depend on it"); + parse_context->rc =3D SQL_TARANTOOL_ERROR; + parse_context->nErr++; + goto exit_drop_table; It seems to be quite complicated to use in current patch = ER_CREATE_FK_CONSTRAINT since it uses constraint name and before next patch FK constraints never = feature one. >> + goto fk_end; >> + } >> + struct space *parent_space =3D space_by_id(parent_id); >> + if (parent_space !=3D 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 =3D=3D 0) >> goto fk_end; >> if (pFromCol =3D=3D 0) { >> @@ -2322,8 +2346,8 @@ sqlite3CreateForeignKey(Parse * pParse, = /* Parsing context */ >> } else { >> nCol =3D pFromCol->nExpr; >> } >> - nByte =3D >> - sizeof(*pFKey) + (nCol - 1) * sizeof(pFKey->aCol[0]) + = pTo->n + 1; >> + nByte =3D sizeof(*pFKey) + (nCol - 1) * sizeof(pFKey->aCol[0]) + >> + strlen(normilized_name) + 1; >=20 > 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. Due to the same reason I replied above. >=20 >> if (pToCol) { >> for (i =3D 0; i < pToCol->nExpr; i++) { >> nByte +=3D sqlite3Strlen30(pToCol->a[i].zName) + = 1; >> @@ -2337,10 +2361,8 @@ sqlite3CreateForeignKey(Parse * pParse, = /* Parsing context */ >> pFKey->pNext=46rom =3D p->pFKey; >> z =3D (char *)&pFKey->aCol[nCol]; >> pFKey->zTo =3D z; >> - memcpy(z, pTo->z, pTo->n); >> - z[pTo->n] =3D 0; >> - sqlite3NormalizeName(z); >> - z +=3D pTo->n + 1; >> + memcpy(z, normilized_name, strlen(normilized_name) + 1); >> + z +=3D strlen(normilized_name) + 1; >> pFKey->nCol =3D nCol; >> if (pFromCol =3D=3D 0) { >> pFKey->aCol[0].i=46rom =3D 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)); >=20 > 9. Some problems with indentation and capitalization. +++ b/test/sql-tap/alter.test.lua @@ -314,8 +314,8 @@ test:do_execsql_test( DROP TABLE IF EXISTS t2; DROP TABLE IF EXISTS t3; 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)); + 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)); >=20 >> 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 =3D app >> description =3D Database tests with #! using TAP >> disabled =3D >> reindex.test.lua ; This test is banned in scope of #2174 >> + gh-2953-drop-table-with-FK.test.lua >=20 > 10. Leading white space. >=20 > 11. Why did you disable it? If it can not be fixed, then just > delete. Ok, just removed whole file. diff --git a/test/sql-tap/gh-2953-drop-table-with-FK.test.lua = b/test/sql-tap/gh-2953-drop-table-with-FK.test.lua deleted file mode 100755 >=20 >> lua_libs =3D lua/sqltester.lua ../sql/lua/sql_tokenizer.lua = ../box/lua/identifier.lua >> is_parallel =3D True >> release_disabled =3D 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); >=20 > 12. Indentation. Something is wrong. +++ b/test/sql-tap/table.test.lua @@ -730,7 +730,7 @@ test:do_catchsql_test( "table-10.2", [[ DROP TABLE t6; - CREATE TABLE t4(a INT PRIMARY KEY); + CREATE TABLE t4(a INT PRIMARY KEY);