From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp62.i.mail.ru (smtp62.i.mail.ru [217.69.128.42]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id C1314452566 for ; Tue, 12 Nov 2019 01:32:53 +0300 (MSK) References: <20191111135246.79818-1-korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Mon, 11 Nov 2019 23:38:58 +0100 MIME-Version: 1.0 In-Reply-To: <20191111135246.79818-1-korablev@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH] box: don't allow referenced collation to be dropped List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik , tarantool-patches@dev.tarantool.org Hi! Thanks for the patch! See 3 comments below. On 11/11/2019 14:52, Nikita Pettik wrote: > This patch fixes one debt related to collations: collation is not allowed > to be dropped until at least one space format or index references to it. > Otherwise, we may face situation when collation doesn't exist anymore, > but space has references to it (in-memory DD objects are not updated on > collation alter). To achieve this we simply iterate over all spaces and > check their formats and indexes. Despite it takes > O(space_count * (field_count + index_count)) complexity, it is not a big > deal: drop of collation is considered to be rare operation. So instead > of maintaining reference counters we'd better once spend a bit more > time during drop. > > Closes #4544 > --- > Branch: https://github.com/tarantool/tarantool/commits/np/gh-4544-drop-collation > Issue: https://github.com/tarantool/tarantool/issues/4544 > > src/box/alter.cc | 48 +++++++++++++++++++++++++++++++++++++++++++-- > test/box/net.box.result | 4 ++-- > test/box/net.box.test.lua | 2 +- > test/sql/collation.result | 33 ++++++++++++++++++++++++++++++- > test/sql/collation.test.lua | 11 +++++++++++ > 5 files changed, 92 insertions(+), 6 deletions(-) > > diff --git a/src/box/alter.cc b/src/box/alter.cc > index 697b7dbf3..f4232ce34 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -3228,6 +3228,48 @@ on_drop_collation_rollback(struct trigger *trigger, void *event) > return 0; > } > > +/** > + * Check if collation is referenced by any space. If so return -1. > + * If collation is mentioned in no one space or index definition, > + * return 0. > + */ > +static int > +space_check_coll_entry(struct space *space, void *data) > +{ > + uint32_t coll_id = *(uint32_t *) data; > + for (uint32_t i = 0; i < space->index_count; ++i) { > + struct index *idx = space->index[i]; > + for (uint32_t j = 0; j < idx->def->key_def->part_count; ++j) { > + struct key_part kp = idx->def->key_def->parts[j]; 1. Please, don't copy key_part by value. This is a big structure. > + if (kp.coll_id == coll_id) { > + char *id_str = tt_static_buf(); > + sprintf(id_str, "%u", coll_id); > + char *msg = tt_static_buf(); > + sprintf(msg, "index %d of space %s has " > + "references to collation", > + idx->def->iid, space->def->name); 2. You can use tt_sprintf(), in both cases. Below too. > + diag_set(ClientError, ER_DROP_COLLATION, id_str, > + msg); > + return -1; > + } > + } > + for (uint32_t j = 0; j < space->def->field_count; ++j) { > + if (space->def->fields[j].coll_id == coll_id) { > + char *id_str = tt_static_buf(); > + sprintf(id_str, "%u", coll_id); > + char *msg = tt_static_buf(); > + sprintf(msg, "format of space %s has " > + "references to collation", > + space->def->name); > + diag_set(ClientError, ER_DROP_COLLATION, id_str, > + msg); > + return -1; > + } > + } > + } > + return 0; > +} 3. Collations are referenced not only by indexes and spaces. tarantool> key_def = require('key_def') --- ... tarantool> kd = key_def.new({{fieldno = 1, collation = 'unicode', type = 'string'}}) --- ... tarantool> box.space._collation:delete(1) --- - [1, 'unicode', 1, 'ICU', '', {'strength': 'tertiary'}] ... tarantool> kd Assertion failed: (coll_id != NULL), function luaT_push_key_def, file /Users/gerold/Work/Repositories/tarantool/src/box/lua/key_def.c, line 67. Process 50999 stopped * thread #1, queue = 'com.apple.main-thread', stop reason = signal SIGABRT frame #0: 0x00007fff688be2c6 libsystem_kernel.dylib`__pthread_kill + 10 libsystem_kernel.dylib`__pthread_kill: -> 0x7fff688be2c6 <+10>: jae 0x7fff688be2d0 ; <+20> 0x7fff688be2c8 <+12>: movq %rax, %rdi 0x7fff688be2cb <+15>: jmp 0x7fff688b8457 ; cerror_nocancel 0x7fff688be2d0 <+20>: retq Target 0: (tarantool) stopped.