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

Vladimir Davydov vdavydov.dev at gmail.com
Thu Mar 28 17:02:40 MSK 2019


On Thu, Mar 28, 2019 at 04:45:50PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev at gmail.com> [19/03/24 00:10]:
> > 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'.



More information about the Tarantool-patches mailing list