From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 28 Mar 2019 16:58:51 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] Re: [PATCH 1/4] vinyl: introduce hash of spaces affected by transaction Message-ID: <20190328135851.i5ugcq7ovqfpyddz@esperanza> References: <77567cf07633e5b3f8c60772cbf7fc702cf15297.1553373792.git.vdavydov.dev@gmail.com> <20190328132552.GA19174@chai> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190328132552.GA19174@chai> To: Konstantin Osipov Cc: tarantool-patches@freelists.org List-ID: On Thu, Mar 28, 2019 at 04:25:52PM +0300, Konstantin Osipov wrote: > * Vladimir Davydov [19/03/24 00:10]: > > We need to abort all transactions writing to an altered space when > > a new index is build. Currently, we use the write set to look up such > > transactions, but it isn't quite correct, because a transaction could > > yield on disk read before inserting a statement into the write set. > > To address this problem, this patch introduces a hash of spaces affected > > by each transaction and makes tx_manager_abort_writers_for_ddl() use it > > instead of the write set to abort transactions for DDL. > > I think this is an overkill for the problem you're dealing with. > > The root of the problem is that a vinyl statement is only added to tx > write set after a read/write has been performed, not before. The > moment we started using tx write set for aborts, we should have > fixed this, since one locks before one writes. > > The approach you chose also adds struct space to vy_tx. I know > that there already is a dependency on struct space in vinyl, but > why make it stronger? > > Now, to track before use, you would need to add a surrogate txv to > tx write set at the beginning of vy_update/vy_replace/etc, not at > the end of it. It doesn't seem difficult to do, but let's assume We'd have to handle this in txw iterator. I'd rather avoid it. At least, at this point. > for a moment it is, and it is indeed perhaps more than we want to > do in 1.10 at this point. > > > Provided we can't track before use, do we really need a hash of all > spaces just to cover the small gap between a read and a track? Why > not have a single vy_tx member struct space *last_stmt_space, > for the purposes of abort? It could be set at begin_statement() and > cleared in rollback_statement() just the same. This sounds perfect to me. I'll rework the patch accordingly. > > In future XM object will be global and we'll keep a list of > struct txn in it, not struct vy_tx. Once we begin doing it, > aborting writers for ddl and ro will also become straightforward. > This is a yet another direction at which one could develop this > patch - but I guess it's only acceptable for 2.x, not for 1.10. Yeah. I'd even refrain from doing this in 2.x until we have a roadmap for the common transaction manager. > > I know there is a subsequent patch that adds a nice feature - > local transactions are not aborted when switching to RO. Well, > it's nice, but not important for this problem, so I think this > patch could be dropped. Okay.