Re: [PATCH 4/5] memtx: fix txn_on_yield for DDL transactions

[Vladimir Davydov <vdavydov.dev@gmail.com>]

On Mon, Jul 08, 2019 at 07:41:41PM +0300, Vladimir Davydov wrote:
> On Mon, Jul 08, 2019 at 03:22:48PM +0300, Konstantin Osipov wrote:
> > * Vladimir Davydov <vdavydov.dev@gmail.com> [19/07/05 23:27]:
> > > Memtx engine doesn't allow yielding inside a transaction. To achieve
> > > that, it installs fiber->on_yield trigger that aborts the current
> > > transaction (rolls it back, but leaves it be so that commit fails).
> > > 
> > > There's an exception though - DDL statements are allowed to yield.
> > > This is required so as not to block the event loop while a new index
> > > is built or a space format is checked. Currently, we handle this
> > > exception by checking space id and omitting installation of the
> > > trigger for system spaces. This isn't entirely correct, because we
> > > may yield after a DDL statement is complete, in which case the
> > > transaction won't be aborted though it should:
> > > 
> > >   box.begin()
> > >   box.space.my_space:create_index('my_index')
> > >   fiber.sleep(0) -- doesn't abort the transaction!
> > > 
> > > This patch fixes the problem by making the memtx engine install the
> > > on_yield trigger unconditionally, for all kinds of transactions, and
> > > instead explicitly disabling the trigger for yielding DDL operations.
> > 
> > Nit: I think since we set up triggers in memtx_* methods, we
> > should clear them inside memtx_* subsystem as well.
> > 
> > so it's not txn_*, it's memtx_{enable|disable}_yields
> 
> Calling a memtx_* method from vinyl would look rather weird IMO.

May be, instead we should move memtx stuff dealing with yields to
txn? Something like, txn_disable_yield() that would disable yields
altogether. Memtx would call this function while vinyl wouldn't.
Then everything related to yields in transactions would live in
txn.c. What do you think?