[tarantool-patches] Re: [PATCH 1/5] sql: prohibit creation of FK on unexisting tables
n.pettik
korablev at tarantool.org
Wed Jul 25 13:03:45 MSK 2018
>> 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);
More information about the Tarantool-patches
mailing list