Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: Nikita Pettik <korablev@tarantool.org>,
	tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH] box: don't allow referenced collation to be dropped
Date: Mon, 11 Nov 2019 23:38:58 +0100	[thread overview]
Message-ID: <fbd2a38b-d7cd-2cee-d89a-4fbf38139667@tarantool.org> (raw)
In-Reply-To: <20191111135246.79818-1-korablev@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.

  parent reply	other threads:[~2019-11-11 22:32 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-11 13:52 Nikita Pettik
2019-11-11 21:50 ` Konstantin Osipov
2019-11-11 22:38 ` Vladislav Shpilevoy [this message]
2019-11-12 13:24   ` Nikita Pettik
2019-11-13  4:37     ` Konstantin Osipov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fbd2a38b-d7cd-2cee-d89a-4fbf38139667@tarantool.org \
    --to=v.shpilevoy@tarantool.org \
    --cc=korablev@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] box: don'\''t allow referenced collation to be dropped' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox