[tarantool-patches] Re: [PATCH 08/12] alter: introduce preparation phase
Konstantin Osipov
kostja at tarantool.org
Tue Apr 10 11:46:09 MSK 2018
* Vladimir Davydov <vdavydov.dev at 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
More information about the Tarantool-patches
mailing list