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 D303822FAD for ; Wed, 25 Apr 2018 19:29:13 -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 VrbvGyYqjlgR for ; Wed, 25 Apr 2018 19:29:13 -0400 (EDT) Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (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 0B2C422C64 for ; Wed, 25 Apr 2018 19:29:12 -0400 (EDT) From: Vladislav Shpilevoy Subject: [tarantool-patches] [PATCH 3/7] alter: fix assertion in collations alter Date: Thu, 26 Apr 2018 02:29:03 +0300 Message-Id: In-Reply-To: References: In-Reply-To: References: 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: kostja@tarantool.org Forbid collations update - it leads to an assertion in collation cache about unavailability to correctly replace collations. Refactor collations deletion and insertion. Besides, collation update potentially changes indexing order with no actual indexes rebuilding. --- src/box/alter.cc | 115 ++++++++++++++++++++++++++------------------------ test/box/ddl.result | 10 +++++ test/box/ddl.test.lua | 3 ++ 3 files changed, 74 insertions(+), 54 deletions(-) diff --git a/src/box/alter.cc b/src/box/alter.cc index 77b03e680..be2ccc061 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -2376,19 +2376,31 @@ coll_cache_rollback(struct trigger *trigger, void *event) struct txn_stmt *stmt = txn_last_stmt((struct txn*) event); struct tuple *new_tuple = stmt->new_tuple; - if (new_tuple != NULL) { + if (new_tuple == NULL && old_coll != NULL) { + /* Rollback DELETE. */ + if (coll_by_id(old_coll->id) == old_coll) { + /* + * Nothing to do - the tx had been failed + * before old collation deletion. + */ + return; + } + struct coll *replaced; + if (coll_cache_replace(old_coll, &replaced) != 0) { + panic("Out of memory on insertion into collation "\ + "cache"); + } + assert(replaced == NULL); + } else { + assert(new_tuple != NULL && old_coll == NULL); uint32_t new_id = tuple_field_u32_xc(new_tuple, BOX_COLLATION_FIELD_ID); struct coll *new_coll = coll_by_id(new_id); - coll_cache_delete(new_coll); - coll_delete(new_coll); - } - - if (old_coll != NULL) { - struct coll *replaced; - int rc = coll_cache_replace(old_coll, &replaced); - assert(rc == 0 && replaced == NULL); - (void)rc; + /* Can be NULL, if failed before cache update. */ + if (new_coll != NULL) { + coll_cache_delete(new_coll); + coll_delete(new_coll); + } } } @@ -2412,59 +2424,54 @@ on_replace_dd_collation(struct trigger * /* trigger */, void *event) struct tuple *old_tuple = stmt->old_tuple; struct tuple *new_tuple = stmt->new_tuple; txn_check_singlestatement_xc(txn, "Space _collation"); - struct coll *old_coll = NULL; - if (old_tuple != NULL) { + if (new_tuple == NULL && old_tuple != NULL) { + /* DELETE */ /* TODO: Check that no index uses the collation */ - uint32_t old_id = tuple_field_u32_xc(old_tuple, - BOX_COLLATION_FIELD_ID); - old_coll = coll_by_id(old_id); + int32_t old_id = tuple_field_u32_xc(old_tuple, + BOX_COLLATION_FIELD_ID); + struct coll *old_coll = coll_by_id(old_id); assert(old_coll != NULL); access_check_ddl(old_coll->name, old_coll->owner_id, - SC_COLLATION, - new_tuple == NULL ? PRIV_D: PRIV_A, - false); - - struct trigger *on_commit = - txn_alter_trigger_new(coll_cache_delete_coll, old_coll); - txn_on_commit(txn, on_commit); - } - - if (new_tuple == NULL) { - /* Simple DELETE */ - assert(old_tuple != NULL); - coll_cache_delete(old_coll); - + SC_COLLATION, PRIV_D, false); + /* + * Set on_rollback before deletion from the cache. + * Else rollback setting can fail and the + * collation will be lost. + */ struct trigger *on_rollback = txn_alter_trigger_new(coll_cache_rollback, old_coll); txn_on_rollback(txn, on_rollback); - return; - } - - struct coll_def new_def; - coll_def_new_from_tuple(new_tuple, &new_def); - access_check_ddl(new_def.name, new_def.owner_id, SC_COLLATION, - old_tuple == NULL ? PRIV_C : PRIV_A, false); - struct coll *new_coll = coll_new(&new_def); - if (new_coll == NULL) - diag_raise(); - auto def_guard = make_scoped_guard([=] { coll_delete(new_coll); }); - - struct coll *replaced; - if (coll_cache_replace(new_coll, &replaced) != 0) - diag_raise(); - if (replaced == NULL && old_coll != NULL) { + coll_cache_delete(old_coll); + struct trigger *on_commit = + txn_alter_trigger_new(coll_cache_delete_coll, old_coll); + txn_on_commit(txn, on_commit); + } else if (new_tuple != NULL && old_tuple == NULL) { + /* INSERT */ + struct coll_def new_def; + coll_def_new_from_tuple(new_tuple, &new_def); + access_check_ddl(new_def.name, new_def.owner_id, SC_COLLATION, + PRIV_C, false); /* - * ID of a collation was changed. - * Remove collation by old ID. + * Set on_rollback before the collation is + * inserted into the cache. Else if the trigger + * creation fails the collation will not be + * deleted on abort. */ - coll_cache_delete(old_coll); + struct trigger *on_rollback = + txn_alter_trigger_new(coll_cache_rollback, NULL); + txn_on_rollback(txn, on_rollback); + struct coll *new_coll = coll_new(&new_def); + if (new_coll == NULL) + diag_raise(); + struct coll *replaced; + if (coll_cache_replace(new_coll, &replaced) != 0) + diag_raise(); + assert(replaced == NULL); + } else { + /* UPDATE */ + assert(new_tuple != NULL && old_tuple != NULL); + tnt_raise(ClientError, ER_UNSUPPORTED, "collation", "alter"); } - - struct trigger *on_rollback = - txn_alter_trigger_new(coll_cache_rollback, old_coll); - txn_on_rollback(txn, on_rollback); - - def_guard.is_active = false; } /** diff --git a/test/box/ddl.result b/test/box/ddl.result index 0eef37992..87b9581c6 100644 --- a/test/box/ddl.result +++ b/test/box/ddl.result @@ -289,6 +289,16 @@ box.internal.collation.create('test', 'ICU', 'ru_RU', {strength='primary'}) --ok box.internal.collation.drop('test') --ok --- ... +c = box.space._collation:get{1}:totable() +--- +... +c[2] = 'unicode_test' +--- +... +box.space._collation:replace(c) +--- +- error: collation does not support alter +... box.begin() box.internal.collation.create('test2', 'ICU', 'ru_RU') --- - error: Space _collation does not support multi-statement transactions diff --git a/test/box/ddl.test.lua b/test/box/ddl.test.lua index 820fe7d4d..a1502ae13 100644 --- a/test/box/ddl.test.lua +++ b/test/box/ddl.test.lua @@ -127,6 +127,9 @@ box.internal.collation.create('test', 'ICU', 'ru_RU', {strength=2}) --ok box.internal.collation.drop('test') --ok box.internal.collation.create('test', 'ICU', 'ru_RU', {strength='primary'}) --ok box.internal.collation.drop('test') --ok +c = box.space._collation:get{1}:totable() +c[2] = 'unicode_test' +box.space._collation:replace(c) box.begin() box.internal.collation.create('test2', 'ICU', 'ru_RU') box.rollback() -- 2.15.1 (Apple Git-101)