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