From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 10 Apr 2018 11:46:09 +0300 From: Konstantin Osipov Subject: Re: [tarantool-patches] Re: [PATCH 08/12] alter: introduce preparation phase Message-ID: <20180410084609.GQ4527@atlas> References: <20180409204647.GI4527@atlas> <20180410083159.4x3t6uo4xctl73ze@esperanza> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180410083159.4x3t6uo4xctl73ze@esperanza> To: Vladimir Davydov Cc: tarantool-patches@freelists.org List-ID: * Vladimir Davydov [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