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

  • From: "n.pettik" <korablev@xxxxxxxxxxxxx>
  • To: tarantool-patches@xxxxxxxxxxxxx
  • Date: Wed, 25 Jul 2018 13:03:45 +0300


Originally, SQLite allows to create table with foreign keys contraint

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 = 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?

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 = sqlite3HashFind(&db->pSchema->tblHash, space_name);
        struct FKey *fk = sqlite3FkReferences(tab);
-       if (fk != NULL && strcmp(fk->pFrom->def->name, tab->def->name) != 0) {
-               sqlite3ErrorMsg(parse_context, "can't drop parent table %s when 
"
-                               "child table refers to it", space_name);
+       if (fk != NULL && (fk->pFrom->def->id != tab->def->id)) {
+               diag_set(ClientError, ER_DROP_SPACE, space_name,
+                               "other objects depend on it");
+               parse_context->rc = SQL_TARANTOOL_ERROR;
+               parse_context->nErr++;


+            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.

Ok, see diff above.


+            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?

Surely, it is a typo.


5. Can we involve Parser.region here?

It doesn’t really matter: in further patches I reworked this function 
significantly
and region is used there.


+    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?

After ’normalisation’ name can become shorter:
“tab” -> tab
So, I guess strlen is right choice.


+    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((

Particularly this moment doesn’t require ER_CREATE_FK_CONSTRAINT:

+               diag_set(ClientError, ER_DROP_SPACE, space_name,
+                               "other objects depend on it");
+               parse_context->rc = 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 = 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.

Due to the same reason I replied above.


     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.

+++ 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));


         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.

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


 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.

+++ 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);


Other related posts: