From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Mon, 9 Apr 2018 23:32:40 +0300 From: Konstantin Osipov Subject: Re: [PATCH 03/12] alter: move dictionary update from ModifySpace::alter_def to alter Message-ID: <20180409203240.GD4527@atlas> References: <8f209a2a0dd9b9e6f2e3100a8331906189442f3d.1523105106.git.vdavydov.dev@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8f209a2a0dd9b9e6f2e3100a8331906189442f3d.1523105106.git.vdavydov.dev@gmail.com> To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: * Vladimir Davydov [18/04/09 10:33]: > - struct tuple_dictionary *new_dict; > - /** > - * Old tuple dictionary stored to rollback in destructor, > - * if an exception had been raised after alter_def(), but > - * before alter(). > - */ > - struct tuple_dictionary *old_dict; > + struct tuple_dictionary *dict; Please provide a comment. > /* > - * Move new names into an old dictionary, which already is > - * referenced by existing tuple formats. New dictionary > - * object is deleted later, in destructor. > + * Use the old dictionary for the new space, because > + * it is already referenced by existing tuple formats. > + * We will update it in place in ModifySpace::alter. > */ I don't understand this comment. Naming the variable "dict" didn't help. Not having a comment for the variable where it was declared didn't help :( I do understand that your strategy is to swap the new space' dict with the old space dict and hold the new space dict in a temp variable until possible rollback. But I don't understand why you had to change it. The changeset comment says nothing about changing the way you modify the format, it only mentions changes in the timing. > - new_dict = def->dict; > - old_dict = alter->old_space->def->dict; > - tuple_dictionary_swap(new_dict, old_dict); > - def->dict = old_dict; > - tuple_dictionary_ref(old_dict); > + dict = def->dict; > + def->dict = alter->old_space->def->dict; > + tuple_dictionary_ref(def->dict); > > space_def_delete(alter->space_def); > alter->space_def = def; > @@ -959,21 +952,21 @@ ModifySpace::alter_def(struct alter_space *alter) > } > > void > -ModifySpace::commit(struct alter_space *alter, int64_t signature) > +ModifySpace::alter(struct alter_space *alter) > { > - (void) alter; > - (void) signature; > - old_dict = NULL; > + tuple_dictionary_swap(alter->new_space->def->dict, dict); > +} > + > +void > +ModifySpace::rollback(struct alter_space *alter) > +{ > + tuple_dictionary_swap(alter->new_space->def->dict, dict); > } > > ModifySpace::~ModifySpace() > { > - if (new_dict != NULL) { > - /* Return old names into the old dict. */ > - if (old_dict != NULL) > - tuple_dictionary_swap(new_dict, old_dict); > - tuple_dictionary_unref(new_dict); > - } > + if (dict != NULL) > + tuple_dictionary_unref(dict); > if (def != NULL) > space_def_delete(def); > } > diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c > index d3a7702d..11481473 100644 > --- a/src/box/tuple_format.c > +++ b/src/box/tuple_format.c > @@ -262,8 +262,6 @@ tuple_format_new(struct tuple_format_vtab *vtab, struct key_def * const *keys, > const struct field_def *space_fields, > uint32_t space_field_count, struct tuple_dictionary *dict) > { > - assert((dict == NULL && space_field_count == 0) || > - (dict != NULL && space_field_count == dict->name_count)); > struct tuple_format *format = > tuple_format_alloc(keys, key_count, space_field_count, dict); > if (format == NULL) > -- > 2.11.0 > -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov