From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id E0C8A4696C7 for ; Sat, 4 Apr 2020 23:51:58 +0300 (MSK) References: From: Vladislav Shpilevoy Message-ID: Date: Sat, 4 Apr 2020 22:51:57 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v5 4/4] box: start counting local space requests separately List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Serge Petrenko , kostja.osipov@gmail.com Cc: tarantool-patches@dev.tarantool.org Thanks for the patch! See 3 comments below. > diff --git a/src/box/box.cc b/src/box/box.cc > index bf95d1b5e..0762266b0 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -1879,6 +1879,23 @@ box_process_subscribe(struct ev_io *io, struct xrow_header *header) > say_info("remote vclock %s local vclock %s", > vclock_to_string(&replica_clock), vclock_to_string(&vclock)); > > + /* > + * Replica clock is used in gc state and recovery > + * initialization, so we need to replace the remote 0-th > + * component with our own one. This doesn't break > + * recovery: it finds the WAL with a vclock strictly less > + * than replia clock in all components except the 0th one. > + * This leads to finding the correct WAL, if it exists, > + * since we do not need to recover local rows (the ones, > + * that contribute to the 0-th vclock component). > + * Note, that it would be bad to set 0-th vclock component > + * to a smaller value, since it would unnecessarily > + * require additional WALs, which may have already been > + * deleted. 1. But you just said above, that 0 component is ignored in all comparators anyway. So what is a problem of setting 0 component to 0? > + * Speaking of gc, remote instances' local vclock > + * components are not used by consumers at all. > + */ > + vclock_reset(&replica_clock, 0, vclock_get(&replicaset.vclock, 0)); > diff --git a/src/box/wal.c b/src/box/wal.c > index 3b094b0e8..a74bdecd9 100644 > --- a/src/box/wal.c > +++ b/src/box/wal.c > @@ -953,13 +953,19 @@ wal_assign_lsn(struct vclock *vclock_diff, struct vclock *base, > /** Assign LSN to all local rows. */ > for ( ; row < end; row++) { > if ((*row)->replica_id == 0) { > - (*row)->lsn = vclock_inc(vclock_diff, instance_id) + > - vclock_get(base, instance_id); > /* > - * Note, an anonymous replica signs local > - * rows whith a zero instance id. > + * All rows representing local space data > + * manipulations are signed wth a zero > + * instance id. This is also true for > + * anonymous replicas, since they are > + * only capable of writing to local and > + * temporary spaces. > */ > - (*row)->replica_id = instance_id; > + if ((*row)->group_id != GROUP_LOCAL) > + (*row)->replica_id = instance_id; > + > + (*row)->lsn = vclock_inc(vclock_diff, (*row)->replica_id) + > + vclock_get(base, (*row)->replica_id); 2. Bad indentation. vlock_get() should be aligned under vclock_inc(). > /* Use lsn of the first local row as transaction id. */ > tsn = tsn == 0 ? (*row)->lsn : tsn; > (*row)->tsn = tsn; 3. I run the new test without your patch, and it just hangs. No xloggaperror like in the ticket. Did it disappear thanks to the previous commit, about GC?