Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladimir Davydov <vdavydov.dev@gmail.com>
To: Konstantin Osipov <kostja@tarantool.org>
Cc: tarantool-patches@freelists.org
Subject: Re: [tarantool-patches] Re: [PATCH 1/4] vinyl: introduce hash of spaces affected by transaction
Date: Thu, 28 Mar 2019 16:58:51 +0300	[thread overview]
Message-ID: <20190328135851.i5ugcq7ovqfpyddz@esperanza> (raw)
In-Reply-To: <20190328132552.GA19174@chai>

On Thu, Mar 28, 2019 at 04:25:52PM +0300, Konstantin Osipov wrote:
> * 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

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.

  reply	other threads:[~2019-03-28 13:58 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-23 21:07 [PATCH 0/4] Fix DML vs DDL race Vladimir Davydov
2019-03-23 21:07 ` [PATCH 1/4] vinyl: introduce hash of spaces affected by transaction Vladimir Davydov
2019-03-28 13:25   ` [tarantool-patches] " Konstantin Osipov
2019-03-28 13:58     ` Vladimir Davydov [this message]
2019-03-23 21:07 ` [PATCH 2/4] vinyl: don't abort transactions that modify only local spaces for ro Vladimir Davydov
2019-03-25  5:27   ` [tarantool-patches] " Георгий Кириченко
2019-03-25  8:13     ` Vladimir Davydov
2019-03-25  8:58       ` Георгий Кириченко
2019-03-25  9:30         ` Vladimir Davydov
2019-03-23 21:07 ` [PATCH 3/4] vinyl: abort affected transactions when space is removed from cache Vladimir Davydov
2019-03-25  5:26   ` [tarantool-patches] " Георгий Кириченко
2019-03-25  8:17     ` Vladimir Davydov
     [not found]       ` <1564677.EMV258VVK2@home.lan>
2019-03-25  9:51         ` Vladimir Davydov
2019-03-25  5:45   ` Георгий Кириченко
2019-03-25  8:21     ` Vladimir Davydov
2019-03-25  9:03       ` Георгий Кириченко
2019-03-28 13:45   ` [tarantool-patches] " Konstantin Osipov
2019-03-28 14:02     ` Vladimir Davydov
2019-03-28 14:12       ` Konstantin Osipov
2019-03-23 21:07 ` [PATCH 4/4] Revert "test: skip ddl test for vinyl on travis" Vladimir Davydov
2019-03-28 13:46   ` [tarantool-patches] " Konstantin Osipov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190328135851.i5ugcq7ovqfpyddz@esperanza \
    --to=vdavydov.dev@gmail.com \
    --cc=kostja@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='Re: [tarantool-patches] Re: [PATCH 1/4] vinyl: introduce hash of spaces affected by transaction' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox