[tarantool-patches] Re: [PATCH] sql: rework VIEW internals

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

> On 7 Jun 2018, at 13:40, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Hello. Thanks for the fixes!
> 
> I have found another crash:
> 
> box.cfg{}
> box.sql.execute('CREATE TABLE test (id INT PRIMARY KEY)')
> fiber = require('fiber')
> function create_view() box.sql.execute('CREATE VIEW view1 AS SELECT * FROM test') end
> function drop_index() box.space._index:delete{box.space.TEST.id, 0} end
> function drop_space() box.space._space:delete{box.space.TEST.id} end
> box.error.injection.set("ERRINJ_WAL_DELAY", true)
> f1 = fiber.create(create_view)
> f2 = fiber.create(drop_index)
> f3 = fiber.create(drop_space)
> box.error.injection.set("ERRINJ_WAL_DELAY", false)
> 
> tarantool> Assertion failed: (space_id != BOX_ID_NIL), function update_view_references, file /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/alter.cc, line 1463.
> Process 43720 stopped
> * thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT
>    frame #0: 0x00007fff500f3b6e libsystem_kernel.dylib`__pthread_kill + 10
> libsystem_kernel.dylib`__pthread_kill:
> ->  0x7fff500f3b6e <+10>: jae    0x7fff500f3b78            ; <+20>
>    0x7fff500f3b70 <+12>: movq   %rax, %rdi
>    0x7fff500f3b73 <+15>: jmp    0x7fff500eab00            ; cerror_nocancel
>    0x7fff500f3b78 <+20>: retq
> Target 0: (tarantool) stopped.
> 
> So we can not increment view references on CREATE VIEW commit.
> We must do it in on_replace_dd_space, and decrement back in
> on_rollback trigger.
> 
> For DROP VIEW almost the same - we still must decrement on commit,
> but be ready that some spaces could be deleted, and just skip them.
> 
> I have made a small patch. But it is raw. Please, finish it, or made
> your own. If you will commit the test above, then please do it in a
> separate test file and make it release_disabled in suite.ini. Error
> injections do not work in release build. For instance,
> sql-tap/errinj.test.lua.

Well, I slightly completed your changes:

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

diff --git a/src/box/alter.cc b/src/box/alter.cc
index fe82006cf..359c712ee 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1447,9 +1447,14 @@ alter_space_move_indexes(struct alter_space *alter, uint32_t begin,
  *
  * @param select Tables from this select to be updated.
  * @param update_value +1 on view creation, -1 on drop.
+ * @param suppress_error If true, silently skip nonexistent
+ *                       spaces from 'FROM' clause.
+ * @retval 0 on success, -1 if suppress_error is false and space
+ *         from 'FROM' clause doesn't exist.
  */
-static void
-update_view_references(struct Select *select, int update_value)
+static int
+update_view_references(struct Select *select, int update_value,
+		       bool suppress_error)
 {
 	assert(update_value == 1 || update_value == -1);
 	sql_select_expand_from_tables(select);
@@ -1460,48 +1465,65 @@ update_view_references(struct Select *select, int update_value)
 			continue;
 		uint32_t space_id = schema_find_id(BOX_SPACE_ID, 2, space_name,
 						   strlen(space_name));
-		assert(space_id != BOX_ID_NIL);
+		if (space_id == BOX_ID_NIL) {
+			if (! suppress_error)
+				return -1;
+			continue;
+		}
 		struct space *space = space_by_id(space_id);
 		assert(space->def->view_ref_count > 0 || update_value > 0);
 		space->def->view_ref_count += update_value;
 	}
+	return 0;
 }
 
 /**
- * Trigger which is fired on creation of new SQL view.
- * Its purpose is to increment view reference counters of
- * dependent spaces.
+ * Trigger which is fired to commit creation of new SQL view.
+ * Its purpose is to release memory of SELECT.
  */
 static void
 on_create_view_commit(struct trigger *trigger, void *event)
 {
 	(void) event;
 	struct Select *select = (struct Select *)trigger->data;
-	update_view_references(select, 1);
 	sql_select_delete(sql_get(), select);
 }
 
 /**
- * Trigger which is fired on drop of SQL view.
+ * Trigger which is fired to rollback creation of new SQL view.
+ * Decrements view reference counters of dependent spaces and
+ * releases memory for SELECT.
+ */
+static void
+on_create_view_rollback(struct trigger *trigger, void *event)
+{
+	(void) event;
+	struct Select *select = (struct Select *)trigger->data;
+	(void) update_view_references(select, -1, true);
+	sql_select_delete(sql_get(), select);
+}
+
+/**
+ * Trigger which is fired to commit drop of SQL view.
  * Its purpose is to decrement view reference counters of
- * dependent spaces.
+ * dependent spaces and release memory for SELECT.
  */
 static void
 on_drop_view_commit(struct trigger *trigger, void *event)
 {
 	(void) event;
 	struct Select *select = (struct Select *)trigger->data;
-	update_view_references(select, -1);
+	(void) update_view_references(select, -1, true);
 	sql_select_delete(sql_get(), select);
 }
 
 /**
- * This trigger is invoked on drop/create of SQL view.
+ * This trigger is invoked to rollback drop of SQL view.
  * Release memory for struct SELECT compiled in
  * on_replace_dd_space trigger.
  */
 static void
-on_alter_view_rollback(struct trigger *trigger, void *event)
+on_drop_view_rollback(struct trigger *trigger, void *event)
 {
 	(void) event;
 	struct Select *select = (struct Select *)trigger->data;
@@ -1610,23 +1632,34 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
 		 * so it's safe to simply drop the space on
 		 * rollback.
 		 */
-		struct trigger *on_commit =
-			txn_alter_trigger_new(on_create_space_commit, space);
-		txn_on_commit(txn, on_commit);
 		if (def->opts.is_view) {
 			struct Select *select = sql_view_compile(sql_get(),
 								 def->opts.sql);
 			if (select == NULL)
 				diag_raise();
+			if (update_view_references(select, 1, false) != 0) {
+				/*
+				 * Decrement counters which have
+				 * been increased by previous call.
+				 */
+				(void) update_view_references(select, -1,
+							      false);
+				sql_select_delete(sql_get(), select);
+				tnt_raise(ClientError,
+					  ER_VIEW_MISSING_REFERENCED_SPACE);
+			}
 			struct trigger *on_commit_view =
 				txn_alter_trigger_new(on_create_view_commit,
 						      select);
 			txn_on_commit(txn, on_commit_view);
 			struct trigger *on_rollback_view =
-				txn_alter_trigger_new(on_alter_view_rollback,
+				txn_alter_trigger_new(on_create_view_rollback,
 						      select);
 			txn_on_rollback(txn, on_rollback_view);
 		}
+		struct trigger *on_commit =
+			txn_alter_trigger_new(on_create_space_commit, space);
+		txn_on_commit(txn, on_commit);
 		struct trigger *on_rollback =
 			txn_alter_trigger_new(on_create_space_rollback, space);
 		txn_on_rollback(txn, on_rollback);
@@ -1673,7 +1706,7 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
 						      select);
 			txn_on_commit(txn, on_commit_view);
 			struct trigger *on_rollback_view =
-				txn_alter_trigger_new(on_alter_view_rollback,
+				txn_alter_trigger_new(on_drop_view_rollback,
 						      select);
 			txn_on_rollback(txn, on_rollback_view);
 		}
diff --git a/src/box/errcode.h b/src/box/errcode.h
index ba5288059..e421a8d58 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -215,6 +215,8 @@ struct errcode_record {
 	/*160 */_(ER_ACTION_MISMATCH,		"Field %d contains %s on conflict action, but %s in index parts") \
 	/*161 */_(ER_VIEW_MISSING_SQL,		"Space declared as a view must have SQL statement") \
 	/*162 */_(ER_FOREIGN_KEY_CONSTRAINT,	"Can not commit transaction: deferred foreign keys violations are not resolved") \
+	/*163 */_(ER_VIEW_MISSING_REFERENCED_SPACE, "Can not create view: referenced space does not exists") \
+
 
 /*
  * !IMPORTANT! Please follow instructions at start of the file
diff --git a/test/box/misc.result b/test/box/misc.result
index 59f168f67..240aa40e7 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -406,8 +406,9 @@ t;
   - 'box.error.FOREIGN_KEY_CONSTRAINT : 162'
   - 'box.error.CROSS_ENGINE_TRANSACTION : 81'
   - 'box.error.ACTION_MISMATCH : 160'
-  - 'box.error.FORMAT_MISMATCH_INDEX_PART : 27'
   - 'box.error.injection : table: <address>
+  - 'box.error.FORMAT_MISMATCH_INDEX_PART : 27'
+  - 'box.error.DROP_SPACE : 11'
   - 'box.error.FUNCTION_TX_ACTIVE : 30'
   - 'box.error.SQL_BIND_NOT_FOUND : 159'
   - 'box.error.RELOAD_CFG : 58'
@@ -452,7 +453,7 @@ t;
   - 'box.error.ROLE_NOT_GRANTED : 92'
   - 'box.error.NO_SUCH_SPACE : 36'
   - 'box.error.WRONG_INDEX_PARTS : 107'
-  - 'box.error.DROP_SPACE : 11'
+  - 'box.error.VIEW_MISSING_REFERENCED_SPACE : 163'
   - 'box.error.MIN_FIELD_COUNT : 39'
   - 'box.error.REPLICASET_UUID_MISMATCH : 63'
   - 'box.error.UPDATE_FIELD : 29'
diff --git a/test/sql/suite.ini b/test/sql/suite.ini
index 2921aa197..5f28f23a3 100644
--- a/test/sql/suite.ini
+++ b/test/sql/suite.ini
@@ -5,4 +5,4 @@ script = app.lua
 use_unix_sockets = True
 is_parallel = True
 lua_libs = lua/sql_tokenizer.lua
-release_disabled = errinj.test.lua
+release_disabled = errinj.test.lua view_delayed_wal.test.lua

new file mode 100644
index 000000000..6a9a6a7c8
--- /dev/null
+++ b/test/sql/view_delayed_wal.result
@@ -0,0 +1,96 @@
+test_run = require('test_run').new()
+---
+...
+fiber = require('fiber')
+---
+...
+-- View reference counters are incremented before firing
+-- on_commit triggers (i.e. before being written into WAL), so
+-- it is impossible to create view on dropped (but not written
+-- into WAL) space.
+--
+box.sql.execute('CREATE TABLE t1(id INT PRIMARY KEY)')
+---
+...
+function create_view() box.sql.execute('CREATE VIEW v1 AS SELECT * FROM t1') end
+---
+...
+function drop_index_t1() box.space._index:delete{box.space.T1.id, 0} end
+---
+...
+function drop_space_t1() box.space._space:delete{box.space.T1.id} end
+---
+...
+box.error.injection.set("ERRINJ_WAL_DELAY", true)
+---
+- ok
+...
+f1 = fiber.create(create_view)
+---
+...
+f2 = fiber.create(drop_index_t1)
+---
+...
+f3 = fiber.create(drop_space_t1)
+---
+...
+box.error.injection.set("ERRINJ_WAL_DELAY", false)
+---
+- ok
+...
+box.space.T1
+---
+- null
+...
+box.space.V1
+---
+- null
+...
+-- In the same way, we have to drop all referenced spaces before
+-- dropping view, since view reference counter of space to be
+-- dropped is checked before firing on_commit trigger. 
+--
+box.sql.execute('CREATE TABLE t2 (id INT PRIMARY KEY)')
+---
+...
+box.sql.execute('CREATE VIEW view2 AS SELECT * FROM t2')
+---
+...
+function drop_view() box.space._space:delete{box.space.VIEW2.id} end
+---
+...
+function drop_index_t2() box.space._index:delete{box.space.T2.id, 0} end
+---
+...
+function drop_space_t2() box.space._space:delete{box.space.T2.id} end
+---
+...
+box.error.injection.set("ERRINJ_WAL_DELAY", true)
+---
+- ok
+...
+f1 = fiber.create(drop_index_t2)
+---
+...
+f2 = fiber.create(drop_space_t2)
+---
+...
+f3 = fiber.create(drop_view)
+---
+...
+box.error.injection.set("ERRINJ_WAL_DELAY", false)
+---
+- ok
+...
+box.space._space:get{box.space.T2.id}['name']
+---
+- T2
+...
+box.space.V2
+---
+- null
+...
+-- Since deletion via Lua doesn't remove entry from
+-- SQL data dictionary we have to restart instance to clean up.
+...
+function drop_index_t1() box.space._index:delete{box.space.T1.id, 0} end
+---
+...
+function drop_space_t1() box.space._space:delete{box.space.T1.id} end
+---
+...
+box.error.injection.set("ERRINJ_WAL_DELAY", true)
+---
+- ok
+...
+f1 = fiber.create(create_view)
+---
+...
+f2 = fiber.create(drop_index_t1)
+---
+...
+f3 = fiber.create(drop_space_t1)
+---
+...
+box.error.injection.set("ERRINJ_WAL_DELAY", false)
+---
+- ok
+...
+box.space.T1
+---
+- null
+...
+box.space.V1
+---
+- null
+...
+-- In the same way, we have to drop all referenced spaces before
+-- dropping view, since view reference counter of space to be
+-- dropped is checked before firing on_commit trigger. 
+--
+box.sql.execute('CREATE TABLE t2 (id INT PRIMARY KEY)')
+---
+...
+box.sql.execute('CREATE VIEW view2 AS SELECT * FROM t2')
+---
+...
+function drop_view() box.space._space:delete{box.space.VIEW2.id} end
+---
+...
+function drop_index_t2() box.space._index:delete{box.space.T2.id, 0} end
+---
+...
+function drop_space_t2() box.space._space:delete{box.space.T2.id} end
+---
+...
+box.error.injection.set("ERRINJ_WAL_DELAY", true)
+---
+- ok
+...
+f1 = fiber.create(drop_index_t2)
+---
+...
+f2 = fiber.create(drop_space_t2)
+---
+...
+f3 = fiber.create(drop_view)
+---
+...
+box.error.injection.set("ERRINJ_WAL_DELAY", false)
+---
+- ok
+...
+box.space._space:get{box.space.T2.id}['name']
+---
+- T2
+...
+box.space.V2
+---
+- null
+...
+-- Since deletion via Lua doesn't remove entry from
+-- SQL data dictionary we have to restart instance to clean up.
+--
+test_run:cmd('restart server default')
diff --git a/test/sql/view_delayed_wal.test.lua b/test/sql/view_delayed_wal.test.lua
new file mode 100644
index 000000000..b7d616142
--- /dev/null
+++ b/test/sql/view_delayed_wal.test.lua
@@ -0,0 +1,42 @@
+test_run = require('test_run').new()
+fiber = require('fiber')
+
+-- View reference counters are incremented before firing
+-- on_commit triggers (i.e. before being written into WAL), so
+-- it is impossible to create view on dropped (but not written
+-- into WAL) space.
+--
+box.sql.execute('CREATE TABLE t1(id INT PRIMARY KEY)')
+function create_view() box.sql.execute('CREATE VIEW v1 AS SELECT * FROM t1') end
+function drop_index_t1() box.space._index:delete{box.space.T1.id, 0} end
+function drop_space_t1() box.space._space:delete{box.space.T1.id} end
+box.error.injection.set("ERRINJ_WAL_DELAY", true)
+f1 = fiber.create(create_view)
+f2 = fiber.create(drop_index_t1)
+f3 = fiber.create(drop_space_t1)
+box.error.injection.set("ERRINJ_WAL_DELAY", false)
+box.space.T1
+box.space.V1
+
+-- In the same way, we have to drop all referenced spaces before
+-- dropping view, since view reference counter of space to be
+-- dropped is checked before firing on_commit trigger.
+--
+box.sql.execute('CREATE TABLE t2 (id INT PRIMARY KEY)')
+box.sql.execute('CREATE VIEW view2 AS SELECT * FROM t2')
+
+function drop_view() box.space._space:delete{box.space.VIEW2.id} end
+function drop_index_t2() box.space._index:delete{box.space.T2.id, 0} end
+function drop_space_t2() box.space._space:delete{box.space.T2.id} end
+box.error.injection.set("ERRINJ_WAL_DELAY", true)
+f1 = fiber.create(drop_index_t2)
+f2 = fiber.create(drop_space_t2)
+f3 = fiber.create(drop_view)
+box.error.injection.set("ERRINJ_WAL_DELAY", false)
+box.space._space:get{box.space.T2.id}['name']
+box.space.V2
+
+-- Since deletion via Lua doesn't remove entry from
+-- SQL data dictionary we have to restart instance to clean up.
+--
+test_run:cmd('restart server default')