[Tarantool-patches] [PATCH v5 0/8] refactoring: remove exceptions from triggers follow-ups

[Tarantool-patches] [PATCH v5 0/8] refactoring: remove exceptions from triggers follow-ups

[Ilya Kosarev]

This patchset is a pack of follow-ups to finish clearing triggers from
exceptions. It is based on review comments for the patches of the
original "refactoring: remove exceptions from triggers" patchset.

Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-4247-remove-exceptions-from-triggers 
Issue: https://github.com/tarantool/tarantool/issues/4247

Ilya Kosarev (8):
  refactoring: wrap new operator calls in triggers
  refactoring: specify struct name in allocation diagnostics
  refactoring: recombine error conditions in triggers
  refactoring: set diagnostics if sequence_by_id fails
  refactoring: clear triggers from fresh exceptions
  refactoring: update comment for index_def_check_tuple
  refactoring: remove redundant line in txn_alter_trigger_new
  refactoring: remove try..catch wrapper around trigger->run

 src/box/alter.cc        | 192 +++++++++++++++++++++++++---------------
 src/box/user.cc         |   2 +-
 src/lib/core/trigger.cc |  25 ++----
 src/lua/trigger.c       |   2 +-
 4 files changed, 133 insertions(+), 88 deletions(-)

-- 
2.17.1

[Tarantool-patches] [PATCH v5 1/8] refactoring: wrap new operator calls in triggers

[Ilya Kosarev]

std operator new might throw so we need to wrap it in triggers to
provide non-throwing triggers. It also means alter_space_move_indexes
returns an error code now. It's usages are updated.

Part of #4247
---
 src/box/alter.cc | 135 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 95 insertions(+), 40 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 4a3241a79..022c7dfa2 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1814,7 +1814,7 @@ on_create_space_rollback(struct trigger *trigger, void *event)
  * Create MoveIndex operation for a range of indexes in a space
  * for range [begin, end)
  */
-void
+int
 alter_space_move_indexes(struct alter_space *alter, uint32_t begin,
 			 uint32_t end)
 {
@@ -1839,9 +1839,17 @@ alter_space_move_indexes(struct alter_space *alter, uint32_t begin,
 				new_def = index_def_dup(old_def);
 				index_def_update_optionality(new_def,
 							     min_field_count);
-				(void) new ModifyIndex(alter, old_index, new_def);
+				try {
+					(void) new ModifyIndex(alter, old_index, new_def);
+				} catch (Exception *e) {
+					return -1;
+				}
 			} else {
-				(void) new MoveIndex(alter, old_def->iid);
+				try {
+					(void) new MoveIndex(alter, old_def->iid);
+				} catch (Exception *e) {
+					return -1;
+				}
 			}
 			continue;
 		}
@@ -1856,11 +1864,20 @@ alter_space_move_indexes(struct alter_space *alter, uint32_t begin,
 		index_def_update_optionality(new_def, min_field_count);
 		auto guard = make_scoped_guard([=] { index_def_delete(new_def); });
 		if (!index_def_change_requires_rebuild(old_index, new_def))
-			(void) new ModifyIndex(alter, old_index, new_def);
+			try {
+				(void) new ModifyIndex(alter, old_index, new_def);
+			} catch (Exception *e) {
+				return -1;
+			}
 		else
-			(void) new RebuildIndex(alter, new_def, old_def);
+			try {
+				(void) new RebuildIndex(alter, new_def, old_def);
+			} catch (Exception *e) {
+				return -1;
+			}
 		guard.is_active = false;
 	}
+	return 0;
 }
 
 /**
@@ -2301,15 +2318,21 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
 						     old_space->index_count,
 						     def->fields,
 						     def->field_count);
-		(void) new CheckSpaceFormat(alter);
-		(void) new ModifySpace(alter, def);
-		(void) new RebuildCkConstraints(alter);
+		try {
+			(void) new CheckSpaceFormat(alter);
+			(void) new ModifySpace(alter, def);
+			(void) new RebuildCkConstraints(alter);
+		} catch (Exception *e) {
+			return -1;
+		}
 		def_guard.is_active = false;
 		/* Create MoveIndex ops for all space indexes. */
-		alter_space_move_indexes(alter, 0, old_space->index_id_max + 1);
-		/* Remember to update schema_version. */
-		(void) new UpdateSchemaVersion(alter);
+		if (alter_space_move_indexes(alter, 0,
+		    old_space->index_id_max + 1) != 0)
+			return -1;
 		try {
+			/* Remember to update schema_version. */
+			(void) new UpdateSchemaVersion(alter);
 			alter_space_do(stmt, alter);
 		} catch (Exception *e) {
 			return -1;
@@ -2476,13 +2499,24 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
 				  "can not drop a referenced index");
 			return -1;
 		}
-		alter_space_move_indexes(alter, 0, iid);
-		(void) new DropIndex(alter, old_index);
+		if (alter_space_move_indexes(alter, 0, iid) != 0)
+			return -1;
+		try {
+			(void) new DropIndex(alter, old_index);
+		} catch (Exception *e) {
+			return -1;
+		}
 	}
 	/* Case 2: create an index, if it is simply created. */
 	if (old_index == NULL && new_tuple != NULL) {
-		alter_space_move_indexes(alter, 0, iid);
-		CreateIndex *create_index = new CreateIndex(alter);
+		if (alter_space_move_indexes(alter, 0, iid))
+			return -1;
+		CreateIndex *create_index;
+		try {
+			create_index = new CreateIndex(alter);
+		} catch (Exception *e) {
+			return -1;
+		}
 		create_index->new_index_def =
 			index_def_new_from_tuple(new_tuple, old_space);
 		if (create_index->new_index_def == NULL)
@@ -2536,10 +2570,16 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
 						     def->field_count);
 		index_def_update_optionality(index_def,
 					     alter->new_min_field_count);
-		alter_space_move_indexes(alter, 0, iid);
+		if (alter_space_move_indexes(alter, 0, iid))
+			return -1;
 		if (index_def_cmp(index_def, old_index->def) == 0) {
 			/* Index is not changed so just move it. */
-			(void) new MoveIndex(alter, old_index->def->iid);
+			try {
+				(void) new MoveIndex(alter, old_index->def->iid);
+			} catch (Exception *e) {
+				return -1;
+			}
+
 		} else if (index_def_change_requires_rebuild(old_index,
 							     index_def)) {
 			if (index_is_used_by_fk_constraint(&old_space->parent_fk_constraint,
@@ -2552,8 +2592,12 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
 			/*
 			 * Operation demands an index rebuild.
 			 */
-			(void) new RebuildIndex(alter, index_def,
-						old_index->def);
+			try {
+				(void) new RebuildIndex(alter, index_def,
+							old_index->def);
+			} catch (Exception *e) {
+				return -1;
+			}
 			index_def_guard.is_active = false;
 		} else {
 			/*
@@ -2561,8 +2605,12 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
 			 * but we still need to check that tuples stored
 			 * in the space conform to the new format.
 			 */
-			(void) new CheckSpaceFormat(alter);
-			(void) new ModifyIndex(alter, old_index, index_def);
+			try {
+				(void) new CheckSpaceFormat(alter);
+				(void) new ModifyIndex(alter, old_index, index_def);
+			} catch (Exception *e) {
+				return -1;
+			}
 			index_def_guard.is_active = false;
 		}
 	}
@@ -2570,11 +2618,12 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
 	 * Create MoveIndex ops for the remaining indexes in the
 	 * old space.
 	 */
-	alter_space_move_indexes(alter, iid + 1, old_space->index_id_max + 1);
-	(void) new MoveCkConstraints(alter);
-	/* Add an op to update schema_version on commit. */
-	(void) new UpdateSchemaVersion(alter);
+	if (alter_space_move_indexes(alter, iid + 1, old_space->index_id_max + 1) != 0)
+		return -1;
 	try {
+		(void) new MoveCkConstraints(alter);
+		/* Add an op to update schema_version on commit. */
+		(void) new UpdateSchemaVersion(alter);
 		alter_space_do(stmt, alter);
 	} catch (Exception *e) {
 		return -1;
@@ -2654,16 +2703,16 @@ on_replace_dd_truncate(struct trigger * /* trigger */, void *event)
 		stmt->row->group_id = GROUP_LOCAL;
 	}
 
-	/*
-	 * Recreate all indexes of the truncated space.
-	 */
-	for (uint32_t i = 0; i < old_space->index_count; i++) {
-		struct index *old_index = old_space->index[i];
-		(void) new TruncateIndex(alter, old_index->def->iid);
-	}
-
-	(void) new MoveCkConstraints(alter);
 	try {
+		/*
+		 * Recreate all indexes of the truncated space.
+		 */
+		for (uint32_t i = 0; i < old_space->index_count; i++) {
+			struct index *old_index = old_space->index[i];
+			(void) new TruncateIndex(alter, old_index->def->iid);
+		}
+
+		(void) new MoveCkConstraints(alter);
 		alter_space_do(stmt, alter);
 	} catch (Exception *e) {
 		return -1;
@@ -5562,13 +5611,19 @@ on_replace_dd_func_index(struct trigger *trigger, void *event)
 	if (alter == NULL)
 		return -1;
 	auto scoped_guard = make_scoped_guard([=] {alter_space_delete(alter);});
-	alter_space_move_indexes(alter, 0, index->def->iid);
-	(void) new RebuildFuncIndex(alter, index->def, func);
-	alter_space_move_indexes(alter, index->def->iid + 1,
-				 space->index_id_max + 1);
-	(void) new MoveCkConstraints(alter);
-	(void) new UpdateSchemaVersion(alter);
+	if (alter_space_move_indexes(alter, 0, index->def->iid) != 0)
+		return -1;
 	try {
+		(void) new RebuildFuncIndex(alter, index->def, func);
+	} catch (Exception *e) {
+		return -1;
+	}
+	if (alter_space_move_indexes(alter, index->def->iid + 1,
+				 space->index_id_max + 1) != 0)
+		return -1;
+	try {
+		(void) new MoveCkConstraints(alter);
+		(void) new UpdateSchemaVersion(alter);
 		alter_space_do(stmt, alter);
 	} catch (Exception *e) {
 		return -1;
-- 
2.17.1

[Tarantool-patches] [PATCH v5 2/8] refactoring: specify struct name in allocation diagnostics

[Ilya Kosarev]

In case of allocation problems in region alloc we were setting
diagnostics using "new slab" stub. Now we specify concrete struct name
which was going to be allocated.

Part of #4247
---
 src/box/alter.cc | 8 ++++----
 src/box/user.cc  | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 022c7dfa2..16e3f7d37 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -540,7 +540,7 @@ space_format_decode(const char *data, uint32_t *out_count,
 	struct field_def *region_defs =
 		(struct field_def *) region_alloc(region, size);
 	if (region_defs == NULL) {
-		diag_set(OutOfMemory, size, "region", "new slab");
+		diag_set(OutOfMemory, size, "region", "struct field_def");
 		return -1;
 	}
 	/*
@@ -810,7 +810,7 @@ txn_alter_trigger_new(trigger_f run, void *data)
 		region_aligned_alloc(&in_txn()->region, size,
 				     alignof(struct trigger));
 	if (trigger == NULL) {
-		diag_set(OutOfMemory, size, "region", "new slab");
+		diag_set(OutOfMemory, size, "region", "struct trigger");
 		return NULL;
 	}
 	trigger = (struct trigger *)memset(trigger, 0, size);
@@ -861,7 +861,7 @@ alter_space_new(struct space *old_space)
 		region_aligned_alloc(&in_txn()->region, size,
 				     alignof(struct alter_space));
 	if (alter == NULL) {
-		diag_set(OutOfMemory, size, "region", "new slab");
+		diag_set(OutOfMemory, size, "region", "struct alter_space");
 		return NULL;
 	}
 	alter = (struct alter_space *)memset(alter, 0, size);
@@ -4824,7 +4824,7 @@ decode_fk_links(struct tuple *tuple, uint32_t *out_count,
 	struct field_link *region_links =
 		(struct field_link *)region_alloc(&fiber()->gc, size);
 	if (region_links == NULL) {
-		diag_set(OutOfMemory, size, "region", "new slab");
+		diag_set(OutOfMemory, size, "region", "struct field_link");
 		return NULL;
 	}
 	memset(region_links, 0, size);
diff --git a/src/box/user.cc b/src/box/user.cc
index 39520f231..a012cb196 100644
--- a/src/box/user.cc
+++ b/src/box/user.cc
@@ -196,7 +196,7 @@ user_grant_priv(struct user *user, struct priv_def *def)
 		old = (struct priv_def *)
 			region_alloc(&user->pool, size);
 		if (old == NULL) {
-			diag_set(OutOfMemory, size, "region", "new slab");
+			diag_set(OutOfMemory, size, "region", "struct priv_def");
 			return -1;
 		}
 		*old = *def;
-- 
2.17.1

[Tarantool-patches] [PATCH v5 3/8] refactoring: recombine error conditions in triggers

[Ilya Kosarev]

Some error conditions in triggers and underlying functions were
combined to look better. On the other hand, in
on_replace_dd_fk_constraint we now return an error immediately if
child space were not found instead of searching for both child and
parent spaces before search results inspection.

Part of #4247
---
 src/box/alter.cc | 35 +++++++++++++++--------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 16e3f7d37..aaaf53493 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -3621,9 +3621,8 @@ on_replace_dd_collation(struct trigger * /* trigger */, void *event)
 int
 priv_def_create_from_tuple(struct priv_def *priv, struct tuple *tuple)
 {
-	if (tuple_field_u32(tuple, BOX_PRIV_FIELD_ID, &(priv->grantor_id)) != 0)
-		return -1;
-	if (tuple_field_u32(tuple, BOX_PRIV_FIELD_UID, &(priv->grantee_id)) != 0)
+	if (tuple_field_u32(tuple, BOX_PRIV_FIELD_ID, &(priv->grantor_id)) != 0 ||
+	    tuple_field_u32(tuple, BOX_PRIV_FIELD_UID, &(priv->grantee_id)) != 0)
 		return -1;
 
 	const char *object_type =
@@ -3882,9 +3881,8 @@ modify_priv(struct trigger *trigger, void *event)
 	(void) event;
 	struct tuple *tuple = (struct tuple *)trigger->data;
 	struct priv_def priv;
-	if (priv_def_create_from_tuple(&priv, tuple) != 0)
-		return -1;
-	if (grant_or_revoke(&priv) != 0)
+	if (priv_def_create_from_tuple(&priv, tuple) != 0 ||
+	    grant_or_revoke(&priv) != 0)
 		return -1;
 	return 0;
 }
@@ -3903,11 +3901,9 @@ on_replace_dd_priv(struct trigger * /* trigger */, void *event)
 	struct priv_def priv;
 
 	if (new_tuple != NULL && old_tuple == NULL) {	/* grant */
-		if (priv_def_create_from_tuple(&priv, new_tuple) != 0)
-			return -1;
-		if (priv_def_check(&priv, PRIV_GRANT) != 0)
-			return -1;
-		if (grant_or_revoke(&priv) != 0)
+		if (priv_def_create_from_tuple(&priv, new_tuple) != 0 ||
+		    priv_def_check(&priv, PRIV_GRANT) != 0 ||
+		    grant_or_revoke(&priv) != 0)
 			return -1;
 		struct trigger *on_rollback =
 			txn_alter_trigger_new(revoke_priv, new_tuple);
@@ -3916,9 +3912,8 @@ on_replace_dd_priv(struct trigger * /* trigger */, void *event)
 		txn_stmt_on_rollback(stmt, on_rollback);
 	} else if (new_tuple == NULL) {                /* revoke */
 		assert(old_tuple);
-		if (priv_def_create_from_tuple(&priv, old_tuple) != 0)
-			return -1;
-		if (priv_def_check(&priv, PRIV_REVOKE) != 0)
+		if (priv_def_create_from_tuple(&priv, old_tuple) != 0 ||
+		    priv_def_check(&priv, PRIV_REVOKE) != 0)
 			return -1;
 		priv.access = 0;
 		if (grant_or_revoke(&priv) != 0)
@@ -3929,11 +3924,9 @@ on_replace_dd_priv(struct trigger * /* trigger */, void *event)
 			return -1;
 		txn_stmt_on_rollback(stmt, on_rollback);
 	} else {                                       /* modify */
-		if (priv_def_create_from_tuple(&priv, new_tuple) != 0)
-			return -1;
-		if (priv_def_check(&priv, PRIV_GRANT) != 0)
-			return -1;
-		if (grant_or_revoke(&priv) != 0)
+		if (priv_def_create_from_tuple(&priv, new_tuple) != 0 ||
+		    priv_def_check(&priv, PRIV_GRANT) != 0 ||
+		    grant_or_revoke(&priv) != 0)
 			return -1;
 		struct trigger *on_rollback =
 			txn_alter_trigger_new(modify_priv, old_tuple);
@@ -5264,8 +5257,10 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
 			return -1;
 		auto fk_def_guard = make_scoped_guard([=] { free(fk_def); });
 		struct space *child_space = space_cache_find(fk_def->child_id);
+		if (child_space == NULL)
+			return -1;
 		struct space *parent_space = space_cache_find(fk_def->parent_id);
-		if (child_space == NULL or parent_space == NULL)
+		if (parent_space == NULL)
 			return -1;
 		struct fk_constraint *old_fk=
 			fk_constraint_remove(&child_space->child_fk_constraint,
-- 
2.17.1

[Tarantool-patches] [PATCH v5 4/8] refactoring: set diagnostics if sequence_by_id fails

[Ilya Kosarev]

In refactoring: use non _xc version of functions in triggers
(b75d5f85338158eb1734ca8832d3c4b8ac0b4086) sequence_cache_find was
replaced by sequence_by_id. It led to the loss of diagnostics in case
of sequence_by_id failure. Now it is fixed.

Part of #4247
---
 src/box/alter.cc | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index aaaf53493..2628e1401 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -4365,8 +4365,10 @@ on_replace_dd_sequence_data(struct trigger * /* trigger */, void *event)
 			    &id) != 0)
 		return -1;
 	struct sequence *seq = sequence_by_id(id);
-	if (seq == NULL)
+	if (seq == NULL) {
+		diag_set(ClientError, ER_NO_SUCH_SEQUENCE, int2str(id));
 		return -1;
+	}
 	if (new_tuple != NULL) {			/* INSERT, UPDATE */
 		int64_t value;
 		if (tuple_field_i64(new_tuple, BOX_SEQUENCE_DATA_FIELD_VALUE,
@@ -4520,8 +4522,10 @@ on_replace_dd_space_sequence(struct trigger * /* trigger */, void *event)
 	if (space == NULL)
 		return -1;
 	struct sequence *seq = sequence_by_id(sequence_id);
-	if (seq == NULL)
+	if (seq == NULL) {
+		diag_set(ClientError, ER_NO_SUCH_SEQUENCE, int2str(sequence_id));
 		return -1;
+	}
 
 	enum priv_type priv_type = stmt->new_tuple ? PRIV_C : PRIV_D;
 	if (stmt->new_tuple && stmt->old_tuple)
-- 
2.17.1

[Tarantool-patches] [PATCH v5 5/8] refactoring: clear triggers from fresh exceptions

[Ilya Kosarev]

Clear triggers from freshly occured exceptions. Trivial replacements:
`diag_raise` by `return -1`, _xc function by it's non _xc version.

Part of #4247
---
 src/box/alter.cc  | 3 ++-
 src/lua/trigger.c | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 2628e1401..4b3bbca31 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -5468,7 +5468,8 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
 			if (old_def->language == ck_def->language &&
 			    strcmp(old_def->expr_str, ck_def->expr_str) == 0) {
 				old_def->is_enabled = ck_def->is_enabled;
-				trigger_run_xc(&on_alter_space, space);
+				if (trigger_run(&on_alter_space, space) != 0)
+					return -1;
 				return 0;
 			}
 		}
diff --git a/src/lua/trigger.c b/src/lua/trigger.c
index b1473bcc1..d588fc12c 100644
--- a/src/lua/trigger.c
+++ b/src/lua/trigger.c
@@ -79,7 +79,7 @@ lbox_trigger_run(struct trigger *ptr, void *event)
 	if (fiber()->storage.lua.stack == NULL) {
 		L = luaT_newthread(tarantool_L);
 		if (L == NULL)
-			diag_raise();
+			return -1;
 		coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
 	} else {
 		L = fiber()->storage.lua.stack;
-- 
2.17.1

[Tarantool-patches] [PATCH v5 6/8] refactoring: update comment for index_def_check_tuple

[Ilya Kosarev]

Originally index_def_check_tuple comment said that it throws a nice
error. Since refactoring: remove exceptions from
index_def_new_from_tuple (90ac0037f6cae587d8ca589dbe45c58e67f05657)
it returns an error. Now it is clearly specified in the comment.

Part of #4247
---
 src/box/alter.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 4b3bbca31..684b93798 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -159,7 +159,7 @@ index_def_check_sequence(struct index_def *index_def, uint32_t sequence_fieldno,
 
 /**
  * Support function for index_def_new_from_tuple(..)
- * Checks tuple (of _index space) and sets a nice diag if it is invalid
+ * Checks tuple (of _index space) and returns an error if it is invalid
  * Checks only types of fields and their count!
  */
 static int
-- 
2.17.1

[Tarantool-patches] [PATCH v5 7/8] refactoring: remove redundant line in txn_alter_trigger_new

[Ilya Kosarev]

Since refactoring: clear privilege managing triggers from exceptions
(977fca292ee97e82c6729d4b62a178ee8835af59) we are doing zero memset for
trigger struct in txn_alter_trigger_new. This means we don't any more
need to set any field of this struct to NULL explicitly.

Part of #4247
---
 src/box/alter.cc | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index 684b93798..a3a9a5ff9 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -816,7 +816,6 @@ txn_alter_trigger_new(trigger_f run, void *data)
 	trigger = (struct trigger *)memset(trigger, 0, size);
 	trigger->run = run;
 	trigger->data = data;
-	trigger->destroy = NULL;
 	return trigger;
 }
 
-- 
2.17.1

[Tarantool-patches] [PATCH v5 8/8] refactoring: remove try..catch wrapper around trigger->run

[Ilya Kosarev]

Triggers don't throw exceptions any more. Now they have
return codes to report errors.

Closes #4247
---
 src/lib/core/trigger.cc | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/src/lib/core/trigger.cc b/src/lib/core/trigger.cc
index 8f6a83fb5..7fe112f8b 100644
--- a/src/lib/core/trigger.cc
+++ b/src/lib/core/trigger.cc
@@ -30,32 +30,23 @@
  */
 
 #include "trigger.h"
-#include "exception.h"
 
 int
 trigger_run(struct rlist *list, void *event)
 {
-	try {
-		struct trigger *trigger, *tmp;
-		rlist_foreach_entry_safe(trigger, list, link, tmp)
-			if (trigger->run(trigger, event) != 0)
-				return -1;
-	} catch (Exception *e) {
-		return -1;
-	}
+	struct trigger *trigger, *tmp;
+	rlist_foreach_entry_safe(trigger, list, link, tmp)
+		if (trigger->run(trigger, event) != 0)
+			return -1;
 	return 0;
 }
 
 int
 trigger_run_reverse(struct rlist *list, void *event)
 {
-	try {
-		struct trigger *trigger, *tmp;
-		rlist_foreach_entry_safe_reverse(trigger, list, link, tmp)
-			if (trigger->run(trigger, event) != 0)
-				return -1;
-	} catch (Exception *e) {
-		return -1;
-	}
+	struct trigger *trigger, *tmp;
+	rlist_foreach_entry_safe_reverse(trigger, list, link, tmp)
+		if (trigger->run(trigger, event) != 0)
+			return -1;
 	return 0;
 }
-- 
2.17.1

Re: [Tarantool-patches] [PATCH v5 1/8] refactoring: wrap new operator calls in triggers

[Sergey Ostanevich]

Hi!

One nit below, otherwise LGTM.

Sergos


On 22 Nov 05:46, Ilya Kosarev wrote:

> @@ -5562,13 +5611,19 @@ on_replace_dd_func_index(struct trigger *trigger, void *event)
>  	if (alter == NULL)
>  		return -1;
>  	auto scoped_guard = make_scoped_guard([=] {alter_space_delete(alter);});
> -	alter_space_move_indexes(alter, 0, index->def->iid);
> -	(void) new RebuildFuncIndex(alter, index->def, func);
> -	alter_space_move_indexes(alter, index->def->iid + 1,
> -				 space->index_id_max + 1);
> -	(void) new MoveCkConstraints(alter);
> -	(void) new UpdateSchemaVersion(alter);
> +	if (alter_space_move_indexes(alter, 0, index->def->iid) != 0)
> +		return -1;
>  	try {
> +		(void) new RebuildFuncIndex(alter, index->def, func);
> +	} catch (Exception *e) {
> +		return -1;
> +	}
> +	if (alter_space_move_indexes(alter, index->def->iid + 1,
> +				 space->index_id_max + 1) != 0)

indentation is broken for the line above - need 4 extra spaces.

> +		return -1;
> +	try {
> +		(void) new MoveCkConstraints(alter);
> +		(void) new UpdateSchemaVersion(alter);
>  		alter_space_do(stmt, alter);
>  	} catch (Exception *e) {
>  		return -1;
> -- 
> 2.17.1
> 

Re: [Tarantool-patches] [PATCH v5 2/8] refactoring: specify struct name in allocation diagnostics

[Sergey Ostanevich]

Hi!

LGTM.

Sergos


On 22 Nov 05:46, Ilya Kosarev wrote:
> In case of allocation problems in region alloc we were setting
> diagnostics using "new slab" stub. Now we specify concrete struct name
> which was going to be allocated.
> 
> Part of #4247
> ---
>  src/box/alter.cc | 8 ++++----
>  src/box/user.cc  | 2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 022c7dfa2..16e3f7d37 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -540,7 +540,7 @@ space_format_decode(const char *data, uint32_t *out_count,
>  	struct field_def *region_defs =
>  		(struct field_def *) region_alloc(region, size);
>  	if (region_defs == NULL) {
> -		diag_set(OutOfMemory, size, "region", "new slab");
> +		diag_set(OutOfMemory, size, "region", "struct field_def");
>  		return -1;
>  	}
>  	/*
> @@ -810,7 +810,7 @@ txn_alter_trigger_new(trigger_f run, void *data)
>  		region_aligned_alloc(&in_txn()->region, size,
>  				     alignof(struct trigger));
>  	if (trigger == NULL) {
> -		diag_set(OutOfMemory, size, "region", "new slab");
> +		diag_set(OutOfMemory, size, "region", "struct trigger");
>  		return NULL;
>  	}
>  	trigger = (struct trigger *)memset(trigger, 0, size);
> @@ -861,7 +861,7 @@ alter_space_new(struct space *old_space)
>  		region_aligned_alloc(&in_txn()->region, size,
>  				     alignof(struct alter_space));
>  	if (alter == NULL) {
> -		diag_set(OutOfMemory, size, "region", "new slab");
> +		diag_set(OutOfMemory, size, "region", "struct alter_space");
>  		return NULL;
>  	}
>  	alter = (struct alter_space *)memset(alter, 0, size);
> @@ -4824,7 +4824,7 @@ decode_fk_links(struct tuple *tuple, uint32_t *out_count,
>  	struct field_link *region_links =
>  		(struct field_link *)region_alloc(&fiber()->gc, size);
>  	if (region_links == NULL) {
> -		diag_set(OutOfMemory, size, "region", "new slab");
> +		diag_set(OutOfMemory, size, "region", "struct field_link");
>  		return NULL;
>  	}
>  	memset(region_links, 0, size);
> diff --git a/src/box/user.cc b/src/box/user.cc
> index 39520f231..a012cb196 100644
> --- a/src/box/user.cc
> +++ b/src/box/user.cc
> @@ -196,7 +196,7 @@ user_grant_priv(struct user *user, struct priv_def *def)
>  		old = (struct priv_def *)
>  			region_alloc(&user->pool, size);
>  		if (old == NULL) {
> -			diag_set(OutOfMemory, size, "region", "new slab");
> +			diag_set(OutOfMemory, size, "region", "struct priv_def");
>  			return -1;
>  		}
>  		*old = *def;
> -- 
> 2.17.1
> 

Re: [Tarantool-patches] [PATCH v5 3/8] refactoring: recombine error conditions in triggers

[Sergey Ostanevich]

Hi!

LGTM

Sergos


On 22 Nov 05:46, Ilya Kosarev wrote:
> Some error conditions in triggers and underlying functions were
> combined to look better. On the other hand, in
> on_replace_dd_fk_constraint we now return an error immediately if
> child space were not found instead of searching for both child and
> parent spaces before search results inspection.
> 
> Part of #4247
> ---
>  src/box/alter.cc | 35 +++++++++++++++--------------------
>  1 file changed, 15 insertions(+), 20 deletions(-)
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 16e3f7d37..aaaf53493 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -3621,9 +3621,8 @@ on_replace_dd_collation(struct trigger * /* trigger */, void *event)
>  int
>  priv_def_create_from_tuple(struct priv_def *priv, struct tuple *tuple)
>  {
> -	if (tuple_field_u32(tuple, BOX_PRIV_FIELD_ID, &(priv->grantor_id)) != 0)
> -		return -1;
> -	if (tuple_field_u32(tuple, BOX_PRIV_FIELD_UID, &(priv->grantee_id)) != 0)
> +	if (tuple_field_u32(tuple, BOX_PRIV_FIELD_ID, &(priv->grantor_id)) != 0 ||
> +	    tuple_field_u32(tuple, BOX_PRIV_FIELD_UID, &(priv->grantee_id)) != 0)
>  		return -1;
>  
>  	const char *object_type =
> @@ -3882,9 +3881,8 @@ modify_priv(struct trigger *trigger, void *event)
>  	(void) event;
>  	struct tuple *tuple = (struct tuple *)trigger->data;
>  	struct priv_def priv;
> -	if (priv_def_create_from_tuple(&priv, tuple) != 0)
> -		return -1;
> -	if (grant_or_revoke(&priv) != 0)
> +	if (priv_def_create_from_tuple(&priv, tuple) != 0 ||
> +	    grant_or_revoke(&priv) != 0)
>  		return -1;
>  	return 0;
>  }
> @@ -3903,11 +3901,9 @@ on_replace_dd_priv(struct trigger * /* trigger */, void *event)
>  	struct priv_def priv;
>  
>  	if (new_tuple != NULL && old_tuple == NULL) {	/* grant */
> -		if (priv_def_create_from_tuple(&priv, new_tuple) != 0)
> -			return -1;
> -		if (priv_def_check(&priv, PRIV_GRANT) != 0)
> -			return -1;
> -		if (grant_or_revoke(&priv) != 0)
> +		if (priv_def_create_from_tuple(&priv, new_tuple) != 0 ||
> +		    priv_def_check(&priv, PRIV_GRANT) != 0 ||
> +		    grant_or_revoke(&priv) != 0)
>  			return -1;
>  		struct trigger *on_rollback =
>  			txn_alter_trigger_new(revoke_priv, new_tuple);
> @@ -3916,9 +3912,8 @@ on_replace_dd_priv(struct trigger * /* trigger */, void *event)
>  		txn_stmt_on_rollback(stmt, on_rollback);
>  	} else if (new_tuple == NULL) {                /* revoke */
>  		assert(old_tuple);
> -		if (priv_def_create_from_tuple(&priv, old_tuple) != 0)
> -			return -1;
> -		if (priv_def_check(&priv, PRIV_REVOKE) != 0)
> +		if (priv_def_create_from_tuple(&priv, old_tuple) != 0 ||
> +		    priv_def_check(&priv, PRIV_REVOKE) != 0)
>  			return -1;
>  		priv.access = 0;
>  		if (grant_or_revoke(&priv) != 0)
> @@ -3929,11 +3924,9 @@ on_replace_dd_priv(struct trigger * /* trigger */, void *event)
>  			return -1;
>  		txn_stmt_on_rollback(stmt, on_rollback);
>  	} else {                                       /* modify */
> -		if (priv_def_create_from_tuple(&priv, new_tuple) != 0)
> -			return -1;
> -		if (priv_def_check(&priv, PRIV_GRANT) != 0)
> -			return -1;
> -		if (grant_or_revoke(&priv) != 0)
> +		if (priv_def_create_from_tuple(&priv, new_tuple) != 0 ||
> +		    priv_def_check(&priv, PRIV_GRANT) != 0 ||
> +		    grant_or_revoke(&priv) != 0)
>  			return -1;
>  		struct trigger *on_rollback =
>  			txn_alter_trigger_new(modify_priv, old_tuple);
> @@ -5264,8 +5257,10 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
>  			return -1;
>  		auto fk_def_guard = make_scoped_guard([=] { free(fk_def); });
>  		struct space *child_space = space_cache_find(fk_def->child_id);
> +		if (child_space == NULL)
> +			return -1;
>  		struct space *parent_space = space_cache_find(fk_def->parent_id);
> -		if (child_space == NULL or parent_space == NULL)
> +		if (parent_space == NULL)
>  			return -1;
>  		struct fk_constraint *old_fk=
>  			fk_constraint_remove(&child_space->child_fk_constraint,
> -- 
> 2.17.1
> 

Re: [Tarantool-patches] [PATCH v5 4/8] refactoring: set diagnostics if sequence_by_id fails

[Sergey Ostanevich]

Hi!

LGTM. 

Please, create a follow-up tracker to review all sequence_by_id() uses:
for example, in on_drop_sequence_data_rollback() this can lead to a
coredump inside the sequence_set() in release build. 

Regards,
Sergos

On 22 Nov 05:46, Ilya Kosarev wrote:
> In refactoring: use non _xc version of functions in triggers
> (b75d5f85338158eb1734ca8832d3c4b8ac0b4086) sequence_cache_find was
> replaced by sequence_by_id. It led to the loss of diagnostics in case
> of sequence_by_id failure. Now it is fixed.
> 
> Part of #4247
> ---
>  src/box/alter.cc | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index aaaf53493..2628e1401 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -4365,8 +4365,10 @@ on_replace_dd_sequence_data(struct trigger * /* trigger */, void *event)
>  			    &id) != 0)
>  		return -1;
>  	struct sequence *seq = sequence_by_id(id);
> -	if (seq == NULL)
> +	if (seq == NULL) {
> +		diag_set(ClientError, ER_NO_SUCH_SEQUENCE, int2str(id));
>  		return -1;
> +	}
>  	if (new_tuple != NULL) {			/* INSERT, UPDATE */
>  		int64_t value;
>  		if (tuple_field_i64(new_tuple, BOX_SEQUENCE_DATA_FIELD_VALUE,
> @@ -4520,8 +4522,10 @@ on_replace_dd_space_sequence(struct trigger * /* trigger */, void *event)
>  	if (space == NULL)
>  		return -1;
>  	struct sequence *seq = sequence_by_id(sequence_id);
> -	if (seq == NULL)
> +	if (seq == NULL) {
> +		diag_set(ClientError, ER_NO_SUCH_SEQUENCE, int2str(sequence_id));
>  		return -1;
> +	}
>  
>  	enum priv_type priv_type = stmt->new_tuple ? PRIV_C : PRIV_D;
>  	if (stmt->new_tuple && stmt->old_tuple)
> -- 
> 2.17.1
> 

Re: [Tarantool-patches] [PATCH v5 5/8] refactoring: clear triggers from fresh exceptions

[Sergey Ostanevich]

Hi!

LGTM.

Sergos

On 22 Nov 05:46, Ilya Kosarev wrote:
> Clear triggers from freshly occured exceptions. Trivial replacements:
> `diag_raise` by `return -1`, _xc function by it's non _xc version.
> 
> Part of #4247
> ---
>  src/box/alter.cc  | 3 ++-
>  src/lua/trigger.c | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 2628e1401..4b3bbca31 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -5468,7 +5468,8 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
>  			if (old_def->language == ck_def->language &&
>  			    strcmp(old_def->expr_str, ck_def->expr_str) == 0) {
>  				old_def->is_enabled = ck_def->is_enabled;
> -				trigger_run_xc(&on_alter_space, space);
> +				if (trigger_run(&on_alter_space, space) != 0)
> +					return -1;
>  				return 0;
>  			}
>  		}
> diff --git a/src/lua/trigger.c b/src/lua/trigger.c
> index b1473bcc1..d588fc12c 100644
> --- a/src/lua/trigger.c
> +++ b/src/lua/trigger.c
> @@ -79,7 +79,7 @@ lbox_trigger_run(struct trigger *ptr, void *event)
>  	if (fiber()->storage.lua.stack == NULL) {
>  		L = luaT_newthread(tarantool_L);
>  		if (L == NULL)
> -			diag_raise();
> +			return -1;
>  		coro_ref = luaL_ref(tarantool_L, LUA_REGISTRYINDEX);
>  	} else {
>  		L = fiber()->storage.lua.stack;
> -- 
> 2.17.1
> 

Re: [Tarantool-patches] [PATCH v5 6/8] refactoring: update comment for index_def_check_tuple

[Sergey Ostanevich]

Hi!

LGTM.

Sergos

On 22 Nov 05:46, Ilya Kosarev wrote:
> Originally index_def_check_tuple comment said that it throws a nice
> error. Since refactoring: remove exceptions from
> index_def_new_from_tuple (90ac0037f6cae587d8ca589dbe45c58e67f05657)
> it returns an error. Now it is clearly specified in the comment.
> 
> Part of #4247
> ---
>  src/box/alter.cc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 4b3bbca31..684b93798 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -159,7 +159,7 @@ index_def_check_sequence(struct index_def *index_def, uint32_t sequence_fieldno,
>  
>  /**
>   * Support function for index_def_new_from_tuple(..)
> - * Checks tuple (of _index space) and sets a nice diag if it is invalid
> + * Checks tuple (of _index space) and returns an error if it is invalid
>   * Checks only types of fields and their count!
>   */
>  static int
> -- 
> 2.17.1
> 

Re: [Tarantool-patches] [PATCH v5 7/8] refactoring: remove redundant line in txn_alter_trigger_new

[Sergey Ostanevich]

Hi!

LGTM, thanks!

Sergos

On 22 Nov 05:46, Ilya Kosarev wrote:
> Since refactoring: clear privilege managing triggers from exceptions
> (977fca292ee97e82c6729d4b62a178ee8835af59) we are doing zero memset for
> trigger struct in txn_alter_trigger_new. This means we don't any more
> need to set any field of this struct to NULL explicitly.
> 
> Part of #4247
> ---
>  src/box/alter.cc | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 684b93798..a3a9a5ff9 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -816,7 +816,6 @@ txn_alter_trigger_new(trigger_f run, void *data)
>  	trigger = (struct trigger *)memset(trigger, 0, size);
>  	trigger->run = run;
>  	trigger->data = data;
> -	trigger->destroy = NULL;
>  	return trigger;
>  }
>  
> -- 
> 2.17.1
> 

Re: [Tarantool-patches] [PATCH v5 8/8] refactoring: remove try..catch wrapper around trigger->run

[Sergey Ostanevich]

Hi!

LGTM, thank you for the whole set!

Sergos

On 22 Nov 05:46, Ilya Kosarev wrote:
> Triggers don't throw exceptions any more. Now they have
> return codes to report errors.
> 
> Closes #4247
> ---
>  src/lib/core/trigger.cc | 25 ++++++++-----------------
>  1 file changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/src/lib/core/trigger.cc b/src/lib/core/trigger.cc
> index 8f6a83fb5..7fe112f8b 100644
> --- a/src/lib/core/trigger.cc
> +++ b/src/lib/core/trigger.cc
> @@ -30,32 +30,23 @@
>   */
>  
>  #include "trigger.h"
> -#include "exception.h"
>  
>  int
>  trigger_run(struct rlist *list, void *event)
>  {
> -	try {
> -		struct trigger *trigger, *tmp;
> -		rlist_foreach_entry_safe(trigger, list, link, tmp)
> -			if (trigger->run(trigger, event) != 0)
> -				return -1;
> -	} catch (Exception *e) {
> -		return -1;
> -	}
> +	struct trigger *trigger, *tmp;
> +	rlist_foreach_entry_safe(trigger, list, link, tmp)
> +		if (trigger->run(trigger, event) != 0)
> +			return -1;
>  	return 0;
>  }
>  
>  int
>  trigger_run_reverse(struct rlist *list, void *event)
>  {
> -	try {
> -		struct trigger *trigger, *tmp;
> -		rlist_foreach_entry_safe_reverse(trigger, list, link, tmp)
> -			if (trigger->run(trigger, event) != 0)
> -				return -1;
> -	} catch (Exception *e) {
> -		return -1;
> -	}
> +	struct trigger *trigger, *tmp;
> +	rlist_foreach_entry_safe_reverse(trigger, list, link, tmp)
> +		if (trigger->run(trigger, event) != 0)
> +			return -1;
>  	return 0;
>  }
> -- 
> 2.17.1
> 

Re: [Tarantool-patches] [PATCH v5 0/8] refactoring: remove exceptions from triggers follow-ups

[Kirill Yukhin]

Hello,

On 22 ноя 05:46, Ilya Kosarev wrote:
> This patchset is a pack of follow-ups to finish clearing triggers from
> exceptions. It is based on review comments for the patches of the
> original "refactoring: remove exceptions from triggers" patchset.
> 
> Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-4247-remove-exceptions-from-triggers 
> Issue: https://github.com/tarantool/tarantool/issues/4247

I've checked your patchset into master.

--
Regards, Kirill Yukhin