From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-f194.google.com (mail-lj1-f194.google.com [209.85.208.194]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 5CF21469719 for ; Fri, 14 Feb 2020 10:25:28 +0300 (MSK) Received: by mail-lj1-f194.google.com with SMTP id x14so9526678ljd.13 for ; Thu, 13 Feb 2020 23:25:28 -0800 (PST) Date: Fri, 14 Feb 2020 10:25:26 +0300 From: Konstantin Osipov Message-ID: <20200214072526.GC15237@atlas> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Tarantool-patches] [PATCH v2 4/4] replication: do not promote local_vclock_at_subscribe unnecessarily List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: sergepetrenko Cc: tarantool-patches@dev.tarantool.org, v.shpilevoy@tarantool.org * sergepetrenko [20/02/14 09:46]: > From: Serge Petrenko > > 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