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

[Konstantin Osipov <kostja@tarantool.org>]

* Vladimir Davydov <vdavydov.dev@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
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.

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.

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.


-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov