[tarantool-patches] [PATCH 0/4] Fixes in SQL involving no-pk or no-format spaces

[tarantool-patches] [PATCH 0/4] Fixes in SQL involving no-pk or no-format spaces

[Nikita Pettik]

Branch: https://github.com/tarantool/tarantool/tree/np/fix-no-index-no-format-DDL

This patch-set consists of several (almost independent) fixes of
bugs which can result in crashes. All of them are based on situation
when SQL appeals to spaces which lack format or indexes.

Nikita Pettik (4):
  Drop foreign keys before indexes in space:drop()
  Fix creation of FK constraint in case of no child's PK
  sql: disallow creation of index on space without format
  sql: disallow creation of FK referencing space without PK

 src/box/alter.cc                 |  2 +-
 src/box/lua/schema.lua           |  6 +++---
 src/box/sql/build.c              | 23 +++++++++++++++++++++-
 src/box/sql/delete.c             |  4 +---
 src/box/sql/sqlInt.h             | 12 ++++++++++++
 test/sql-tap/lua-tables.test.lua | 17 ++++++++++++++++-
 test/sql/foreign-keys.result     | 41 ++++++++++++++++++++++++++++++++++++++++
 test/sql/foreign-keys.test.lua   | 21 ++++++++++++++++++++
 8 files changed, 117 insertions(+), 9 deletions(-)

-- 
2.15.1

[tarantool-patches] [PATCH 1/4] Drop foreign keys before indexes in space:drop()

[Nikita Pettik]

Implicitly involved in foreign key constraint index can't be dropped
before constraint itself. So, to drop self-referenced table, we should
firstly drop foreign key constraints, and only then indexes.
---
 src/box/lua/schema.lua         | 6 +++---
 test/sql/foreign-keys.result   | 8 ++++++++
 test/sql/foreign-keys.test.lua | 5 +++++
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index d9483a6b2..f31cf7f2c 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -521,14 +521,14 @@ box.schema.space.drop = function(space_id, space_name, opts)
     for _, t in _trigger.index.space_id:pairs({space_id}) do
         _trigger:delete({t.name})
     end
+    for _, t in _fk_constraint.index.child_id:pairs({space_id}) do
+        _fk_constraint:delete({t.name, space_id})
+    end
     local keys = _vindex:select(space_id)
     for i = #keys, 1, -1 do
         local v = keys[i]
         _index:delete{v[1], v[2]}
     end
-    for _, t in _fk_constraint.index.child_id:pairs({space_id}) do
-        _fk_constraint:delete({t.name, space_id})
-    end
     revoke_object_privs('space', space_id)
     _truncate:delete{space_id}
     if _space:delete{space_id} == nil then
diff --git a/test/sql/foreign-keys.result b/test/sql/foreign-keys.result
index 3c6464e7d..731807227 100644
--- a/test/sql/foreign-keys.result
+++ b/test/sql/foreign-keys.result
@@ -377,5 +377,13 @@ box.sql.execute('CREATE TABLE t2 (id INT PRIMARY KEY REFERENCES t2 ON DELETE CAS
 box.space.T1:drop()
 ---
 ...
+-- Make sure that space:drop() works fine on self-referenced spaces.
+--
+box.sql.execute("CREATE TABLE t4 (id INT PRIMARY KEY REFERENCES t4);")
+---
+...
+box.space.T4:drop()
+---
+...
 --- Clean-up SQL DD hash.
 -test_run:cmd('restart server default with cleanup=1')
diff --git a/test/sql/foreign-keys.test.lua b/test/sql/foreign-keys.test.lua
index 3fb7cab18..6f4941e09 100644
--- a/test/sql/foreign-keys.test.lua
+++ b/test/sql/foreign-keys.test.lua
@@ -169,5 +169,10 @@ box.sql.execute('CREATE TABLE t2 (id INT PRIMARY KEY REFERENCES t2 ON DELETE CAS
 box.sql.execute('CREATE TABLE t2 (id INT PRIMARY KEY REFERENCES t2 ON DELETE CASCADE MATCH FULL);')
 box.space.T1:drop()
 
+-- Make sure that space:drop() works fine on self-referenced spaces.
+--
+box.sql.execute("CREATE TABLE t4 (id INT PRIMARY KEY REFERENCES t4);")
+box.space.T4:drop()
+
 --- Clean-up SQL DD hash.
 -test_run:cmd('restart server default with cleanup=1')
-- 
2.15.1

[tarantool-patches] [PATCH 2/4] Fix creation of FK constraint in case of no child's PK

[Nikita Pettik]

on_replace_dd_fk_constraint() assumes that child's primary index
exists: it is used to verify emptiness of space invoking index_size().
This commit fixes this obvious bug which could lead to crash.
---
 src/box/alter.cc               |  2 +-
 test/sql/foreign-keys.result   | 25 +++++++++++++++++++++++++
 test/sql/foreign-keys.test.lua | 10 ++++++++++
 3 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 40b32eafe..2f783c8e7 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -3901,7 +3901,7 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
 		 * checks on existing data in space.
 		 */
 		struct index *pk = space_index(child_space, 0);
-		if (index_size(pk) > 0) {
+		if (pk != NULL && index_size(pk) > 0) {
 			tnt_raise(ClientError, ER_CREATE_FK_CONSTRAINT,
 				  fk_def->name,
 				  "referencing space must be empty");
diff --git a/test/sql/foreign-keys.result b/test/sql/foreign-keys.result
index 731807227..97c3e2442 100644
--- a/test/sql/foreign-keys.result
+++ b/test/sql/foreign-keys.result
@@ -385,5 +385,30 @@ box.sql.execute("CREATE TABLE t4 (id INT PRIMARY KEY REFERENCES t4);")
 box.space.T4:drop()
 ---
 ...
+-- Make sure that child space can feature no PK.
+--
+t1 = box.schema.create_space("T1")
+---
+...
+box.space.T1:format({'ID'})
+---
+...
+t2 = box.schema.create_space("T2")
+---
+...
+i1 = box.space.T2:create_index('I1')
+---
+...
+box.sql.execute("ALTER TABLE t1 ADD CONSTRAINT fk FOREIGN KEY (id) REFERENCES t2;")
+---
+- error: 'Failed to create foreign key constraint ''FK'': foreign key refers to nonexistent
+    field'
+...
+t1:drop()
+---
+...
+t2:drop()
+---
+...
 --- Clean-up SQL DD hash.
 -test_run:cmd('restart server default with cleanup=1')
diff --git a/test/sql/foreign-keys.test.lua b/test/sql/foreign-keys.test.lua
index 6f4941e09..078c10c54 100644
--- a/test/sql/foreign-keys.test.lua
+++ b/test/sql/foreign-keys.test.lua
@@ -174,5 +174,15 @@ box.space.T1:drop()
 box.sql.execute("CREATE TABLE t4 (id INT PRIMARY KEY REFERENCES t4);")
 box.space.T4:drop()
 
+-- Make sure that child space can feature no PK.
+--
+t1 = box.schema.create_space("T1")
+box.space.T1:format({'ID'})
+t2 = box.schema.create_space("T2")
+i1 = box.space.T2:create_index('I1')
+box.sql.execute("ALTER TABLE t1 ADD CONSTRAINT fk FOREIGN KEY (id) REFERENCES t2;")
+t1:drop()
+t2:drop()
+
 --- Clean-up SQL DD hash.
 -test_run:cmd('restart server default with cleanup=1')
-- 
2.15.1

[tarantool-patches] Re: [PATCH 2/4] Fix creation of FK constraint in case of no child's PK

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

I've made a tiny fix and force merged it into
the commit.

After that the patchset LGTM.

=============================================

diff --git a/test/sql/foreign-keys.result b/test/sql/foreign-keys.result
index 97c3e2442..d69f93a9c 100644
--- a/test/sql/foreign-keys.result
+++ b/test/sql/foreign-keys.result
@@ -387,10 +387,7 @@ box.space.T4:drop()
 ...
 -- Make sure that child space can feature no PK.
 --
-t1 = box.schema.create_space("T1")
----
-...
-box.space.T1:format({'ID'})
+t1 = box.schema.create_space("T1", {format = {'ID'}})
 ---
 ...
 t2 = box.schema.create_space("T2")
diff --git a/test/sql/foreign-keys.test.lua b/test/sql/foreign-keys.test.lua
index 078c10c54..4ddf5e83b 100644
--- a/test/sql/foreign-keys.test.lua
+++ b/test/sql/foreign-keys.test.lua
@@ -176,8 +176,7 @@ box.space.T4:drop()
 
 -- Make sure that child space can feature no PK.
 --
-t1 = box.schema.create_space("T1")
-box.space.T1:format({'ID'})
+t1 = box.schema.create_space("T1", {format = {'ID'}})
 t2 = box.schema.create_space("T2")
 i1 = box.space.T2:create_index('I1')
 box.sql.execute("ALTER TABLE t1 ADD CONSTRAINT fk FOREIGN KEY (id) REFERENCES t2;")

[tarantool-patches] [PATCH 3/4] sql: disallow creation of index on space without format

[Nikita Pettik]

During index creation from SQL we should resolve columns names, which in
turn impossible for spaces without format. Now it leads to crash. Let's
fix it by raising corresponding diag error.
---
 src/box/sql/build.c              | 16 ++++++++++++++++
 src/box/sql/delete.c             |  4 +---
 src/box/sql/sqlInt.h             | 12 ++++++++++++
 test/sql-tap/lua-tables.test.lua | 17 ++++++++++++++++-
 4 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 5b1e933c7..73ce5b7bf 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1996,6 +1996,18 @@ table_add_index(struct space *space, struct index *index)
 	space->index_id_max =  MAX(space->index_id_max, index->def->iid);;
 }
 
+int
+sql_space_def_check_format(const struct space_def *space_def)
+{
+	assert(space_def != NULL);
+	if (space_def->field_count == 0) {
+		diag_set(ClientError, ER_UNSUPPORTED, "SQL",
+			 "space without format");
+		return -1;
+	}
+	return 0;
+}
+
 /**
  * Create and set index_def in the given Index.
  *
@@ -2160,6 +2172,10 @@ sql_create_index(struct Parse *parse, struct Token *token,
 		parse->is_aborted = true;
 		goto exit_create_index;
 	}
+	if (sql_space_def_check_format(def) != 0) {
+		parse->is_aborted = true;
+		goto exit_create_index;
+	}
 	/*
 	 * Find the name of the index.  Make sure there is not
 	 * already another index with the same name.
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 5a3d7cfad..8d6e848bb 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -47,9 +47,7 @@ sql_lookup_space(struct Parse *parse, struct SrcList_item *space_name)
 		return NULL;
 	}
 	assert(space != NULL);
-	if (space->def->field_count == 0) {
-		diag_set(ClientError, ER_UNSUPPORTED, "SQL",
-			 "space without format");
+	if (sql_space_def_check_format(space->def) != 0) {
 		parse->is_aborted = true;
 		return NULL;
 	}
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 72bd4ee0f..22d21f74b 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4401,6 +4401,18 @@ void
 sql_drop_foreign_key(struct Parse *parse_context, struct SrcList *table,
 		     struct Token *constraint);
 
+/**
+ * Now our SQL implementation can't operate on spaces which
+ * lack format: it is reasonable since for instance we can't
+ * resolve column names, their types etc. In case of format
+ * absence, diag error is raised.
+ *
+ * @retval 0 in case space features format.
+ * @retval -1 if space doesn't have format.
+ */
+int
+sql_space_def_check_format(const struct space_def *space_def);
+
 /**
  * Counts the trail bytes for a UTF-8 lead byte of a valid UTF-8
  * sequence.
diff --git a/test/sql-tap/lua-tables.test.lua b/test/sql-tap/lua-tables.test.lua
index aa10c7066..7ba1d7ac5 100755
--- a/test/sql-tap/lua-tables.test.lua
+++ b/test/sql-tap/lua-tables.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(12)
+test:plan(14)
 
 test:do_test(
     "lua-tables-prepare-1",
@@ -160,4 +160,19 @@ test:do_eqp_test(
         {0, 0, 0, 'SEARCH TABLE TEST USING COVERING INDEX secondary (A=?)'}
     })
 
+-- Make sure that without format it is impossible to create
+-- an index: format is required to resolve column names.
+test:do_test(
+    "no-format-create-index-prep",
+    function()
+        s = box.schema.create_space('T')
+    end, {})
+
+test:do_catchsql_test(
+    "no-format-create-index",
+    [[
+        CREATE INDEX i1 ON t(id);
+    ]],
+        {1, "SQL does not support space without format"})
+
 test:finish_test()
-- 
2.15.1

[tarantool-patches] [PATCH 4/4] sql: disallow creation of FK referencing space without PK

[Nikita Pettik]

In our SQL implementation we can omit list of referenced columns in
FOREIGN KEY constraint:

... FOREIGN KEY (id) REFERENCES parent;

In this case columns composing primary key are used. Hence, it makes
it impossible to use this variant of FK statement if space doesn't have
primary key. Let's now raise an error in this case.
---
 src/box/sql/build.c            | 7 ++++++-
 test/sql/foreign-keys.result   | 8 ++++++++
 test/sql/foreign-keys.test.lua | 6 ++++++
 3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 73ce5b7bf..0a461f69e 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1797,7 +1797,12 @@ sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child,
 		 * columns of parent table are used as referenced.
 		 */
 		struct index *parent_pk = space_index(parent_space, 0);
-		assert(parent_pk != NULL);
+		if (parent_pk == NULL) {
+			diag_set(ClientError, ER_CREATE_FK_CONSTRAINT,
+				 constraint_name,
+				 "referenced space doesn't feature PRIMARY KEY");
+			goto tnt_error;
+		}
 		if (parent_pk->def->key_def->part_count != child_cols_count) {
 			diag_set(ClientError, ER_CREATE_FK_CONSTRAINT,
 				 constraint_name, error_msg);
diff --git a/test/sql/foreign-keys.result b/test/sql/foreign-keys.result
index 97c3e2442..e0b3eb51f 100644
--- a/test/sql/foreign-keys.result
+++ b/test/sql/foreign-keys.result
@@ -404,6 +404,14 @@ box.sql.execute("ALTER TABLE t1 ADD CONSTRAINT fk FOREIGN KEY (id) REFERENCES t2
 - error: 'Failed to create foreign key constraint ''FK'': foreign key refers to nonexistent
     field'
 ...
+-- Make sure that if referenced columns (of parent space) are
+-- ommitted and parent space doesn't have PK, then error is raised.
+--
+box.sql.execute("ALTER TABLE t2 ADD CONSTRAINT fk FOREIGN KEY (id) REFERENCES t1;")
+---
+- error: 'Failed to create foreign key constraint ''FK'': referenced space doesn''t
+    feature PRIMARY KEY'
+...
 t1:drop()
 ---
 ...
diff --git a/test/sql/foreign-keys.test.lua b/test/sql/foreign-keys.test.lua
index 078c10c54..3fb39ea43 100644
--- a/test/sql/foreign-keys.test.lua
+++ b/test/sql/foreign-keys.test.lua
@@ -181,6 +181,12 @@ box.space.T1:format({'ID'})
 t2 = box.schema.create_space("T2")
 i1 = box.space.T2:create_index('I1')
 box.sql.execute("ALTER TABLE t1 ADD CONSTRAINT fk FOREIGN KEY (id) REFERENCES t2;")
+
+-- Make sure that if referenced columns (of parent space) are
+-- ommitted and parent space doesn't have PK, then error is raised.
+--
+box.sql.execute("ALTER TABLE t2 ADD CONSTRAINT fk FOREIGN KEY (id) REFERENCES t1;")
+
 t1:drop()
 t2:drop()
 
-- 
2.15.1