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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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>