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

["Ilya Kosarev" <i.kosarev@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 1962 bytes --]


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

[-- Attachment #2: Type: text/html, Size: 3858 bytes --]