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 30D7D6EC5E; Fri, 9 Apr 2021 01:57:04 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 30D7D6EC5E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1617922624; bh=9Yf57qrPapwskgg0eknQFnA+JySGOvG65chuQD0B6hg=; 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=TN+E1GA2F9nXVjvopRXH0iMyQ69GDIuSlCazt8PjRlw/gBf9nOX8/L2tsmeoXlqBv aZF0bcplQePqH81JnEjLi9gWTyuOBZkhvznV1Srx3bnEgd0fFL4sj7DpBpfuX749MB pi1XIC6E1MsM8OJSrj2r8bRhWeUyTUa4mhbB+sb4= Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 489016EC5D for ; Fri, 9 Apr 2021 01:57:01 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 489016EC5D Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1lUda8-00005U-DB; Fri, 09 Apr 2021 01:57:00 +0300 To: Serge Petrenko , tarantool-patches@dev.tarantool.org, gorcunov@gmail.com References: <00c39bc5e3090383a5f0e732b5a1f32130191cf1.1617835503.git.v.shpilevoy@tarantool.org> <71ee3907-a8fd-d916-fc6a-3205e66f2d29@tarantool.org> Message-ID: Date: Fri, 9 Apr 2021 00:56:59 +0200 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.9.0 MIME-Version: 1.0 In-Reply-To: <71ee3907-a8fd-d916-fc6a-3205e66f2d29@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD92FFCB8E6708E7480257C85EA0BB7A95D0F00AE41BB9A5343182A05F53808504078ACE6AEDA7D833BC36E1127C20C86AC103CB822E88413C765AE1580BD0E5786 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE71B1B5393BB9C1313EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637EEFC5FE85C3736658638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B265DEB8D26733E61EE099EBE99FABB7D785184F9B92705903D2E47CDBA5A96583C09775C1D3CA48CFCA5A41EBD8A3A0199FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C327ED053E960B195E117882F4460429728AD0CFFFB425014E868A13BD56FB6657E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F7900637BBEA499411984DA1EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2AD77751E876CB595E8F7B195E1C97831E1D38DBB9BDDBA4292B5ACE33AB3463B X-C1DE0DAB: 0D63561A33F958A5647881EDAC06F2B487345102201CF921C7EB95ED5B25C511D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34431D0341F6B74DD3AB3D0A9EDFA5368513E1B616516B244F47FE1A6E28F1A1D1D2A73E0B575A86A01D7E09C32AA3244CA7026BDB30F02D9D3522FAEC5840DAD9795D98D676DD64D0FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojyO2lHpuZu4QrUq3d4HdXGw== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822573D36A9F9B723A001D33EF0BB983F4D3841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH 1/1] applier: process synchro rows after WAL write 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" Hi! Thanks for the review! > Thanks for the patch! > > I'm a bit worried about two different synchro rows coming from two > appliers. Is everything ok in this case? > Or even normal rows coming from other appliers. Say some other replica > has already applied this synchro row and even has written some rows on > top of it. Its applier won't block on replica_id latch, and may fail to apply > some txs following this synchro row, because it's not yet written to WAL > and thus not applied (limbo is still not empty or belongs to other instance). Should not happen, if I understood you correctly. See my response in the other email + a test for test-run + a console test. >> diff --git a/src/box/applier.cc b/src/box/applier.cc >> index 971b2e64c..f7c82548c 100644 >> --- a/src/box/applier.cc >> +++ b/src/box/applier.cc >> @@ -762,26 +762,21 @@ applier_txn_wal_write_cb(struct trigger *trigger, void *event) >>   } >>     struct synchro_entry { >> -    /** Encoded form of a synchro record. */ >> -    struct synchro_body_bin    body_bin; >> - >> -    /** xrow to write, used by the journal engine. */ >> -    struct xrow_header row; >> - >> +    /** Request to process when WAL write is done. */ >> +    struct synchro_request *req; >> +    /** Fiber created the entry. To wakeup when WAL write is done. */ >> +    struct fiber *owner; >>       /** >> -     * The journal entry itself. Note since >> -     * it has unsized array it must be the >> -     * last entry in the structure. >> +     * The base journal entry. It has unsized array and then must be the >> +     * last entry in the structure. But can workaround it via a union >> +     * adding the needed tail as char[]. >>        */ >> -    struct journal_entry journal_entry; >> +    union { >> +        struct journal_entry base; >> +        char base_buf[sizeof(base) + sizeof(base.rows[0])]; >> +    }; >>   }; > > I don't understand this union stuff. > The journal_entry is the last entry in synchro_entry anyway. > Is this a hack for allowing to allocate synchro_entry on the stack? Yes. Otherwise I would need a more ugly hack: struct synchro_entry *e; alignas(alignof(*e)) char e_buf[sizeof(*e) + sizeof(e->base.rows[0])]; e = (struct synchro_entry *)e_buf; Then I managed to create the union hack, which I find less ugly, and it seems to be allow to declare it on the stack. The interesting thing though is that I still can't make that union not a last member. I also see now there is a problem in one of the CI jobs: https://github.com/tarantool/tarantool/runs/2291961909 It complains about out of bound array access. Which has nothing to do with the union, just a C++ issue I suppose. I fixed it in the new version, see the other email. >> diff --git a/test/replication/gh-5213-qsync-applier-order.test.lua b/test/replication/gh-5213-qsync-applier-order.test.lua >> new file mode 100644 >> index 000000000..3a32626f2 >> --- /dev/null >> +++ b/test/replication/gh-5213-qsync-applier-order.test.lua >> @@ -0,0 +1,129 @@ >> +-- >> +-- gh-5213: applier used to process CONFIRM/ROLLBACK before writing them to WAL. >> +-- As a result it could happen that the transactions became visible on CONFIRM, >> +-- then somehow weren't written to WAL, and on restart the data might not be >> +-- visible again. Which means rollback of confirmed data and is not acceptable >> +-- (on the contrary with commit after rollback). >> +-- >> +test_run = require('test_run').new() >> +fiber = require('fiber') >> +old_synchro_quorum = box.cfg.replication_synchro_quorum >> +old_synchro_timeout = box.cfg.replication_synchro_timeout >> + >> +box.schema.user.grant('guest', 'super') >> + >> +s = box.schema.space.create('test', {is_sync = true}) >> +_ = s:create_index('pk') >> + >> +test_run:cmd('create server replica with rpl_master=default,\ >> +              script="replication/gh-5213-replica.lua"') >> +test_run:cmd('start server replica') >> + >> +test_run:switch('replica') >> +assert(box.info.id == 2) >> +lsn = box.info.vclock[1] >> + >> +test_run:switch('default') >> +fiber = require('fiber') >> +box.cfg{                                                                        \ >> +    replication_synchro_quorum = 3,                                             \ >> +    replication_synchro_timeout = 1000,                                         \ >> +} >> +f = fiber.new(function() s:replace{1} end) >> + >> +test_run:switch('replica') >> +-- Wait when the transaction is written to WAL. >> +test_run:wait_cond(function() return box.info.vclock[1] == lsn + 1 end) > > This shouldn't go wrong, but I find > test_run:wait_lsn('replica, 'default') more durable. > And you wouldn't need to save lsn above then. > > Same for other usages of lsn = ... and wait_cond(box.info.vclock[1] == lsn + ...) > > Up to you though. I totally forgot about wait_lsn(), thanks for noticing. I applied it. See the full diff in the second email.