From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 28 Mar 2019 17:02:40 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] Re: [PATCH 3/4] vinyl: abort affected transactions when space is removed from cache Message-ID: <20190328140240.krymirsra4dibnst@esperanza> References: <20190328134550.GB19174@chai> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190328134550.GB19174@chai> To: Konstantin Osipov Cc: tarantool-patches@freelists.org List-ID: On Thu, Mar 28, 2019 at 04:45:50PM +0300, Konstantin Osipov wrote: > * Vladimir Davydov [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'.