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 AAC646FC8D; Sun, 24 Oct 2021 19:01:50 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org AAC646FC8D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1635091310; bh=JQfDf2WCW93XxGK4msTvpJxF1DPKuUA5wD2gBBOOGnA=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=Vr2F6Scr0ufVF7AdmrA7aytuoUAV68fPwLKKNgL0GVZx11NUyOap5XJhQaasC+h85 YoUyz7xlE1AXyF60mKI89R9dVvAwfPyBkW9VYJedVnL2h+SUaI2am1DMlrnXzJrd4x HNhP5m6G2kTHawuZjOhbo7Fslm1GTrw+Y5NdJdDI= Received: from mail-lf1-f47.google.com (mail-lf1-f47.google.com [209.85.167.47]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 1F8516FC8D for ; Sun, 24 Oct 2021 19:01:50 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 1F8516FC8D Received: by mail-lf1-f47.google.com with SMTP id u21so7874444lff.8 for ; Sun, 24 Oct 2021 09:01:50 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=0U2PzKC7QJ4IQdFpyx/3lgiTT+Zphd9zvypVeo9XD1s=; b=3eX9UN0fAUZHbtKyYM/Gs0B+kzpBKPMOKSNeVTTf7T0Yv7Xx9+fRySVPYQtBg/TT/Y Clvyggbcvs7BJJjerYIUpsgHg8QU5FdwG+ixwc8duKp2ZQ0mpFUG9VjvyqOr74FiFRm2 HBlf1iuaLBO4sLlsgS1HA+1OXkbWwvwAeXPGQG2q4y7R6qn1kDanWcoYFf4kkn+lvFC+ 7U8pY2ATwRntbQEQII5Z6iv4kuGSnHZ/pZdw6GZdgwjN47HWRMF5SwrWJhsL9mTv0uhS +EdH9LKL954BtxGVi1w7v+ayLDxXrY8WV94mOHqV2unGt0RIHIm6Tr5YZh+/aLGXttGv pxAA== X-Gm-Message-State: AOAM530tNe4l14Icy9Pmc7sGuAg4uo66Tg6c0ywYhuiEC6Vjb/v00Fqz ROagspIMm6yWOft3RfXUMZxplbWvSbXDWg== X-Google-Smtp-Source: ABdhPJx3zufhJWDqsBkhIYtqybzpbLLDw5O5U2DnK/i1av0FIuDCXg249GAqxhp1nUIsxZp1wUiLcg== X-Received: by 2002:ac2:4c34:: with SMTP id u20mr12312241lfq.112.1635091308983; Sun, 24 Oct 2021 09:01:48 -0700 (PDT) Received: from grain.localdomain ([5.18.251.97]) by smtp.gmail.com with ESMTPSA id m23sm1363761lfh.129.2021.10.24.09.01.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 24 Oct 2021 09:01:48 -0700 (PDT) Received: by grain.localdomain (Postfix, from userid 1000) id 530325A0020; Sun, 24 Oct 2021 19:01:47 +0300 (MSK) Date: Sun, 24 Oct 2021 19:01:47 +0300 To: Vladislav Shpilevoy Cc: tml Message-ID: References: <20211014215622.49732-1-gorcunov@gmail.com> <20211014215622.49732-4-gorcunov@gmail.com> <31fa78f8-c01b-27be-83a8-8eb9119a837a@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <31fa78f8-c01b-27be-83a8-8eb9119a837a@tarantool.org> User-Agent: Mutt/2.0.7 (2021-05-04) Subject: Re: [Tarantool-patches] [PATCH v23 3/3] test: add gh-6036-qsync-order test 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: Cyrill Gorcunov via Tarantool-patches Reply-To: Cyrill Gorcunov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" On Sun, Oct 24, 2021 at 05:39:34PM +0200, Vladislav Shpilevoy wrote: > >> I added this diff and all the tests passed. That means the only > >> tested parts of the patchset are apply_synchro_row() and > >> txn_limbo_is_replica_outdated(). > >> > > > > Without the rest of locking the overall picture becomes incomplete, > > we introduce the first part of split brain detection. And in second > > part we'are about to introduce filtering into begin()/commit() pair. > > Sorry, but how is my comment above related to filtering? You introduced > some code which is dead - it is not tested at all. That was my only > point here. Vlad, these locking calls are not dead, they are not covered by tests because they will be so in next part, where we introduce the filtering. But these lockings are taken and running. At least that is what I've had in mind. But I see your point, thanks! > > It is not related to filtering - the ordering issue is separate as > you yourself noted when this was all a single patchset with the > split-brain detection. This is even proved by the test you added > here. I just need more tests for the other places which use the locking > now. > > >> Please, lets try not to submit more untested code. It is enough that > >> we already have one ticket which simply adds assert(false) into a few > >> places and the tests pass. It does not make the code better, it just > >> adds more uncertainty how it works and whether it works at all. > > > > This is the correct way to guard access to some particular data to me. > > In this case we're guarding @promote_term_map and @promote_greatest_term. > > What you propose is to guard them in "some path" but leave untouched for > > all others. So the person which would read this code after some years > > gonna be scratching a head trying to figure out why sometimes access to > > these members is done locked and somtimes are not. I completely disagree > > here. Either we lock all accesses either not, not partial lockings please. > > Well, the only thing which I am worrying right now about is that if this > person in a few years will drop almost the entirety of your patchset > wondering whether it is needed really, all the tests will pass. OK, I got your worrying. Lets see if we manage to settle it down. > > Also you seem to misunderstand my proposal. I didn't propose to make less > locking, I only proposed to increase it. I thought it will be easier to do > with full blocking of certain functions because it might be just easier to > get something locked in the tests. The bigger the critical section is - the > easier to hit it. Or maybe fine-grained locking works better if you say so. > Don't know. > > It will only affect performance. Perf of qsync/raft now is 0, because > it is not used at all, due to being unsafe. Because of the reordering and > of the split-brain. This patch can't say it makes the reordering stuff much > better, because there are no tests except for a single case of reordering. > > If from my diff you understood that I proposed to this drop code - sorry, > I didn't mean this. I meant that your patch should test these places, not > drop them. By commenting this code out I tried to prove that this code is > dead now, and that it shouldn't be so. OK, I see. Gimme some time, I'll try various ways. Thanks for comments, Vlad! Cyrill