Tarantool development patches archive
 help / color / mirror / Atom feed
From: Nikita Pettik <korablev@tarantool.org>
To: tarantool-patches@dev.tarantool.org
Cc: v.shpilevoy@tarantool.org
Subject: [Tarantool-patches] [PATCH] box: don't allow referenced collation to be dropped
Date: Mon, 11 Nov 2019 16:52:46 +0300	[thread overview]
Message-ID: <20191111135246.79818-1-korablev@tarantool.org> (raw)

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

             reply	other threads:[~2019-11-11 13:52 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-11 13:52 Nikita Pettik [this message]
2019-11-11 21:50 ` Konstantin Osipov
2019-11-11 22:38 ` Vladislav Shpilevoy
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=20191111135246.79818-1-korablev@tarantool.org \
    --to=korablev@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --cc=v.shpilevoy@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