Re: [Tarantool-patches] [PATCH v2 7/7] replication: do not ignore replica vclock on register

[Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

26.03.2021 23:50, Vladislav Shpilevoy пишет:
> Thanks for the patch!
>
> See 2 comments below.
>
> On 24.03.2021 13:24, Serge Petrenko wrote:
>> There was a bug in box_process_register. It decoded replica's vclock but
>> never used it when sending the registration stream. So the replica might
>> lose the data in range (replica_vclock, start_vclock).
> 1. Could you please add a test?

Yes, sure.

>
>> Follow-up #5566
>> ---
>>   src/box/box.cc | 14 ++++++--------
>>   1 file changed, 6 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/box/box.cc b/src/box/box.cc
>> index 292a54213..0bcb505a8 100644
>> --- a/src/box/box.cc
>> +++ b/src/box/box.cc
>> @@ -2154,7 +2154,8 @@ box_process_register(struct ev_io *io, struct xrow_header *header)
>>   			  "wal_mode = 'none'");
>>   	}
>>   
>> -	struct gc_consumer *gc = gc_consumer_register(&replicaset.vclock,
>> +	vclock_reset(&replica_vclock, 0, vclock_get(&replicaset.vclock, 0));
> 2. xrow_decode_register_xc() already returns the vclock with empty 0
> part. Why do you need this reset?

It sets vclock[0] to master's vclock[0]. But it's not needed here, indeed.
Thanks for noticing!

I thought having vclock[0] = 0 would harm gc somehow, but that's not true.

>
>> +	struct gc_consumer *gc = gc_consumer_register(&replica_vclock,
>>   				"replica %s", tt_uuid_str(&instance_uuid));
>>   	if (gc == NULL)
>>   		diag_raise();

=================================

diff --git a/src/box/box.cc b/src/box/box.cc
index 0bcb505a8..ecec8df27 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -2154,7 +2154,6 @@ box_process_register(struct ev_io *io, struct 
xrow_header *header)
                "wal_mode = 'none'");
      }

-    vclock_reset(&replica_vclock, 0, vclock_get(&replicaset.vclock, 0));
      struct gc_consumer *gc = gc_consumer_register(&replica_vclock,
                  "replica %s", tt_uuid_str(&instance_uuid));
      if (gc == NULL)
diff --git a/test/replication/anon_register_gap.result 
b/test/replication/anon_register_gap.result
new file mode 100644
index 000000000..24a3548c8
--- /dev/null
+++ b/test/replication/anon_register_gap.result
@@ -0,0 +1,116 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+
+--
+-- When master's registering an anonymous replica, it might ignore the 
replica's
+-- current vclock, and skip the data in range (replica_clock, 
master_clock).
+--
+box.schema.user.grant('guest', 'replication')
+ | ---
+ | ...
+_ = box.schema.space.create('test')
+ | ---
+ | ...
+_ = box.space.test:create_index('pk')
+ | ---
+ | ...
+
+test_run:cmd('create server replica with rpl_master=default,\
+              script="replication/anon1.lua"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('start server replica')
+ | ---
+ | - true
+ | ...
+
+test_run:wait_lsn('replica', 'default')
+ | ---
+ | ...
+box.error.injection.set('ERRINJ_RELAY_SEND_DELAY', true)
+ | ---
+ | - ok
+ | ...
+
+box.space.test:insert{1}
+ | ---
+ | - [1]
+ | ...
+
+test_run:switch('replica')
+ | ---
+ | - true
+ | ...
+
+test_run:wait_upstream(1, {status='disconnected'})
+ | ---
+ | - true
+ | ...
+box.space.test:select{}
+ | ---
+ | - []
+ | ...
+
+fiber = require('fiber')
+ | ---
+ | ...
+f = fiber.new(function() box.cfg{replication_anon=false} end)
+ | ---
+ | ...
+test_run:wait_upstream(1, {status='register'})
+ | ---
+ | - true
+ | ...
+
+test_run:switch('default')
+ | ---
+ | - true
+ | ...
+box.error.injection.set('ERRINJ_RELAY_SEND_DELAY', false)
+ | ---
+ | - ok
+ | ...
+box.space.test:insert{2}
+ | ---
+ | - [2]
+ | ...
+
+test_run:switch('replica')
+ | ---
+ | - true
+ | ...
+test_run:wait_lsn('replica', 'default')
+ | ---
+ | ...
+f:status()
+ | ---
+ | - dead
+ | ...
+box.space.test:select{}
+ | ---
+ | - - [1]
+ |   - [2]
+ | ...
+
+-- Cleanup
+test_run:switch('default')
+ | ---
+ | - true
+ | ...
+test_run:cmd('stop server replica')
+ | ---
+ | - true
+ | ...
+test_run:cmd('delete server replica')
+ | ---
+ | - true
+ | ...
+box.space.test:drop()
+ | ---
+ | ...
+box.schema.user.revoke('guest', 'replication')
+ | ---
+ | ...
diff --git a/test/replication/anon_register_gap.test.lua 
b/test/replication/anon_register_gap.test.lua
new file mode 100644
index 000000000..c71576a23
--- /dev/null
+++ b/test/replication/anon_register_gap.test.lua
@@ -0,0 +1,43 @@
+test_run = require('test_run').new()
+
+--
+-- When master's registering an anonymous replica, it might ignore the 
replica's
+-- current vclock, and skip the data in range (replica_clock, 
master_clock).
+--
+box.schema.user.grant('guest', 'replication')
+_ = box.schema.space.create('test')
+_ = box.space.test:create_index('pk')
+
+test_run:cmd('create server replica with rpl_master=default,\
+              script="replication/anon1.lua"')
+test_run:cmd('start server replica')
+
+test_run:wait_lsn('replica', 'default')
+box.error.injection.set('ERRINJ_RELAY_SEND_DELAY', true)
+
+box.space.test:insert{1}
+
+test_run:switch('replica')
+
+test_run:wait_upstream(1, {status='disconnected'})
+box.space.test:select{}
+
+fiber = require('fiber')
+f = fiber.new(function() box.cfg{replication_anon=false} end)
+test_run:wait_upstream(1, {status='register'})
+
+test_run:switch('default')
+box.error.injection.set('ERRINJ_RELAY_SEND_DELAY', false)
+box.space.test:insert{2}
+
+test_run:switch('replica')
+test_run:wait_lsn('replica', 'default')
+f:status()
+box.space.test:select{}
+
+-- Cleanup
+test_run:switch('default')
+test_run:cmd('stop server replica')
+test_run:cmd('delete server replica')
+box.space.test:drop()
+box.schema.user.revoke('guest', 'replication')
diff --git a/test/replication/suite.cfg b/test/replication/suite.cfg
index aea8b6157..aff5fda26 100644
--- a/test/replication/suite.cfg
+++ b/test/replication/suite.cfg
@@ -1,5 +1,6 @@
  {
      "anon.test.lua": {},
+    "anon_register_gap.test.lua": {},
      "gh-2991-misc-asserts-on-update.test.lua": {},
      "gh-3111-misc-rebootstrap-from-ro-master.test.lua": {},
      "gh-3160-misc-heartbeats-on-master-changes.test.lua": {},
diff --git a/test/replication/suite.ini b/test/replication/suite.ini
index fc161700a..a9e44e8cf 100644
--- a/test/replication/suite.ini
+++ b/test/replication/suite.ini
@@ -3,7 +3,7 @@ core = tarantool
  script =  master.lua
  description = tarantool/box, replication
  disabled = consistent.test.lua
-release_disabled = catch.test.lua errinj.test.lua gc.test.lua 
gc_no_space.test.lua before_replace.test.lua qsync_advanced.test.lua 
qsync_errinj.test.lua quorum.test.lua recover_missing_xlog.test.lua 
sync.test.lua long_row_timeout.test.lua gh-4739-vclock-assert.test.lua 
gh-4730-applier-rollback.test.lua gh-5140-qsync-casc-rollback.test.lua 
gh-5144-qsync-dup-confirm.test.lua gh-5167-qsync-rollback-snap.test.lua 
gh-5506-election-on-off.test.lua gh-5536-wal-limit.test.lua 
hang_on_synchro_fail.test.lua
+release_disabled = catch.test.lua errinj.test.lua gc.test.lua 
gc_no_space.test.lua before_replace.test.lua qsync_advanced.test.lua 
qsync_errinj.test.lua quorum.test.lua recover_missing_xlog.test.lua 
sync.test.lua long_row_timeout.test.lua gh-4739-vclock-assert.test.lua 
gh-4730-applier-rollback.test.lua gh-5140-qsync-casc-rollback.test.lua 
gh-5144-qsync-dup-confirm.test.lua gh-5167-qsync-rollback-snap.test.lua 
gh-5506-election-on-off.test.lua gh-5536-wal-limit.test.lua 
hang_on_synchro_fail.test.lua anon_register_gap.test.lua
  config = suite.cfg
  lua_libs = lua/fast_replica.lua lua/rlimit.lua
  use_unix_sockets = True

-- 
Serge Petrenko