Re: [Tarantool-patches] [PATCH v5 4/4] box: start counting local space requests separately

[Serge Petrenko <sergepetrenko@tarantool.org>]

> 4 апр. 2020 г., в 23:51, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> написал(а):
> 
> Thanks for the patch!

Thanks for the review!

> 
> 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?

I didn’t. vclock[0] is ignored in gc_consumer objects now,
but when initializing recovery vclock[0] cannot be ignored.
Otherwise the instance itself would fail to recover local
rows on local recovery.
So relay recovery needs to be initialized with some valid vclock[0]
value.
Valid means «big enough to not require wals that have already been
deleted»

> 
>> +	 * 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().

Sorry, my bad. Fixed.

> 
>> 			/* 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?

Hmm, that’s an interesting bug (or a peculiarity?)
After deleting the old xlog replica is left with only one .snap file,
and doesn’t produce an error regarding «no wal between lsn x and y».
Master is left hanging until replica inserts some new data,
and opens a WAL which cannot be recovered from.

I worked it around.




diff --git a/src/box/wal.c b/src/box/wal.c
index a74bdecd9..1eb20272c 100644
--- a/src/box/wal.c
+++ b/src/box/wal.c
@@ -965,7 +965,7 @@ wal_assign_lsn(struct vclock *vclock_diff, struct vclock *base,
 				(*row)->replica_id = instance_id;
 
 			(*row)->lsn = vclock_inc(vclock_diff, (*row)->replica_id) +
-						 vclock_get(base, (*row)->replica_id);
+				      vclock_get(base, (*row)->replica_id);
 			/* Use lsn of the first local row as transaction id. */
 			tsn = tsn == 0 ? (*row)->lsn : tsn;
 			(*row)->tsn = tsn;
diff --git a/test/replication/gh-4114-local-space-replication.result b/test/replication/gh-4114-local-space-replication.result
index e524c9a1b..4a71bb6dc 100644
--- a/test/replication/gh-4114-local-space-replication.result
+++ b/test/replication/gh-4114-local-space-replication.result
@@ -76,10 +76,7 @@ box.space.test:insert{3}
  | ---
  | - [3]
  | ...
-box.snapshot()
- | ---
- | - ok
- | ...
+
 
 box.info.vclock[0]
  | ---
diff --git a/test/replication/gh-4114-local-space-replication.test.lua b/test/replication/gh-4114-local-space-replication.test.lua
index 26dccee68..114c592c5 100644
--- a/test/replication/gh-4114-local-space-replication.test.lua
+++ b/test/replication/gh-4114-local-space-replication.test.lua
@@ -26,7 +26,7 @@ box.snapshot()
 box.space.test:insert{2}
 box.snapshot()
 box.space.test:insert{3}
-box.snapshot()
+
 
 box.info.vclock[0]
 

--
Serge Petrenko
sergepetrenko@tarantool.org