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

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

Thanks for the review!

I add Sergey Os. to the review as Sergey P. is on vacation.

>> 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");
>> +            return -1;
>>           }
>> ====================
>>
>> I couldn't test this check because of the bug in mvcc:
>> https://github.com/tarantool/tarantool/issues/6246
>>
>> ====================
> 
> I don't understand how this could happen
> (I mean the if branch being taken).
> 
> Would you mind explaining?

When MVCC will be fixed, it can happen that there are 2 transactions
trying to change _cluster.

If they add the same replica ID and the second transaction starts
before the first one is finished, it is going to see
replicaset.replica_by_id[new_id] != NULL from the first transaction.
Even before any of them is committed.

Hence it can't add its own new struct replica into the same place nor
can it touch the struct replica from the foreign transaction.

Therefore the only choice is to fail with an error.

The same would happen if MVCC is turned off - then space:insert()
would fail with duplicate tuple error right away due to dirty reads.

>> diff --git a/src/box/replication.h b/src/box/replication.h
>> index 57e0f10ae..5d1fa1255 100644
>> --- a/src/box/replication.h
>> +++ b/src/box/replication.h
>> @@ -360,6 +360,10 @@ replica_by_uuid(const struct tt_uuid *uuid);
>>   struct replica *
>>   replica_by_id(uint32_t replica_id);
>>   +/** Find the smallest free replica ID in the available range. */
> 
> 
> nit: free -> empty / unoccupied

Oxford dictionary says 'free' meanings are, among others,
"not busy", "not being used", "not blocked", "ready to give".
https://www.oxfordlearnersdictionaries.com/definition/english/free_1?q=free

Which means the current word is fine. But I don't mind to
change it:

====================
@@ -360,7 +360,7 @@ replica_by_uuid(const struct tt_uuid *uuid);
 struct replica *
 replica_by_id(uint32_t replica_id);
 
-/** Find the smallest free replica ID in the available range. */
+/** Find the smallest empty replica ID in the available range. */
 int
 replica_find_new_id(uint32_t *replica_id);