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

[Nikita Pettik <korablev@tarantool.org>]

Originally, SQLite allows to create table with foreign keys contraint
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) {
+		sqlite3ErrorMsg(parse_context, "can't drop parent table %s when "
+				"child table refers to it", space_name);
+		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);
+	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));
+	if (parent_id == BOX_ID_NIL &&
+	    strcmp(normilized_name, p->def->name) != 0) {
+		sqlite3ErrorMsg(pParse, "foreign key constraint references "\
+				"nonexistent table: %s", normilized_name);
+		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;
 	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;
@@ -2394,6 +2416,7 @@ sqlite3CreateForeignKey(Parse * pParse,	/* Parsing context */
 
  fk_end:
 	sqlite3DbFree(db, pFKey);
+	free(normilized_name);
 #endif				/* !defined(SQLITE_OMIT_FOREIGN_KEY) */
 	sql_expr_list_delete(db, pFromCol);
 	sql_expr_list_delete(db, pToCol);
diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
index 6c75c4772..be080324f 100644
--- a/src/box/sql/fkey.c
+++ b/src/box/sql/fkey.c
@@ -330,13 +330,10 @@ sqlite3FkLocateIndex(Parse * pParse,	/* Parse context to store any error in */
 			}
 		}
 	}
-
 	if (!pIdx) {
-		if (!pParse->disableTriggers) {
-			sqlite3ErrorMsg(pParse,
-					"foreign key mismatch - \"%w\" referencing \"%w\"",
-					pFKey->pFrom->def->name, pFKey->zTo);
-		}
+		sqlite3ErrorMsg(pParse, "foreign key mismatch - "\
+					"\"%w\" referencing \"%w\"",
+				pFKey->pFrom->def->name, pFKey->zTo);
 		sqlite3DbFree(pParse->db, aiCol);
 		return 1;
 	}
@@ -754,46 +751,6 @@ sql_fk_trigger_delete(struct sqlite3 *db, struct sql_trigger *trigger)
 	sqlite3DbFree(db, trigger);
 }
 
-/**
- * This function is called to generate code that runs when table
- * pTab is being dropped from the database. The SrcList passed as
- * the second argument to this function contains a single entry
- * guaranteed to resolve to table pTab.
- *
- * Normally, no code is required. However, if the table is
- * parent table of a FK constraint, then the equivalent
- * of "DELETE FROM <tbl>" is executed in a single transaction
- * before dropping the table from the database. If any FK
- * violations occur, rollback transaction and halt VDBE. Triggers
- * are disabled while running this DELETE, but foreign key
- * actions are not.
- */
-void
-sqlite3FkDropTable(Parse *parser, SrcList *name, Table *table)
-{
-	struct session *user_session = current_session();
-	if ((user_session->sql_flags & SQLITE_ForeignKeys) == 0 ||
-	    table->def->opts.is_view || sqlite3FkReferences(table) == NULL)
-		return;
-	struct Vdbe *v = sqlite3GetVdbe(parser);
-	assert(v != NULL);
-	parser->disableTriggers = 1;
-	/* Staring new transaction before DELETE FROM <tbl> */
-	sqlite3VdbeAddOp0(v, OP_TTransaction);
-	sql_table_delete_from(parser, sqlite3SrcListDup(parser->db, name, 0),
-			      NULL);
-	parser->disableTriggers = 0;
-	/*
-	 * If the DELETE has generated immediate foreign key
-	 * constraint violations, rollback, halt the VDBE and
-	 * return an error at this point, before any modifications
-	 * of the _space and _index spaces. This is because these
-	 * spaces don't support multistatement transactions.
-	 * Otherwise, just commit changes.
-	 */
-	sqlite3VdbeAddOp0(v, OP_FkCheckCommit);
-}
-
 /*
  * The second argument points to an FKey object representing a foreign key
  * for which pTab is the child table. An UPDATE statement against pTab
@@ -902,7 +859,6 @@ sqlite3FkCheck(Parse * pParse,	/* Parse context */
 {
 	sqlite3 *db = pParse->db;	/* Database handle */
 	FKey *pFKey;		/* Used to iterate through FKs */
-	int isIgnoreErrors = pParse->disableTriggers;
 	struct session *user_session = current_session();
 
 	/* Exactly one of regOld and regNew should be non-zero. */
@@ -935,42 +891,10 @@ sqlite3FkCheck(Parse * pParse,	/* Parse context */
 		 * schema items cannot be located, set an error in pParse and return
 		 * early.
 		 */
-		if (pParse->disableTriggers) {
-			pTo = sqlite3HashFind(&db->pSchema->tblHash,
-					      pFKey->zTo);
-		} else {
-			pTo = sqlite3LocateTable(pParse, 0, pFKey->zTo);
-		}
-		if (!pTo
-		    || sqlite3FkLocateIndex(pParse, pTo, pFKey, &pIdx,
-					    &aiFree)) {
-			assert(isIgnoreErrors == 0
-			       || (regOld != 0 && regNew == 0));
-			if (!isIgnoreErrors || db->mallocFailed)
+		pTo = sqlite3LocateTable(pParse, 0, pFKey->zTo);
+		if (!pTo || sqlite3FkLocateIndex(pParse, pTo, pFKey, &pIdx,
+					    &aiFree))
 				return;
-			if (pTo == 0) {
-				/* If isIgnoreErrors is true, then a table is being dropped. In this
-				 * case SQLite runs a "DELETE FROM xxx" on the table being dropped
-				 * before actually dropping it in order to check FK constraints.
-				 * If the parent table of an FK constraint on the current table is
-				 * missing, behave as if it is empty. i.e. decrement the relevant
-				 * FK counter for each row of the current table with non-NULL keys.
-				 */
-				Vdbe *v = sqlite3GetVdbe(pParse);
-				int iJump =
-				    sqlite3VdbeCurrentAddr(v) + pFKey->nCol + 1;
-				for (i = 0; i < pFKey->nCol; i++) {
-					int iReg =
-					    pFKey->aCol[i].iFrom + regOld + 1;
-					sqlite3VdbeAddOp2(v, OP_IsNull, iReg,
-							  iJump);
-					VdbeCoverage(v);
-				}
-				sqlite3VdbeAddOp2(v, OP_FkCounter,
-						  pFKey->isDeferred, -1);
-			}
-			continue;
-		}
 		assert(pFKey->nCol == 1 || (aiFree && pIdx));
 
 		if (aiFree) {
@@ -1036,11 +960,8 @@ sqlite3FkCheck(Parse * pParse,	/* Parse context */
 			continue;
 		}
 
-		if (sqlite3FkLocateIndex(pParse, pTab, pFKey, &pIdx, &aiCol)) {
-			if (!isIgnoreErrors || db->mallocFailed)
-				return;
-			continue;
-		}
+		if (sqlite3FkLocateIndex(pParse, pTab, pFKey, &pIdx, &aiCol))
+			return;
 		assert(aiCol || pFKey->nCol == 1);
 
 		/* Create a SrcList structure containing the child table.  We need the
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 8b75ae888..5c5369aeb 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2914,7 +2914,6 @@ struct Parse {
 	u32 newmask;		/* Mask of new.* columns referenced */
 	u8 eTriggerOp;		/* TK_UPDATE, TK_INSERT or TK_DELETE */
 	u8 eOrconf;		/* Default ON CONFLICT policy for trigger steps */
-	u8 disableTriggers;	/* True to disable triggers */
 	/** Region to make SQL temp allocations. */
 	struct region region;
 
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 7a4d37602..2c6bd2ba8 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -2959,36 +2959,6 @@ case OP_Savepoint: {
 	break;
 }
 
-/* Opcode: FkCheckCommit * * * * *
- *
- * This opcode is used and required by DROP TABLE statement,
- * since deleted rows should be rollbacked in case of foreign keys
- * constraint violations. In case of rollback, instruction
- * also causes the VM to halt, because it makes no sense to continue
- * execution with FK violations. If there is no FK violations, then
- * just commit changes - deleted rows.
- *
- * Do not use this instruction in any statement implementation
- * except for DROP TABLE!
- */
-case OP_FkCheckCommit: {
-	if (!box_txn()) {
-		sqlite3VdbeError(p, "cannot commit or rollback - " \
-			"no transaction is active");
-		rc = SQLITE_ERROR;
-		goto abort_due_to_error;
-	}
-	if ((rc = sqlite3VdbeCheckFk(p, 0) != SQLITE_OK)) {
-		box_txn_rollback();
-		sqlite3VdbeHalt(p);
-		goto vdbe_return;
-	} else {
-		rc = box_txn_commit() == 0 ? SQLITE_OK : SQL_TARANTOOL_ERROR;
-		if (rc) goto abort_due_to_error;
-	}
-	break;
-}
-
 /* Opcode: CheckViewReferences P1 * * * *
  * Synopsis: r[P1] = space id
  *
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));
         INSERT INTO t2 VALUES(1);
         INSERT INTO t3 VALUES(2);
         INSERT INTO t1 VALUES(1, 1, 2);
diff --git a/test/sql-tap/fkey1.test.lua b/test/sql-tap/fkey1.test.lua
index bca82d93d..494af4b4a 100755
--- a/test/sql-tap/fkey1.test.lua
+++ b/test/sql-tap/fkey1.test.lua
@@ -6,6 +6,15 @@ test:plan(19)
 
 test:do_execsql_test(
     "fkey1-1.1",
+    [[
+        CREATE TABLE t2(x PRIMARY KEY, y TEXT);
+    ]], {
+        -- <fkey1-1.1>
+        -- </fkey1-1.1>
+    })
+
+test:do_execsql_test(
+    "fkey1-1.2",
     [[
         CREATE TABLE t1(
             a INTEGER PRIMARY KEY,
@@ -19,15 +28,6 @@ test:do_execsql_test(
         -- </fkey1-1.1>
     })
 
-test:do_execsql_test(
-    "fkey1-1.2",
-    [[
-        CREATE TABLE t2(x PRIMARY KEY, y TEXT);
-    ]], {
-        -- <fkey1-1.2>
-        -- </fkey1-1.2>
-    })
-
 test:do_execsql_test(
     "fkey1-1.3",
     [[
diff --git a/test/sql-tap/fkey2.test.lua b/test/sql-tap/fkey2.test.lua
index 9d04a04b0..89a9279da 100755
--- a/test/sql-tap/fkey2.test.lua
+++ b/test/sql-tap/fkey2.test.lua
@@ -15,9 +15,6 @@ test:do_execsql_test(
 
         CREATE TABLE t7(a, b INTEGER PRIMARY KEY);
         CREATE TABLE t8(c PRIMARY KEY REFERENCES t7, d);
-
-        CREATE TABLE t9(a PRIMARY KEY REFERENCES nosuchtable, b);
-        CREATE TABLE t10(a PRIMARY KEY REFERENCES t9(c), b);
     ]], {
         -- <fkey2-1.1>
         -- </fkey2-1.1>
@@ -301,21 +298,19 @@ test:do_catchsql_test(
 test:do_catchsql_test(
     "fkey2-1.29",
     [[
-        INSERT INTO t9 VALUES(1, 3);
+        CREATE TABLE t9(a PRIMARY KEY REFERENCES nosuchtable, b);
     ]], {
-        -- <fkey2-1.29>
-        1, "no such table: NOSUCHTABLE"
-        -- </fkey2-1.29>
+        1, "foreign key constraint references nonexistent table: NOSUCHTABLE"
     })
 
 test:do_catchsql_test(
     "fkey2-1.30",
     [[
-        INSERT INTO t10 VALUES(1, 3);
+        INSERT INTO t9 VALUES(1, 3);
     ]], {
-        -- <fkey2-1.30>
-        1, "foreign key mismatch - \"T10\" referencing \"T9\""
-        -- </fkey2-1.30>
+        -- <fkey2-1.29>
+        1, "no such table: T9"
+        -- </fkey2-1.29>
     })
 
 test:do_execsql_test(
@@ -731,8 +726,8 @@ test:do_catchsql_test(
     [[
         DROP TABLE IF EXISTS c;
         DROP TABLE IF EXISTS p;
-        CREATE TABLE c(x PRIMARY KEY REFERENCES v(y));
         CREATE VIEW v AS SELECT x AS y FROM c;
+        CREATE TABLE c(x PRIMARY KEY REFERENCES v(y));
         INSERT INTO c DEFAULT VALUES;
     ]], {
         -- <fkey2-7.2>
@@ -1050,15 +1045,15 @@ test:do_execsql_test(
 --         -- </fkey2-10.5>
 --     })
 
-test:do_execsql_test(
+test:do_catchsql_test(
     "fkey2-10.6",
     [[
         DROP TABLE IF EXISTS t2;
         DROP TABLE IF EXISTS t1;
         CREATE TABLE t1(a PRIMARY KEY, b REFERENCES nosuchtable);
-        DROP TABLE t1;
     ]], {
         -- <fkey2-10.6>
+        1, "foreign key constraint references nonexistent table: NOSUCHTABLE
         -- </fkey2-10.6>
     })
 
@@ -1211,8 +1206,8 @@ test:do_execsql_test(
     "fkey2-10.20",
     [[
         DROP VIEW IF EXISTS v;
-        CREATE TABLE t1(x PRIMARY KEY REFERENCES v);
         CREATE VIEW v AS SELECT * FROM t1;
+        CREATE TABLE t1(x PRIMARY KEY REFERENCES v);
         DROP VIEW v;
     ]], {
         -- <fkey2-10.20>
diff --git a/test/sql-tap/fkey3.test.lua b/test/sql-tap/fkey3.test.lua
index 82796ba33..2532ec6a0 100755
--- a/test/sql-tap/fkey3.test.lua
+++ b/test/sql-tap/fkey3.test.lua
@@ -36,7 +36,7 @@ test:do_catchsql_test(
         DROP TABLE t1;
     ]], {
         -- <fkey3-1.3.1>
-        1, "FOREIGN KEY constraint failed"
+        1, "can't drop parent table T1 when child table refers to it"
         -- </fkey3-1.3.1>
     })
 
@@ -46,7 +46,7 @@ test:do_catchsql_test(
         DROP TABLE t1;
     ]], {
         -- <fkey3-1.3.2>
-        1, "FOREIGN KEY constraint failed"
+        1, "can't drop parent table T1 when child table refers to it"
         -- </fkey3-1.3.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 
 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);
         CREATE TABLE t6(a REFERENCES t4(a) MATCH PARTIAL primary key);
     ]], {
         -- <table-10.2>
diff --git a/test/sql-tap/tkt-b1d3a2e531.test.lua b/test/sql-tap/tkt-b1d3a2e531.test.lua
index 5cfa2e12b..951299dbd 100755
--- a/test/sql-tap/tkt-b1d3a2e531.test.lua
+++ b/test/sql-tap/tkt-b1d3a2e531.test.lua
@@ -124,7 +124,7 @@ test:do_catchsql_test(
           DROP TABLE cc1;
     ]], {
         -- <3.2>
-        1, "FOREIGN KEY constraint failed"
+        1, "can't drop parent table PP1 when child table refers to it"
         -- </3.2>
     })
 
-- 
2.15.1