From: Konstantin Osipov <kostja@tarantool.org> To: Vladimir Davydov <vdavydov.dev@gmail.com> Cc: tarantool-patches@freelists.org Subject: Re: [PATCH 03/12] alter: move dictionary update from ModifySpace::alter_def to alter Date: Mon, 9 Apr 2018 23:32:40 +0300 [thread overview] Message-ID: <20180409203240.GD4527@atlas> (raw) In-Reply-To: <8f209a2a0dd9b9e6f2e3100a8331906189442f3d.1523105106.git.vdavydov.dev@gmail.com> * Vladimir Davydov <vdavydov.dev@gmail.com> [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
next prev parent reply other threads:[~2018-04-09 20:32 UTC|newest] Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-04-07 13:37 [PATCH 00/12] vinyl: allow to modify format of non-empty spaces Vladimir Davydov 2018-04-07 13:37 ` [PATCH 01/12] alter: introduce CheckSpaceFormat AlterSpaceOp for validating format Vladimir Davydov 2018-04-09 20:25 ` Konstantin Osipov 2018-04-07 13:37 ` [PATCH 02/12] alter: fold ModifySpaceFormat into ModifySpace Vladimir Davydov 2018-04-09 20:26 ` Konstantin Osipov 2018-04-07 13:38 ` [PATCH 03/12] alter: move dictionary update from ModifySpace::alter_def to alter Vladimir Davydov 2018-04-09 20:32 ` Konstantin Osipov [this message] 2018-04-10 7:53 ` Vladimir Davydov 2018-04-10 11:45 ` Vladimir Davydov 2018-04-07 13:38 ` [PATCH 04/12] alter: use space_index instead of index_find where appropriate Vladimir Davydov 2018-04-09 20:34 ` Konstantin Osipov 2018-04-07 13:38 ` [PATCH 05/12] alter: allocate triggers before the point of no return Vladimir Davydov 2018-04-09 20:36 ` Konstantin Osipov 2018-04-10 7:57 ` Vladimir Davydov 2018-04-10 11:54 ` Vladimir Davydov 2018-04-07 13:38 ` [PATCH 06/12] space: space_vtab::build_secondary_key => build_index Vladimir Davydov 2018-04-09 20:39 ` Konstantin Osipov 2018-04-10 8:05 ` Vladimir Davydov 2018-04-10 12:14 ` Vladimir Davydov 2018-04-07 13:38 ` [PATCH 07/12] space: pass new format instead of new space to space_vtab::check_format Vladimir Davydov 2018-04-09 20:40 ` Konstantin Osipov 2018-04-07 13:38 ` [PATCH 08/12] alter: introduce preparation phase Vladimir Davydov 2018-04-09 20:46 ` [tarantool-patches] " Konstantin Osipov 2018-04-10 8:31 ` Vladimir Davydov 2018-04-10 8:46 ` Konstantin Osipov 2018-04-07 13:38 ` [PATCH 09/12] alter: zap space_def_check_compatibility Vladimir Davydov 2018-04-09 20:49 ` Konstantin Osipov 2018-04-07 13:38 ` [PATCH 10/12] vinyl: remove superfluous ddl checks Vladimir Davydov 2018-04-09 20:49 ` Konstantin Osipov 2018-04-07 13:38 ` [PATCH 11/12] vinyl: force index rebuild if indexed field type is narrowed Vladimir Davydov 2018-04-09 20:51 ` Konstantin Osipov 2018-04-07 13:38 ` [PATCH 12/12] vinyl: allow to modify format of non-empty spaces Vladimir Davydov 2018-04-09 8:24 ` Vladimir Davydov 2018-04-09 20:55 ` Konstantin Osipov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20180409203240.GD4527@atlas \ --to=kostja@tarantool.org \ --cc=tarantool-patches@freelists.org \ --cc=vdavydov.dev@gmail.com \ --subject='Re: [PATCH 03/12] alter: move dictionary update from ModifySpace::alter_def to alter' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox