From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 99ABB6EC55; Tue, 27 Jul 2021 01:06:37 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 99ABB6EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1627337197; bh=ZSLaBJV4SfF4BgPnft0nphgZaZBz3ei3y4BKajGLl0o=; h=To:References:Date:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=gopULlqQGzNcrKutKBJWjjJw4knszP0zP2NTC1BJ7SieiPWRYm6ogmyOP0aFq2RvZ tYAGGSwvwoWcQFipXlt/paryxkWavgA+kp7I2WFkys7Sh0ODuyztuezzu3DjtJ9D9R XuPNO3xiB7pZTrWWdFzPjHJ1fmXOE6em3/hn3hwQ= Received: from smtpng2.i.mail.ru (smtpng2.i.mail.ru [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 73C826EC55 for ; Tue, 27 Jul 2021 01:06:35 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 73C826EC55 Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1m88k6-0006gc-F9; Tue, 27 Jul 2021 01:06:34 +0300 To: Sergey Petrenko , tarantool-patches@dev.tarantool.org, gorcunov@gmail.com, Sergey Ostanevich References: Message-ID: <8274284e-52a2-6110-2344-b170ecfc3de6@tarantool.org> Date: Tue, 27 Jul 2021 00:06:33 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD941C43E597735A9C366EE405EC28A2001F8302D8429E0DE58182A05F538085040E2BE7BC1C59B7FED9D6EF7B9FE354D6E456FD297289C8BA692B80D4071956AC5 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7AA1605287C7F04D6EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006371758572763D318798638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D82E07B9B2AA0F7E42AC972E652FF98F23117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAE9A1BBD95851C5BA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F4460429728776938767073520140C956E756FBB7AF04B652EEC242312D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B613439FA09F3DCB32089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A514B76A81B6F330BC852B267DDF0CE26DA81BE9FEF9DEB7D6D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75FA7FF33AA1A4D21C410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D347341ACA13FB8BDD9B71FBD54F8BF6092C6515DE44F40C2920BA28A2C2CCF32E7C8312014A6A684E51D7E09C32AA3244CAFAFE43C24E2E17180DEF01D8EFE80C05595C85A795C7BAE927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojMEANdStWW5+iHmYHd28xuw== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D4D67760A6284BA6C1F269641CBCBBD1C3841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH 1/1] replication: set replica ID before _cluster commit X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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);