Re: [Tarantool-patches] [PATCH 1/1] replication: set replica ID before _cluster commit

[Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi! Thanks for the review!

>> On 25 Jul 2021, at 19:53, Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org> wrote:
>>
>> Replica registration works via looking for the smallest not
>> occupied ID in _cluster and inserting it into the space.
>>
>> It works not so good when mvcc is enabled. In particular, if more
>> than 1 replica try to register at the same time, they might get
>> the same replica_id because don't see changes of each other until
>> the registration in _cluster is complete.
>>
>> This in the end leads to all replicas failing the registration
>> except one with the 'duplicate key' error (primary index in
>> _cluster is replica ID).
>>
>> The patch makes the replicas occupy their ID before they commit it
>> into _cluster. And new replica ID search now uses the replica ID
>> map instead of _cluster iterator.
>>
>> This way the registration works like before - like MVCC does not
>> exist which is fine.
> 
> Did you discuss the MVCC capabilities - if we can address it there, by 
> setting a dedicated flag for this particular - and perhaps for some 
> other internal - space(s) to suppress MVCC ‘coverage’ for them. The 
> solution will be way more common with supposedly less hassle with 
> local structures, triggers, and so on?

I did think of that in the beginning, but then I decided it is a bad
idea. Firstly, from the side of unnecessary complication of MVCC which
is complex enough already. Secondly, _cluster DQL is going to see the
uncommitted changes despite MVCC turned on. Does not look well. In my
patch I made so that you do not see the tuples in :get() and :select()
in _cluster and you won't stumble into the "dirty" errors because you
are not supposed to make any :insert() nor :replace() into this space
manually. Automatic registration bypasses the busy IDs before trying
to insert them.

>> diff --git a/src/box/alter.cc b/src/box/alter.cc
>> index 89bb5946c..64ba09021 100644
>> --- a/src/box/alter.cc
>> +++ b/src/box/alter.cc
>> @@ -4280,14 +4244,34 @@ on_replace_dd_cluster(struct trigger *trigger, void *event)
>> 					  "updates of instance uuid");
>> 				return -1;
>> 			}
>> -		} else {
>> -			struct trigger *on_commit;
>> -			on_commit = txn_alter_trigger_new(register_replica,
>> -							  new_tuple);
>> -			if (on_commit == NULL)
>> -				return -1;
>> -			txn_stmt_on_commit(stmt, on_commit);
>> +			return 0;
>> +		}
>> +		/*
>> +		 * With read-views enabled there might be already a replica
>> +		 * whose registration is in progress in another transaction.
>> +		 * With the same replica ID.
>> +		 */
>> +		if (replica_by_id(replica_id) != NULL) {
>> +			diag_set(ClientError, ER_UNSUPPORTED, "Tarantool",
>> +				 "more than 1 replica with the same ID");
> 
> There should be more details on replica ID/UUID/name here, same as in unregister
> panic below.

The info would be visible in the logs anyway because before something
is added to _cluster, there are always more logs about 'joining ...',
'subscribed replica ...' and so on.

But I don't mind to extend the logs.

====================
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 64ba09021..390199298 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -4251,9 +4251,14 @@ on_replace_dd_cluster(struct trigger *trigger, void *event)
 		 * whose registration is in progress in another transaction.
 		 * With the same replica ID.
 		 */
-		if (replica_by_id(replica_id) != NULL) {
-			diag_set(ClientError, ER_UNSUPPORTED, "Tarantool",
-				 "more than 1 replica with the same ID");
+		struct replica *replica = replica_by_id(replica_id);
+		if (replica != NULL) {
+			const char *msg = tt_sprintf(
+				"more than 1 replica with the same ID %u: new "
+				"uuid - %s, old uuid - %s", replica_id,
+				tt_uuid_str(&replica_uuid),
+				tt_uuid_str(&replica->uuid));
+			diag_set(ClientError, ER_UNSUPPORTED, "Tarantool", msg);
 			return -1;
 		}
 		struct trigger *on_rollback = txn_alter_trigger_new(
@@ -4265,7 +4270,7 @@ on_replace_dd_cluster(struct trigger *trigger, void *event)
 		 * replica ID now. While WAL write is in progress, new replicas
 		 * might come, they should see the ID is already in use.
 		 */
-		struct replica *replica = replica_by_uuid(&replica_uuid);
+		replica = replica_by_uuid(&replica_uuid);
 		if (replica != NULL)
 			replica_set_id(replica, replica_id);
 		else
@@ -4283,6 +4288,10 @@ on_replace_dd_cluster(struct trigger *trigger, void *event)
 			return -1;
 		if (replica_check_id(replica_id) != 0)
 			return -1;
+		tt_uuid replica_uuid;
+		if (tuple_field_uuid(old_tuple, BOX_CLUSTER_FIELD_UUID,
+				    &replica_uuid) != 0)
+			return -1;
 
 		struct replica *replica = replica_by_id(replica_id);
 		if (replica == NULL) {
@@ -4292,7 +4301,15 @@ on_replace_dd_cluster(struct trigger *trigger, void *event)
 			 * subsystem is affected.
 			 */
 			panic("Tried to unregister a replica not stored in "
-			      "replica_by_id map, id is %u", replica_id);
+			      "replica_by_id map, id is %u, uuid is %s",
+			      replica_id, tt_uuid_str(&replica_uuid));
+		}
+		if (!tt_uuid_is_equal(&replica->uuid, &replica_uuid)) {
+			panic("Tried to unregister a replica with id %u, but "
+			      "its uuid is different from stored internally: "
+			      "in space - %s, internally - %s", replica_id,
+			      tt_uuid_str(&replica_uuid),
+			      tt_uuid_str(&replica->uuid));
 		}
 		/*
 		 * Unregister only after commit. Otherwise if the transaction
====================

>> diff --git a/src/box/box.cc b/src/box/box.cc
>> index 8c10a99dd..5c10aceff 100644
>> --- a/src/box/box.cc
>> +++ b/src/box/box.cc
>> @@ -2407,22 +2407,9 @@ box_on_join(const tt_uuid *instance_uuid)
>> 		return; /* nothing to do - already registered */
>>
>> 	box_check_writable_xc();
>> -
>> -	/** Find the largest existing replica id. */
>> -	struct space *space = space_cache_find_xc(BOX_CLUSTER_ID);
>> -	struct index *index = index_find_system_xc(space, 0);
>> -	struct iterator *it = index_create_iterator_xc(index, ITER_ALL,
>> -						       NULL, 0);
>> -	IteratorGuard iter_guard(it);
>> -	struct tuple *tuple;
>> -	/** Assign a new replica id. */
>> -	uint32_t replica_id = 1;
>> -	while ((tuple = iterator_next_xc(it)) != NULL) {
>> -		if (tuple_field_u32_xc(tuple,
>> -				       BOX_CLUSTER_FIELD_ID) != replica_id)
>> -			break;
>> -		replica_id++;
>> -	}
>> +	uint32_t replica_id;
>> +	if (replica_find_new_id(&replica_id) != 0)
>> +		diag_raise();
> 
> Any info on why register fails?

replica_find_new_id() sets diag with a proper error. This is
why I do diag_raise() here.