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

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Mon, Apr 09, 2018 at 11:46:47PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [18/04/09 10:33]:
> > We may yield while rebuilding a vinyl index hence we must not modify
> > the old space before we are done building all indexes, otherwise the
> > user might see an inconsistent state while a space is being altered.
> > Currently, this requirement doesn't hold - we both modify the old space
> > and build the new indexes in AlterSpaceOp::alter. Let's introduce
> > AlterSpaceOp::prepare method that will perform all yielding operations
> > and forbid yielding in AlterSpaceOp::alter.
> > 
> > Actually, we introduce two new methods AlterSpaceOp::prepare and abort.
> > The latter is called if preparation phase failed and we need to undo the
> > effect of AlterSpaceOp::prepare for all operations that have performed
> > it. AlterSpaceOp::rollback is now only called in case of WAL error.
> > AlterSpaceOp::alter may not fail anymore, not that it needs to anyway.
> > 
> > Here's the list of AlterSpaceOp descendants that now use 'prepare'
> > instead of 'alter': CheckSpaceFormat, CreateIndex, DropIndex, and
> > RebuildIndex.
> 
> Any part of alter may fail. alter_def may fail.

I changed that - see the new comments. Now 'alter_def' may not fail, nor
may 'alter'. Only 'prepare' is allowed to fail or yield. This simplifies
the code of alter_space_do() and makes it easier to follow IMO. Anyway,
we don't really need to fail while performing these operations, not with
the new 'prepare' phase.

> We don't have a
> separate abort() callback for a failed alter_def. Undoing effects
> of alter in 3 separate calls (abort(), rollback(), destructor) 
> creates a lot of confusion. The idea is that each operation should
> clean things up automatically: if commit() was called, old stuff
> needs to  be cleaned up. If rollback() was called, new stuff needs
> to be cleaned up. If neither was called, we assume we didn't reach
> the point of writing to WAL, and clean up everything that has been
> built up so far.

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.

 - 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.