[tarantool-patches] Re: [PATCH 1/4] vinyl: introduce hash of spaces affected by transaction

[Vladimir Davydov]

On Thu, Mar 28, 2019 at 04:25:52PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev at gmail.com> [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.