[tarantool-patches] [PATCH v2 1/5] alter: fix assertion in collations alter

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Forbid collations update - it leads to an assertion in collation
cache about unavailability to correctly replace collations.
Refactor collations deletion and insertion.

Besides, collation update potentially changes indexing order with
no actual indexes rebuilding.
---
 src/box/alter.cc      | 115 ++++++++++++++++++++++++++------------------------
 test/box/ddl.result   |  10 +++++
 test/box/ddl.test.lua |   3 ++
 3 files changed, 74 insertions(+), 54 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 77b03e680..be2ccc061 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -2376,19 +2376,31 @@ coll_cache_rollback(struct trigger *trigger, void *event)
 	struct txn_stmt *stmt = txn_last_stmt((struct txn*) event);
 	struct tuple *new_tuple = stmt->new_tuple;
 
-	if (new_tuple != NULL) {
+	if (new_tuple == NULL && old_coll != NULL) {
+		/* Rollback DELETE. */
+		if (coll_by_id(old_coll->id) == old_coll) {
+			/*
+			 * Nothing to do - the tx had been failed
+			 * before old collation deletion.
+			 */
+			return;
+		}
+		struct coll *replaced;
+		if (coll_cache_replace(old_coll, &replaced) != 0) {
+			panic("Out of memory on insertion into collation "\
+			      "cache");
+		}
+		assert(replaced == NULL);
+	} else {
+		assert(new_tuple != NULL && old_coll == NULL);
 		uint32_t new_id = tuple_field_u32_xc(new_tuple,
 						     BOX_COLLATION_FIELD_ID);
 		struct coll *new_coll = coll_by_id(new_id);
-		coll_cache_delete(new_coll);
-		coll_delete(new_coll);
-	}
-
-	if (old_coll != NULL) {
-		struct coll *replaced;
-		int rc = coll_cache_replace(old_coll, &replaced);
-		assert(rc == 0 && replaced == NULL);
-		(void)rc;
+		/* Can be NULL, if failed before cache update. */
+		if (new_coll != NULL) {
+			coll_cache_delete(new_coll);
+			coll_delete(new_coll);
+		}
 	}
 }
 
@@ -2412,59 +2424,54 @@ on_replace_dd_collation(struct trigger * /* trigger */, void *event)
 	struct tuple *old_tuple = stmt->old_tuple;
 	struct tuple *new_tuple = stmt->new_tuple;
 	txn_check_singlestatement_xc(txn, "Space _collation");
-	struct coll *old_coll = NULL;
-	if (old_tuple != NULL) {
+	if (new_tuple == NULL && old_tuple != NULL) {
+		/* DELETE */
 		/* TODO: Check that no index uses the collation */
-		uint32_t old_id = tuple_field_u32_xc(old_tuple,
-						     BOX_COLLATION_FIELD_ID);
-		old_coll = coll_by_id(old_id);
+		int32_t old_id = tuple_field_u32_xc(old_tuple,
+						    BOX_COLLATION_FIELD_ID);
+		struct coll *old_coll = coll_by_id(old_id);
 		assert(old_coll != NULL);
 		access_check_ddl(old_coll->name, old_coll->owner_id,
-				 SC_COLLATION,
-				 new_tuple == NULL ? PRIV_D: PRIV_A,
-				 false);
-
-		struct trigger *on_commit =
-			txn_alter_trigger_new(coll_cache_delete_coll, old_coll);
-		txn_on_commit(txn, on_commit);
-	}
-
-	if (new_tuple == NULL) {
-		/* Simple DELETE */
-		assert(old_tuple != NULL);
-		coll_cache_delete(old_coll);
-
+				 SC_COLLATION, PRIV_D, false);
+		/*
+		 * Set on_rollback before deletion from the cache.
+		 * Else rollback setting can fail and the
+		 * collation will be lost.
+		 */
 		struct trigger *on_rollback =
 			txn_alter_trigger_new(coll_cache_rollback, old_coll);
 		txn_on_rollback(txn, on_rollback);
-		return;
-	}
-
-	struct coll_def new_def;
-	coll_def_new_from_tuple(new_tuple, &new_def);
-	access_check_ddl(new_def.name, new_def.owner_id, SC_COLLATION,
-			 old_tuple == NULL ? PRIV_C : PRIV_A, false);
-	struct coll *new_coll = coll_new(&new_def);
-	if (new_coll == NULL)
-		diag_raise();
-	auto def_guard = make_scoped_guard([=] { coll_delete(new_coll); });
-
-	struct coll *replaced;
-	if (coll_cache_replace(new_coll, &replaced) != 0)
-		diag_raise();
-	if (replaced == NULL && old_coll != NULL) {
+		coll_cache_delete(old_coll);
+		struct trigger *on_commit =
+			txn_alter_trigger_new(coll_cache_delete_coll, old_coll);
+		txn_on_commit(txn, on_commit);
+	} else if (new_tuple != NULL && old_tuple == NULL) {
+		/* INSERT */
+		struct coll_def new_def;
+		coll_def_new_from_tuple(new_tuple, &new_def);
+		access_check_ddl(new_def.name, new_def.owner_id, SC_COLLATION,
+				 PRIV_C, false);
 		/*
-		 * ID of a collation was changed.
-		 * Remove collation by old ID.
+		 * Set on_rollback before the collation is
+		 * inserted into the cache. Else if the trigger
+		 * creation fails the collation will not be
+		 * deleted on abort.
 		 */
-		coll_cache_delete(old_coll);
+		struct trigger *on_rollback =
+			txn_alter_trigger_new(coll_cache_rollback, NULL);
+		txn_on_rollback(txn, on_rollback);
+		struct coll *new_coll = coll_new(&new_def);
+		if (new_coll == NULL)
+			diag_raise();
+		struct coll *replaced;
+		if (coll_cache_replace(new_coll, &replaced) != 0)
+			diag_raise();
+		assert(replaced == NULL);
+	} else {
+		/* UPDATE */
+		assert(new_tuple != NULL && old_tuple != NULL);
+		tnt_raise(ClientError, ER_UNSUPPORTED, "collation", "alter");
 	}
-
-	struct trigger *on_rollback =
-		txn_alter_trigger_new(coll_cache_rollback, old_coll);
-	txn_on_rollback(txn, on_rollback);
-
-	def_guard.is_active = false;
 }
 
 /**
diff --git a/test/box/ddl.result b/test/box/ddl.result
index 0eef37992..87b9581c6 100644
--- a/test/box/ddl.result
+++ b/test/box/ddl.result
@@ -289,6 +289,16 @@ box.internal.collation.create('test', 'ICU', 'ru_RU', {strength='primary'}) --ok
 box.internal.collation.drop('test') --ok
 ---
 ...
+c = box.space._collation:get{1}:totable()
+---
+...
+c[2] = 'unicode_test'
+---
+...
+box.space._collation:replace(c)
+---
+- error: collation does not support alter
+...
 box.begin() box.internal.collation.create('test2', 'ICU', 'ru_RU')
 ---
 - error: Space _collation does not support multi-statement transactions
diff --git a/test/box/ddl.test.lua b/test/box/ddl.test.lua
index 820fe7d4d..a1502ae13 100644
--- a/test/box/ddl.test.lua
+++ b/test/box/ddl.test.lua
@@ -127,6 +127,9 @@ box.internal.collation.create('test', 'ICU', 'ru_RU', {strength=2}) --ok
 box.internal.collation.drop('test') --ok
 box.internal.collation.create('test', 'ICU', 'ru_RU', {strength='primary'}) --ok
 box.internal.collation.drop('test') --ok
+c = box.space._collation:get{1}:totable()
+c[2] = 'unicode_test'
+box.space._collation:replace(c)
 
 box.begin() box.internal.collation.create('test2', 'ICU', 'ru_RU')
 box.rollback()
-- 
2.15.1 (Apple Git-101)