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

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

On Thu, Mar 28, 2019 at 04:45:50PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@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'.