Re: [Tarantool-patches] [PATCH v2 4/4] replication: do not promote local_vclock_at_subscribe unnecessarily

[Konstantin Osipov <kostja.osipov@gmail.com>]

* sergepetrenko <sergepetrenko@tarantool.org> [20/02/14 09:46]:
> From: Serge Petrenko <sergepetrenko@tarantool.org>
> 
> When master processes a subscribe response, it responds with its vclock
> at the moment of receiving the request. However, the fiber processing
> the request may yield on coio_write_xrow, when sending the response to
> the replica. In the meantime, master may apply additional rows coming
> from the replica after it has issued SUBSCRIBE.
> Then in relay_subscribe master sets its local vclock_at_subscribe to
> a possibly updated value of replicaset.vclock
> So, set local_vclock_at_subscribe to a remembered value, rather than an
> updated one.

I don't fully understand the explanation and what this fix
achieves.

local_vclock_at_subscribe is used to leave orphan mode. It
basically tells the applier when it more or less has fully caught
up with the relay. 

It should not impact replication correctness in any other way.

I.e. it shouldn't matter whether or not it's accurate - while the
applier is reading rows from the master, the master can get new
rows anyway.

If local_vclock_at_subscribe has any other meaning/impact, it's a bug. 


> 
> Follow-up #4739
> ---
>  src/box/box.cc   |  2 +-
>  src/box/relay.cc | 13 +++++++++++--
>  src/box/relay.h  |  3 ++-
>  3 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 952d60ad1..7dec1ae6b 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -1871,7 +1871,7 @@ box_process_subscribe(struct ev_io *io, struct xrow_header *header)
>  	 * indefinitely).
>  	 */
>  	relay_subscribe(replica, io->fd, header->sync, &replica_clock,
> -			replica_version_id);
> +			replica_version_id, &vclock);
>  }
>  
>  void
> diff --git a/src/box/relay.cc b/src/box/relay.cc
> index b89632273..b69646446 100644
> --- a/src/box/relay.cc
> +++ b/src/box/relay.cc
> @@ -676,7 +676,8 @@ relay_subscribe_f(va_list ap)
>  /** Replication acceptor fiber handler. */
>  void
>  relay_subscribe(struct replica *replica, int fd, uint64_t sync,
> -		struct vclock *replica_clock, uint32_t replica_version_id)
> +		struct vclock *replica_clock, uint32_t replica_version_id,
> +		struct vclock *clock_at_subscribe)
>  {
>  	assert(replica->anon || replica->id != REPLICA_ID_NIL);
>  	struct relay *relay = replica->relay;
> @@ -699,7 +700,15 @@ relay_subscribe(struct replica *replica, int fd, uint64_t sync,
>  		replica_on_relay_stop(replica);
>  	});
>  
> -	vclock_copy(&relay->local_vclock_at_subscribe, &replicaset.vclock);
> +	/*
> +	 * It's too late to remember replicaset.vclock as local
> +	 * vclock at subscribe. It might have incremented while we
> +	 * were writing a subscribe response, and we don't want to
> +	 * replicate back rows originating from the replica and
> +	 * having arrived later than replica has issued
> +	 * SUBSCRIBE.


I still don't und
> +	 */
> +	vclock_copy(&relay->local_vclock_at_subscribe, clock_at_subscribe);
>  	relay->r = recovery_new(cfg_gets("wal_dir"), false,
>  			        replica_clock);
>  	vclock_copy(&relay->tx.vclock, replica_clock);
> diff --git a/src/box/relay.h b/src/box/relay.h
> index e1782d78f..54ebd6731 100644
> --- a/src/box/relay.h
> +++ b/src/box/relay.h
> @@ -124,6 +124,7 @@ relay_final_join(int fd, uint64_t sync, struct vclock *start_vclock,
>   */
>  void
>  relay_subscribe(struct replica *replica, int fd, uint64_t sync,
> -		struct vclock *replica_vclock, uint32_t replica_version_id);
> +		struct vclock *replica_vclock, uint32_t replica_version_id,
> +		struct vclock *clock_at_subscribe);
>  
>  #endif /* TARANTOOL_REPLICATION_RELAY_H_INCLUDED */
> -- 
> 2.20.1 (Apple Git-117)
> 

-- 
Konstantin Osipov, Moscow, Russia
https://scylladb.com