[tarantool-patches] Re: [PATCH] sql: rework VIEW internals
n.pettik
korablev at tarantool.org
Thu Jun 7 21:25:31 MSK 2018
> On 7 Jun 2018, at 13:40, Vladislav Shpilevoy <v.shpilevoy at 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')
More information about the Tarantool-patches
mailing list