From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id DC7C925DC6 for ; Thu, 7 Jun 2018 14:25:35 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id WbhXUGffGh_u for ; Thu, 7 Jun 2018 14:25:35 -0400 (EDT) Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 078E125DBC for ; Thu, 7 Jun 2018 14:25:34 -0400 (EDT) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH] sql: rework VIEW internals From: "n.pettik" In-Reply-To: Date: Thu, 7 Jun 2018 21:25:31 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <1528129571.147907906@f369.i.mail.ru> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy > On 7 Jun 2018, at 13:40, Vladislav Shpilevoy = wrote: >=20 > Hello. Thanks for the fixes! >=20 > I have found another crash: >=20 > box.cfg{} > box.sql.execute('CREATE TABLE test (id INT PRIMARY KEY)') > fiber =3D 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 =3D fiber.create(create_view) > f2 =3D fiber.create(drop_index) > f3 =3D fiber.create(drop_space) > box.error.injection.set("ERRINJ_WAL_DELAY", false) >=20 > tarantool> Assertion failed: (space_id !=3D 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 =3D 'com.apple.main-thread', stop reason =3D 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. >=20 > 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. >=20 > 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. >=20 > 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: =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D 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 =3D=3D 1 || update_value =3D=3D -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 =3D schema_find_id(BOX_SPACE_ID, 2, = space_name, strlen(space_name)); - assert(space_id !=3D BOX_ID_NIL); + if (space_id =3D=3D BOX_ID_NIL) { + if (! suppress_error) + return -1; + continue; + } struct space *space =3D space_by_id(space_id); assert(space->def->view_ref_count > 0 || update_value > = 0); space->def->view_ref_count +=3D update_value; } + return 0; } =20 /** - * 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 =3D (struct Select *)trigger->data; - update_view_references(select, 1); sql_select_delete(sql_get(), select); } =20 /** - * 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 =3D (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 =3D (struct Select *)trigger->data; - update_view_references(select, -1); + (void) update_view_references(select, -1, true); sql_select_delete(sql_get(), select); } =20 /** - * 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 =3D (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 =3D - txn_alter_trigger_new(on_create_space_commit, = space); - txn_on_commit(txn, on_commit); if (def->opts.is_view) { struct Select *select =3D = sql_view_compile(sql_get(), = def->opts.sql); if (select =3D=3D NULL) diag_raise(); + if (update_view_references(select, 1, false) !=3D = 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 =3D = txn_alter_trigger_new(on_create_view_commit, select); txn_on_commit(txn, on_commit_view); struct trigger *on_rollback_view =3D - = 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 =3D + txn_alter_trigger_new(on_create_space_commit, = space); + txn_on_commit(txn, on_commit); struct trigger *on_rollback =3D 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 =3D - = 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") \ + =20 /* * !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:
+ - '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 =3D app.lua use_unix_sockets =3D True is_parallel =3D True lua_libs =3D lua/sql_tokenizer.lua -release_disabled =3D errinj.test.lua +release_disabled =3D 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 =3D require('test_run').new() +--- +... +fiber =3D 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 =3D fiber.create(create_view) +--- +... +f2 =3D fiber.create(drop_index_t1) +--- +... +f3 =3D 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.=20 +-- +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 =3D fiber.create(drop_index_t2) +--- +... +f2 =3D fiber.create(drop_space_t2) +--- +... +f3 =3D 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 =3D fiber.create(create_view) +--- +... +f2 =3D fiber.create(drop_index_t1) +--- +... +f3 =3D 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.=20 +-- +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 =3D fiber.create(drop_index_t2) +--- +... +f2 =3D fiber.create(drop_space_t2) +--- +... +f3 =3D 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 =3D require('test_run').new() +fiber =3D 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 =3D fiber.create(create_view) +f2 =3D fiber.create(drop_index_t1) +f3 =3D 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 =3D fiber.create(drop_index_t2) +f2 =3D fiber.create(drop_space_t2) +f3 =3D 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')