Re: [tarantool-patches] Re: [PATCH 08/12] alter: introduce preparation phase

[Konstantin Osipov <kostja@tarantool.org>]

* Vladimir Davydov <vdavydov.dev@gmail.com> [18/04/10 11:32]:
> The problem is that we first call 'prepare' for all registered
> operations. If 'prepare' fails for any of those operations, we
> need to undo its effect for all operations for which 'prepare'
> succeeded. I see the following ways of doing this:
> 
>  - Reuse 'rollback' for undoing the effect of 'prepare', but this way we
>    would have to be able to differentiate between 'rollback' after WAL
>    error and 'rollback' after 'prepare' for all operations, even those
>    that don't use 'prepare' at all, e.g. ModifySpace. This would look
>    convoluted IMO.
> 
>  - Undo the effect of 'prepare' in destructor. I don't like it, because
>    IMO destructor should be used only for freeing up resources. Calling,
>    for example, index_abort_create() from it would look ugly IMO.

I prefer this option. I don't see why we need two functions simply
because "destructor should be used only for freeing up resources".
Let's drop C++ there altogether and forget this argument, if it
makes it easier.
> 
>  - Introduce 'abort' that would undo the effect of 'prepare'. All things
>    considered, I find it the best way to go from the code readability
>    point of view.
> 
> > 
> > > While we are at it, let's also add some comments to AlterSpaceOp
> > > methods.
> > > ---
> > >  src/box/alter.cc | 104 ++++++++++++++++++++++++++++++++++++++++++++++---------
> > >  1 file changed, 88 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/src/box/alter.cc b/src/box/alter.cc
> > > index 9dd8d8d5..f5996850 100644
> > > --- a/src/box/alter.cc
> > > +++ b/src/box/alter.cc
> > > @@ -614,12 +614,48 @@ struct alter_space;
> > >  class AlterSpaceOp {
> > >  public:
> > >  	AlterSpaceOp(struct alter_space *alter);
> > > +
> > > +	/** Link in alter_space::ops. */
> > >  	struct rlist link;
> > > +	/**
> > > +	 * Called before creating the new space. Used to update
> > > +	 * the space definition and/or key list that will be used
> > > +	 * for creating the new space. Must not yield or fail.
> > > +	 */
> > >  	virtual void alter_def(struct alter_space * /* alter */) {}
> > > +	/**
> > > +	 * Called after creating a new space. Used for performing
> > > +	 * long-lasting operations, such as index rebuild or format
> > > +	 * check. May yield. May throw an exception. Must not modify
> > > +	 * the old space.
> > > +	 */
> > > +	virtual void prepare(struct alter_space * /* alter */) {}
> > > +	/**
> > > +	 * Called if the preparation phase failed after this
> > > +	 * operation has been successfully prepared. Supposed
> > > +	 * to undo the effect of AlterSpaceOp::prepare.
> > > +	 */
> > > +	virtual void abort(struct alter_space * /* alter */) {}
> > > +	/**
> > > +	 * Called after all registered operations have completed
> > > +	 * the preparation phase. Used to propagate the old space
> > > +	 * state to the new space (e.g. move unchanged indexes).
> > > +	 * Must not yield or fail.
> > > +	 */
> > >  	virtual void alter(struct alter_space * /* alter */) {}
> > > +	/**
> > > +	 * Called after the change has been successfully written
> > > +	 * to WAL. Must not fail.
> > > +	 */
> > >  	virtual void commit(struct alter_space * /* alter */,
> > >  			    int64_t /* signature */) {}
> > > +	/**
> > > +	 * Called in case a WAL error occurred. It is supposed to undo
> > > +	 * the effect of AlterSpaceOp::prepare and AlterSpaceOp::alter.
> > > +	 * Must not fail.
> > > +	 */
> > >  	virtual void rollback(struct alter_space * /* alter */) {}
> > > +
> > >  	virtual ~AlterSpaceOp() {}
> > >  
> > >  	void *operator new(size_t size)
> > > @@ -657,6 +693,12 @@ struct alter_space {
> > >  	/** New space. */
> > >  	struct space *new_space;
> > >  	/**
> > > +	 * Space used as the source when building a new index.
> > > +	 * Initially it is set to old_space, but may be reset
> > > +	 * to new_space if the primary key is recreated.
> > > +	 */
> > > +	struct space *source_space;
> > 
> > Sounds like this shouldn't be in alter, but should be in its own
> > op designated to recreation of the primary key.
> 
> We need to update it for all CreateIndex operations, whenever we detect
> that the primary index is truncated, i.e. CreateIndex is called for the
> primary index. I haven't figured out a better way to do it. We might
> need to discuss it f2f.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov