[tarantool-patches] Re: [PATCH 3/4] vinyl: abort affected transactions when space is removed from cache

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev at gmail.com> [19/03/28 17:05]:
> > > A DDL operation creates a new struct space container, moving unaffected
> > > indexes from the old container, then destroying it. The problem is there
> > > may be a DML request for this space, which was passed the old container
> > > in the argument list and then yielded on disk read. When it resumes, it
> > > may try to dereference the old space container, which may have already
> > > been destroyed. This will most certainly result in a crash.
> > > 
> > > To address this problem, we introduce a new space callback, invalidate,
> > > which is called for the old space on space_cache_replace(). In case of
> > > vinyl, this callback aborts all transactions involving the space. To
> > > prevent a DML request from dereferencing a destroyed space, we also make
> > > the iterator code check the current transaction state after each yield
> > > and return an error if it was aborted. This should make any DML request
> > > bail out early without dereferencing the space anymore.
> > 
> > This patch looks good to me. I don't share Georgy's concerns. I
> > would make the name of the callback more explicit, like
> > abort_transactions_in_engine, so that it clear what it is doing
> > now. In future, should we find more use for it, I would rename it.
> 
> 'invalidate' sounds so much better IMO. Besides, the name suggests that
> the space becomes invalid when it's called and hence the function should
> take extra care.
> 
> 'abort_transactions' is, in fact, a more generic name. Looking at it,
> I wouldn't say that it should take extra precautions to assure that
> currently running transactions won't dereference the old space by any
> chance.
> 
> So I'm quite convinced we'd better call it 'invalidate'.

:)

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