From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp55.i.mail.ru (smtp55.i.mail.ru [217.69.128.35]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id CA70B452566 for ; Mon, 11 Nov 2019 17:37:55 +0300 (MSK) Date: Mon, 11 Nov 2019 17:37:55 +0300 From: Sergey Ostanevich Message-ID: <20191111143755.GB10433@tarantool.org> References: <1573329671.649762688@f148.i.mail.ru> <1573339939.692166871@f275.i.mail.ru> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1573339939.692166871@f275.i.mail.ru> Subject: Re: [Tarantool-patches] [tarantool-patches] [PATCH v4 11/20] refactoring: use non _xc version of functions in triggers List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ilya Kosarev Cc: tarantool-patches , tarantool-patches 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 : > > > >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