Re: [PATCH 4/4] vinyl: abort rw transactions when instance switches to ro

[Konstantin Osipov <kostja@tarantool.org>]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/03/05 10:25]:
> A Vinyl transaction may yield while having a non-empty write set. This
> opens a time window for the instance to switch to read-only mode. Since
> we check ro flag only before executing a DML request, the transaction
> would successfully commit in such a case, breaking the assumption that
> no writes are possible on an instance after box.cfg{read_only=true}
> returns. In particular, this breaks master-replica switching logic.
> 
> Fix this by aborting all local rw transactions before switching to
> read-only mode. Note, remote rw transactions must not be aborted,
> because they ignore ro flag.
> 
> Closes #4016

OK to push, a few comments below.

> -	double timeout = (current_session()->type != SESSION_TYPE_APPLIER ?
> -			  env->timeout : TIMEOUT_INFINITY);
> +	double timeout = (tx->is_remote ? TIMEOUT_INFINITY : env->timeout);

is_remote is a vague name. A net.box connection is remote as well.
Why not simply have tx->session_type or
tx->session_type_is_applier or tx->is_session_type_applier? 

>  void
> +tx_manager_abort_writers_for_ro(struct tx_manager *xm)
> +{
> +	struct vy_tx *tx;
> +	rlist_foreach_entry(tx, &xm->writers, in_writers) {
> +		/* Remote transactions ignore ro flag. */
> +		if (tx->state == VINYL_TX_READY && !tx->is_remote)
> +			tx->state = VINYL_TX_ABORT;
> +	}
> +}

I think we should retry applier transaction in case of
abort conlfict. The error code for transaction conflict is
distinguishable in the diagnostics area. This would help here and
when conflicting with user transactions. Please feel free to do in
a separate patch - or Georgy could do it in scope of his work on
transactional applier.

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