Tarantool development patches archive
 help / color / mirror / Atom feed
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);
> >  }

  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