[tarantool-patches] Re: [PATCH 1/5] sql: prohibit creation of FK on unexisting tables

  • From: Vladislav Shpilevoy <v.shpilevoy@xxxxxxxxxxxxx>
  • To: tarantool-patches@xxxxxxxxxxxxx, Nikita Pettik <korablev@xxxxxxxxxxxxx>
  • Date: Wed, 18 Jul 2018 00:05:08 +0300

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>

Other related posts:

  1. Home
  2. tarantool-patches
  3. Archive
  4. 07-2018