<HTML><BODY><div id="composeWebView_editable_content" data-mailruapp-compose-id="composeWebView_editable_content" style="text-align: left;"><div>Thanks for updates! LGTM.</div><div><br></div><div id="mail-app-auto-signature">
Best regards,<div>Sergos </div>
</div><br><br>Wednesday, 28 July 2021, 00:53 +0300 from Shpilevoy Vladislav <v.shpilevoy@tarantool.org>:<br><div id="composeWebView_previouse_content" data-mailruapp-compose-id="composeWebView_previouse_content"><blockquote id="mail-app-auto-quote" style="border-left-width: 1px; border-left-style: solid; border-left-color: rgb(11, 102, 249); margin: 10px 0px 10px 5px; padding: 0px 0px 0px 10px; display: inherit;" data-darkosha-id="1:3"><div class="js-helper js-readmsg-msg" data-darkosha-id="1:4" style="">
<style type="text/css" data-darkosha-id="1:5" style=""></style>
<div data-darkosha-id="1:6" style="">
<base target="_self" href="https://e.mail.ru/" data-darkosha-id="1:7" style="">
<div id="style_16274228261952089000_BODY" data-darkosha-id="1:8" style="">Hi! Thanks for the review!<br data-darkosha-id="1:9" style="">
<br data-darkosha-id="1:10" style="">
>> On 25 Jul 2021, at 19:53, Vladislav Shpilevoy via Tarantool-patches <<a href="mailto:tarantool-patches@dev.tarantool.org" data-darkosha-id="1:11" style="">tarantool-patches@dev.tarantool.org</a>> wrote:<br data-darkosha-id="1:12" style="">
>><br data-darkosha-id="1:13" style="">
>> Replica registration works via looking for the smallest not<br data-darkosha-id="1:14" style="">
>> occupied ID in _cluster and inserting it into the space.<br data-darkosha-id="1:15" style="">
>><br data-darkosha-id="1:16" style="">
>> It works not so good when mvcc is enabled. In particular, if more<br data-darkosha-id="1:17" style="">
>> than 1 replica try to register at the same time, they might get<br data-darkosha-id="1:18" style="">
>> the same replica_id because don't see changes of each other until<br data-darkosha-id="1:19" style="">
>> the registration in _cluster is complete.<br data-darkosha-id="1:20" style="">
>><br data-darkosha-id="1:21" style="">
>> This in the end leads to all replicas failing the registration<br data-darkosha-id="1:22" style="">
>> except one with the 'duplicate key' error (primary index in<br data-darkosha-id="1:23" style="">
>> _cluster is replica ID).<br data-darkosha-id="1:24" style="">
>><br data-darkosha-id="1:25" style="">
>> The patch makes the replicas occupy their ID before they commit it<br data-darkosha-id="1:26" style="">
>> into _cluster. And new replica ID search now uses the replica ID<br data-darkosha-id="1:27" style="">
>> map instead of _cluster iterator.<br data-darkosha-id="1:28" style="">
>><br data-darkosha-id="1:29" style="">
>> This way the registration works like before - like MVCC does not<br data-darkosha-id="1:30" style="">
>> exist which is fine.<br data-darkosha-id="1:31" style="">
> <br data-darkosha-id="1:32" style="">
> Did you discuss the MVCC capabilities - if we can address it there, by <br data-darkosha-id="1:33" style="">
> setting a dedicated flag for this particular - and perhaps for some <br data-darkosha-id="1:34" style="">
> other internal - space(s) to suppress MVCC ‘coverage’ for them. The <br data-darkosha-id="1:35" style="">
> solution will be way more common with supposedly less hassle with <br data-darkosha-id="1:36" style="">
> local structures, triggers, and so on?<br data-darkosha-id="1:37" style="">
<br data-darkosha-id="1:38" style="">
I did think of that in the beginning, but then I decided it is a bad<br data-darkosha-id="1:39" style="">
idea. Firstly, from the side of unnecessary complication of MVCC which<br data-darkosha-id="1:40" style="">
is complex enough already. Secondly, _cluster DQL is going to see the<br data-darkosha-id="1:41" style="">
uncommitted changes despite MVCC turned on. Does not look well. In my<br data-darkosha-id="1:42" style="">
patch I made so that you do not see the tuples in :get() and :select()<br data-darkosha-id="1:43" style="">
in _cluster and you won't stumble into the "dirty" errors because you<br data-darkosha-id="1:44" style="">
are not supposed to make any :insert() nor :replace() into this space<br data-darkosha-id="1:45" style="">
manually. Automatic registration bypasses the busy IDs before trying<br data-darkosha-id="1:46" style="">
to insert them.<br data-darkosha-id="1:47" style="">
<br data-darkosha-id="1:48" style="">
>> diff --git a/src/box/alter.cc b/src/box/alter.cc<br data-darkosha-id="1:49" style="">
>> index 89bb5946c..64ba09021 100644<br data-darkosha-id="1:50" style="">
>> --- a/src/box/alter.cc<br data-darkosha-id="1:51" style="">
>> +++ b/src/box/alter.cc<br data-darkosha-id="1:52" style="">
>> @@ -4280,14 +4244,34 @@ on_replace_dd_cluster(struct trigger *trigger, void *event)<br data-darkosha-id="1:53" style="">
>> "updates of instance uuid");<br data-darkosha-id="1:54" style="">
>> return -1;<br data-darkosha-id="1:55" style="">
>> }<br data-darkosha-id="1:56" style="">
>> - } else {<br data-darkosha-id="1:57" style="">
>> - struct trigger *on_commit;<br data-darkosha-id="1:58" style="">
>> - on_commit = txn_alter_trigger_new(register_replica,<br data-darkosha-id="1:59" style="">
>> - new_tuple);<br data-darkosha-id="1:60" style="">
>> - if (on_commit == NULL)<br data-darkosha-id="1:61" style="">
>> - return -1;<br data-darkosha-id="1:62" style="">
>> - txn_stmt_on_commit(stmt, on_commit);<br data-darkosha-id="1:63" style="">
>> + return 0;<br data-darkosha-id="1:64" style="">
>> + }<br data-darkosha-id="1:65" style="">
>> + /*<br data-darkosha-id="1:66" style="">
>> + * With read-views enabled there might be already a replica<br data-darkosha-id="1:67" style="">
>> + * whose registration is in progress in another transaction.<br data-darkosha-id="1:68" style="">
>> + * With the same replica ID.<br data-darkosha-id="1:69" style="">
>> + */<br data-darkosha-id="1:70" style="">
>> + if (replica_by_id(replica_id) != NULL) {<br data-darkosha-id="1:71" style="">
>> + diag_set(ClientError, ER_UNSUPPORTED, "Tarantool",<br data-darkosha-id="1:72" style="">
>> + "more than 1 replica with the same ID");<br data-darkosha-id="1:73" style="">
> <br data-darkosha-id="1:74" style="">
> There should be more details on replica ID/UUID/name here, same as in unregister<br data-darkosha-id="1:75" style="">
> panic below.<br data-darkosha-id="1:76" style="">
<br data-darkosha-id="1:77" style="">
The info would be visible in the logs anyway because before something<br data-darkosha-id="1:78" style="">
is added to _cluster, there are always more logs about 'joining ...',<br data-darkosha-id="1:79" style="">
'subscribed replica ...' and so on.<br data-darkosha-id="1:80" style="">
<br data-darkosha-id="1:81" style="">
But I don't mind to extend the logs.<br data-darkosha-id="1:82" style="">
<br data-darkosha-id="1:83" style="">
====================<br data-darkosha-id="1:84" style="">
diff --git a/src/box/alter.cc b/src/box/alter.cc<br data-darkosha-id="1:85" style="">
index 64ba09021..390199298 100644<br data-darkosha-id="1:86" style="">
--- a/src/box/alter.cc<br data-darkosha-id="1:87" style="">
+++ b/src/box/alter.cc<br data-darkosha-id="1:88" style="">
@@ -4251,9 +4251,14 @@ on_replace_dd_cluster(struct trigger *trigger, void *event)<br data-darkosha-id="1:89" style="">
* whose registration is in progress in another transaction.<br data-darkosha-id="1:90" style="">
* With the same replica ID.<br data-darkosha-id="1:91" style="">
*/<br data-darkosha-id="1:92" style="">
- if (replica_by_id(replica_id) != NULL) {<br data-darkosha-id="1:93" style="">
- diag_set(ClientError, ER_UNSUPPORTED, "Tarantool",<br data-darkosha-id="1:94" style="">
- "more than 1 replica with the same ID");<br data-darkosha-id="1:95" style="">
+ struct replica *replica = replica_by_id(replica_id);<br data-darkosha-id="1:96" style="">
+ if (replica != NULL) {<br data-darkosha-id="1:97" style="">
+ const char *msg = tt_sprintf(<br data-darkosha-id="1:98" style="">
+ "more than 1 replica with the same ID %u: new "<br data-darkosha-id="1:99" style="">
+ "uuid - %s, old uuid - %s", replica_id,<br data-darkosha-id="1:100" style="">
+ tt_uuid_str(&replica_uuid),<br data-darkosha-id="1:101" style="">
+ tt_uuid_str(&replica->uuid));<br data-darkosha-id="1:102" style="">
+ diag_set(ClientError, ER_UNSUPPORTED, "Tarantool", msg);<br data-darkosha-id="1:103" style="">
return -1;<br data-darkosha-id="1:104" style="">
}<br data-darkosha-id="1:105" style="">
struct trigger *on_rollback = txn_alter_trigger_new(<br data-darkosha-id="1:106" style="">
@@ -4265,7 +4270,7 @@ on_replace_dd_cluster(struct trigger *trigger, void *event)<br data-darkosha-id="1:107" style="">
* replica ID now. While WAL write is in progress, new replicas<br data-darkosha-id="1:108" style="">
* might come, they should see the ID is already in use.<br data-darkosha-id="1:109" style="">
*/<br data-darkosha-id="1:110" style="">
- struct replica *replica = replica_by_uuid(&replica_uuid);<br data-darkosha-id="1:111" style="">
+ replica = replica_by_uuid(&replica_uuid);<br data-darkosha-id="1:112" style="">
if (replica != NULL)<br data-darkosha-id="1:113" style="">
replica_set_id(replica, replica_id);<br data-darkosha-id="1:114" style="">
else<br data-darkosha-id="1:115" style="">
@@ -4283,6 +4288,10 @@ on_replace_dd_cluster(struct trigger *trigger, void *event)<br data-darkosha-id="1:116" style="">
return -1;<br data-darkosha-id="1:117" style="">
if (replica_check_id(replica_id) != 0)<br data-darkosha-id="1:118" style="">
return -1;<br data-darkosha-id="1:119" style="">
+ tt_uuid replica_uuid;<br data-darkosha-id="1:120" style="">
+ if (tuple_field_uuid(old_tuple, BOX_CLUSTER_FIELD_UUID,<br data-darkosha-id="1:121" style="">
+ &replica_uuid) != 0)<br data-darkosha-id="1:122" style="">
+ return -1;<br data-darkosha-id="1:123" style="">
<br data-darkosha-id="1:124" style="">
struct replica *replica = replica_by_id(replica_id);<br data-darkosha-id="1:125" style="">
if (replica == NULL) {<br data-darkosha-id="1:126" style="">
@@ -4292,7 +4301,15 @@ on_replace_dd_cluster(struct trigger *trigger, void *event)<br data-darkosha-id="1:127" style="">
* subsystem is affected.<br data-darkosha-id="1:128" style="">
*/<br data-darkosha-id="1:129" style="">
panic("Tried to unregister a replica not stored in "<br data-darkosha-id="1:130" style="">
- "replica_by_id map, id is %u", replica_id);<br data-darkosha-id="1:131" style="">
+ "replica_by_id map, id is %u, uuid is %s",<br data-darkosha-id="1:132" style="">
+ replica_id, tt_uuid_str(&replica_uuid));<br data-darkosha-id="1:133" style="">
+ }<br data-darkosha-id="1:134" style="">
+ if (!tt_uuid_is_equal(&replica->uuid, &replica_uuid)) {<br data-darkosha-id="1:135" style="">
+ panic("Tried to unregister a replica with id %u, but "<br data-darkosha-id="1:136" style="">
+ "its uuid is different from stored internally: "<br data-darkosha-id="1:137" style="">
+ "in space - %s, internally - %s", replica_id,<br data-darkosha-id="1:138" style="">
+ tt_uuid_str(&replica_uuid),<br data-darkosha-id="1:139" style="">
+ tt_uuid_str(&replica->uuid));<br data-darkosha-id="1:140" style="">
}<br data-darkosha-id="1:141" style="">
/*<br data-darkosha-id="1:142" style="">
* Unregister only after commit. Otherwise if the transaction<br data-darkosha-id="1:143" style="">
====================<br data-darkosha-id="1:144" style="">
<br data-darkosha-id="1:145" style="">
>> diff --git a/src/box/box.cc b/src/box/box.cc<br data-darkosha-id="1:146" style="">
>> index 8c10a99dd..5c10aceff 100644<br data-darkosha-id="1:147" style="">
>> --- a/src/box/box.cc<br data-darkosha-id="1:148" style="">
>> +++ b/src/box/box.cc<br data-darkosha-id="1:149" style="">
>> @@ -2407,22 +2407,9 @@ box_on_join(const tt_uuid *instance_uuid)<br data-darkosha-id="1:150" style="">
>> return; /* nothing to do - already registered */<br data-darkosha-id="1:151" style="">
>><br data-darkosha-id="1:152" style="">
>> box_check_writable_xc();<br data-darkosha-id="1:153" style="">
>> -<br data-darkosha-id="1:154" style="">
>> - /** Find the largest existing replica id. */<br data-darkosha-id="1:155" style="">
>> - struct space *space = space_cache_find_xc(BOX_CLUSTER_ID);<br data-darkosha-id="1:156" style="">
>> - struct index *index = index_find_system_xc(space, 0);<br data-darkosha-id="1:157" style="">
>> - struct iterator *it = index_create_iterator_xc(index, ITER_ALL,<br data-darkosha-id="1:158" style="">
>> - NULL, 0);<br data-darkosha-id="1:159" style="">
>> - IteratorGuard iter_guard(it);<br data-darkosha-id="1:160" style="">
>> - struct tuple *tuple;<br data-darkosha-id="1:161" style="">
>> - /** Assign a new replica id. */<br data-darkosha-id="1:162" style="">
>> - uint32_t replica_id = 1;<br data-darkosha-id="1:163" style="">
>> - while ((tuple = iterator_next_xc(it)) != NULL) {<br data-darkosha-id="1:164" style="">
>> - if (tuple_field_u32_xc(tuple,<br data-darkosha-id="1:165" style="">
>> - BOX_CLUSTER_FIELD_ID) != replica_id)<br data-darkosha-id="1:166" style="">
>> - break;<br data-darkosha-id="1:167" style="">
>> - replica_id++;<br data-darkosha-id="1:168" style="">
>> - }<br data-darkosha-id="1:169" style="">
>> + uint32_t replica_id;<br data-darkosha-id="1:170" style="">
>> + if (replica_find_new_id(&replica_id) != 0)<br data-darkosha-id="1:171" style="">
>> + diag_raise();<br data-darkosha-id="1:172" style="">
> <br data-darkosha-id="1:173" style="">
> Any info on why register fails?<br data-darkosha-id="1:174" style="">
<br data-darkosha-id="1:175" style="">
replica_find_new_id() sets diag with a proper error. This is<br data-darkosha-id="1:176" style="">
why I do diag_raise() here.</div>
<base target="_self" href="https://e.mail.ru/" data-darkosha-id="1:177" style="">
</div>
</div></blockquote></div></div></BODY></HTML>