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

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

[Nikita Pettik]

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];
+			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);
+				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;
+}
+
 /**
  * A trigger invoked on replace in a space containing
  * collations that a user defined.
@@ -3248,11 +3290,13 @@ on_replace_dd_collation(struct trigger * /* trigger */, void *event)
 		if (on_commit == NULL || on_rollback == NULL)
 			return -1;
 		/*
-		 * TODO: Check that no index uses the collation
-		 * identifier.
+		 * Check that no index or space format uses the
+		 * collation identifier.
 		 */
 		int32_t old_id = tuple_field_u32_xc(old_tuple,
 						    BOX_COLLATION_FIELD_ID);
+		if (space_foreach(space_check_coll_entry, &old_id) != 0)
+			return -1;
 		/*
 		 * Don't allow user to drop "none" collation
 		 * since it is very special and vastly used
diff --git a/test/box/net.box.result b/test/box/net.box.result
index e3dabf7d9..dcb7e03f4 100644
--- a/test/box/net.box.result
+++ b/test/box/net.box.result
@@ -2944,10 +2944,10 @@ end
 c:close()
 ---
 ...
-box.internal.collation.drop('test')
+space:drop()
 ---
 ...
-space:drop()
+box.internal.collation.drop('test')
 ---
 ...
 c.state
diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
index 8e65ff470..d4fe32e17 100644
--- a/test/box/net.box.test.lua
+++ b/test/box/net.box.test.lua
@@ -1209,8 +1209,8 @@ else                                             \
     return parts[1].collation_id == collation_id \
 end
 c:close()
-box.internal.collation.drop('test')
 space:drop()
+box.internal.collation.drop('test')
 c.state
 c = nil
 
diff --git a/test/sql/collation.result b/test/sql/collation.result
index 11962ef47..e77712636 100644
--- a/test/sql/collation.result
+++ b/test/sql/collation.result
@@ -222,7 +222,7 @@ box.space._collation:select{0}
 ...
 box.space._collation:delete{0}
 ---
-- error: 'Can''t drop collation none : system collation'
+- error: 'Can''t drop collation 0 : index 0 of space _schema has references to collation'
 ...
 -- gh-3185: collations of LHS and RHS must be compatible.
 --
@@ -1236,3 +1236,34 @@ s:select{}
 s:drop()
 ---
 ...
+-- gh-4544: make sure that collation can't be dropped until it
+-- is referenced by space or index.
+--
+box.internal.collation.create('c', 'ICU', 'unicode')
+---
+...
+coll_id = box.space._collation.index.name:get({'c'})[1]
+---
+...
+box.execute([[CREATE TABLE things (id STRING COLLATE "unicode" PRIMARY KEY, a STRING COLLATE "c");]])
+---
+- row_count: 1
+...
+box.space._collation:delete(1)
+---
+- error: 'Can''t drop collation 1 : index 0 of space THINGS has references to collation'
+...
+box.space._collation:delete(coll_id)
+---
+- error: 'Can''t drop collation 277 : format of space THINGS has references to collation'
+...
+box.internal.collation.drop('c')
+---
+- error: 'Can''t drop collation 277 : format of space THINGS has references to collation'
+...
+box.space.THINGS:drop()
+---
+...
+box.internal.collation.drop('c')
+---
+...
diff --git a/test/sql/collation.test.lua b/test/sql/collation.test.lua
index 1be28b3ff..70b0c9622 100644
--- a/test/sql/collation.test.lua
+++ b/test/sql/collation.test.lua
@@ -343,3 +343,14 @@ s:insert{'ё'}
 s:select{}
 s:drop()
 
+-- gh-4544: make sure that collation can't be dropped until it
+-- is referenced by space or index.
+--
+box.internal.collation.create('c', 'ICU', 'unicode')
+coll_id = box.space._collation.index.name:get({'c'})[1]
+box.execute([[CREATE TABLE things (id STRING COLLATE "unicode" PRIMARY KEY, a STRING COLLATE "c");]])
+box.space._collation:delete(1)
+box.space._collation:delete(coll_id)
+box.internal.collation.drop('c')
+box.space.THINGS:drop()
+box.internal.collation.drop('c')
-- 
2.15.1

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

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/11/11 21:27]:
> 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.

Shouldn't you be able to define json path index on 'collation'
option and use it for lookup instead?


-- 
Konstantin Osipov, Moscow, Russia

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

[Vladislav Shpilevoy]

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.

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

[Nikita Pettik]

On 11 Nov 23:38, Vladislav Shpilevoy wrote:
> 
> 3. Collations are referenced not only by indexes and spaces.

Oh, that's pretty sad. AFAIU there's no way to get list of all
existing key defs, so it seems that we have to use reference counters.
I'm going to rework patch and send v2.
 
> 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.

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

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/11/12 16:29]:
> On 11 Nov 23:38, Vladislav Shpilevoy wrote:
> > 
> > 3. Collations are referenced not only by indexes and spaces.
> 
> Oh, that's pretty sad. AFAIU there's no way to get list of all
> existing key defs, so it seems that we have to use reference counters.
> I'm going to rework patch and send v2.

This is not going to work reliably - reference counting code tends
to rot and leak references over time.

Basically, if you forget to ref or deref in any place, you will
not see it in a test or in a real life.

While this piece was ugly but contained in one line, the
refcounting will be both ugly and spread all over.

Please either consider having an index on collation, so that all
references can be checked quickly using a data dictionary, or drop
this issue altogether - only a superuser can drop a collation, and
this is never used in practice, so not worth fixing, really.


-- 
Konstantin Osipov, Moscow, Russia