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

["n.pettik" <korablev@tarantool.org>]

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