[tarantool-patches] Re: [PATCH 1/5] sql: prohibit creation of FK on unexisting tables
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed Jul 18 00:05:08 MSK 2018
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);
> ]], {
> -- <table-10.2>
More information about the Tarantool-patches
mailing list