Re: [Tarantool-patches] [PATCH] box: don't allow referenced collation to be dropped

[Vladislav Shpilevoy <v.shpilevoy@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.