From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <vdavydov.dev@gmail.com>
Date: Tue, 10 Apr 2018 10:53:07 +0300
From: Vladimir Davydov <vdavydov.dev@gmail.com>
Subject: Re: [PATCH 03/12] alter: move dictionary update from
 ModifySpace::alter_def to alter
Message-ID: <20180410075307.2swfafgf3hqvzecs@esperanza>
References: <cover.1523105106.git.vdavydov.dev@gmail.com>
 <8f209a2a0dd9b9e6f2e3100a8331906189442f3d.1523105106.git.vdavydov.dev@gmail.com>
 <20180409203240.GD4527@atlas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180409203240.GD4527@atlas>
To: Konstantin Osipov <kostja@tarantool.org>
Cc: tarantool-patches@freelists.org
List-ID: <tarantool-patches.dev.tarantool.org>

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);
> >  }