From: Vladimir Davydov <vdavydov.dev@gmail.com> To: Konstantin Osipov <kostja@tarantool.org> Cc: tarantool-patches@freelists.org Subject: Re: [PATCH 03/12] alter: move dictionary update from ModifySpace::alter_def to alter Date: Tue, 10 Apr 2018 10:53:07 +0300 [thread overview] Message-ID: <20180410075307.2swfafgf3hqvzecs@esperanza> (raw) In-Reply-To: <20180409203240.GD4527@atlas> On Mon, Apr 09, 2018 at 11:32:40PM +0300, Konstantin Osipov wrote: > * 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. There is a comment left from 'new_dict'. It just didn't get into the diff. Here is the omitted context: class ModifySpace: public AlterSpaceOp { public: ModifySpace(struct alter_space *alter, struct space_def *def_arg) :AlterSpaceOp(alter), def(def_arg) {} /** * Newely created field dictionary. When new space_def is * created, it allocates new dictionary. Alter moves new * names into an old dictionary and deletes new one. */ struct tuple_dictionary *dict; /* New space definition. */ struct space_def *def; virtual void alter_def(struct alter_space *alter); virtual void alter(struct alter_space *alter); virtual void rollback(struct alter_space *alter); virtual ~ModifySpace(); }; > > > /* > > - * 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. Because I did only change the timing. The logic behind tuple dictionary update was originally introduced by commit 9f6113abb ("alter: introduce ModifySpaceFormat alter operation") and I didn't change it. We need to swap the content of tuple dictionary, because the old space's dictionary is referenced by existing tuples and we need to update tuple field names for them too. We can't just update a pointer in tuple_format because there may be tuples referencing formats left from prevoius ALTER operations and we don't keep track of those. So whenever we alter a space definition, we use the dictionary of the old space when creating a new space and swap their contents on success to update names for all tuples stored in the space. > > > - 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); > > }
next prev parent reply other threads:[~2018-04-10 7:53 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 2018-04-10 7:53 ` Vladimir Davydov [this message] 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=20180410075307.2swfafgf3hqvzecs@esperanza \ --to=vdavydov.dev@gmail.com \ --cc=kostja@tarantool.org \ --cc=tarantool-patches@freelists.org \ --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