Re: [Tarantool-patches] [tarantool-patches] [PATCH v4 11/20] refactoring: use non _xc version of functions in triggers

[Sergey Ostanevich <sergos@tarantool.org>]

Hi!

Great, LGTM.

Sergos

On 10 Nov 01:52, Ilya Kosarev wrote:
> 
> Hi!
> 
> Thanks for your review.
> Yes, i definitely missed diag_set with sequence_by_id. Will be fixed in v5. 
> 
> Also i will reconsider combining child & parent space search.
> Sincerely,
> Ilya Kosarev
> 
> 
> >Суббота,  9 ноября 2019, 23:01 +03:00 от Sergey Ostanevich <sergos@tarantool.org>:
> >
> >Hi!
> >
> >LGTM, just couple of nits below you can add to a follow-up.
> >
> >Sergos
> >
> >>diff --git a/src/box/alter.cc b/src/box/alter.cc
> >>index e56d7c6cc..69a749326 100644
> >>--- a/src/box/alter.cc
> >>+++ b/src/box/alter.cc
> >>@@ -4117,14 +4167,18 @@ on_replace_dd_sequence_data(struct trigger * /* trigger */, void *event)
> >> 	struct tuple *old_tuple = stmt->old_tuple;
> >> 	struct tuple *new_tuple = stmt->new_tuple;
> >>
> >>-	uint32_t id = tuple_field_u32_xc(old_tuple ?: new_tuple,
> >>-					 BOX_SEQUENCE_DATA_FIELD_ID);
> >>-	struct sequence *seq = sequence_cache_find(id);
> >>+	uint32_t id;
> >>+	if (tuple_field_u32(old_tuple ?: new_tuple, BOX_SEQUENCE_DATA_FIELD_ID,
> >>+			    &id) != 0)
> >>+		return -1;
> >>+	struct sequence *seq = sequence_by_id(id);
> >> 	if (seq == NULL)
> >> 		return -1;
> >Note, that here we will miss the diag_set for non-zero value. 
> >Perhaps a question of further patches for sequence_by_id() - but I didn't find it in log
> >
> >>@@ -4958,10 +5034,10 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
> >> 			fk_constraint_def_new_from_tuple(old_tuple,
> >> 						ER_DROP_FK_CONSTRAINT);
> >> 		auto fk_def_guard = make_scoped_guard([=] { free(fk_def); });
> >>-		struct space *child_space =
> >>-			space_cache_find_xc(fk_def->child_id);
> >>-		struct space *parent_space =
> >>-			space_cache_find_xc(fk_def->parent_id);
> >>+		struct space *child_space = space_cache_find(fk_def->child_id);
> >>+		struct space *parent_space = space_cache_find(fk_def->parent_id);
> >>+		if (child_space == NULL or parent_space == NULL)
> >>+			return -1; 
> >Shall we return after the first error? Will they affect each other?
> >
> >>
> >     
> 
> 
> -- 
> Ilya Kosarev