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 705D46ECE3; Wed, 3 Nov 2021 17:37:32 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 705D46ECE3 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1635950252; bh=Sehn/Zh6BBDvCyT6S3hzWMkS/IcPJGdKjhnK++JfqnM=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=LObr3zzboeymKUuYtMTdY+VfT6DVH7lR1TaxbPCdeuv5vYsLvnenlcsnTh4osJWs1 +x2vmv/20eGdT+QvAjm7/qW5Ds0QT28LDqA2plLrL8lF4Gd4YNJnxrqZXh3BUYn3/3 ELHQLxSMISeBGyeSbNYwrvtyk04G+9RcydsODRNU= Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [94.100.177.101]) (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 C41EC6ECE3 for ; Wed, 3 Nov 2021 17:37:30 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C41EC6ECE3 Received: by smtp41.i.mail.ru with esmtpa (envelope-from ) id 1miHOM-0004OQ-4F; Wed, 03 Nov 2021 17:37:30 +0300 Message-ID: <6a035fcc-a416-c3b3-5cd6-55c617e1c6fd@tarantool.org> Date: Wed, 3 Nov 2021 17:37:29 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.2.1 Content-Language: en-GB To: =?UTF-8?B?0K/QvSDQqNGC0YPQvdC00LXRgA==?= , tml References: <20211025104449.26693-1-ya.shtunder@gmail.com> <986cb3d1-64a9-87ec-8734-afe69c81c245@tarantool.org> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-4EC0790: 10 X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD9F9D976862A30D4FE71E4F4AD1F019DBD166E326202659E54182A05F538085040F2B5B19F2788038B5009918167E16155F196DF71BA6C85997F6B8365E363AA15 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE75210414551E8CD62EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637D8FE4B420C2EEC248638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8E2E6CE7F4902BBBE52C7B207C2AAA07F117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F44604297287769387670735201E561CDFBCA1751F2CC0D3CB04F14752D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE7B96B19DC409332149AF716F719AB83ED8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE3A6C7FFFE744CA7FB9735652A29929C6CC4224003CC836476EA7A3FFF5B025636E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F79006372A310894EFA5E9A9EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458834459D11680B505938063BEA80E6B72A7627F2A65545C38 X-C1DE0DAB: 0D63561A33F958A5FCC756991348516F3988585114EFD952B856FCF56FA14E29D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75C69C5C0DDE134364410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3490011A262ADAEDFEE912C9F5853082CF5C227FF058B624557F58D792F267F95DC34F82CF04892C5F1D7E09C32AA3244C8A0DC3E8945C702609118559FA85619235DA7DC5AF9B58C0729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj3JiVFN03mSU96+2mHSigpQ== X-Mailru-Sender: 11C2EC085EDE56FA38FD4C59F7EFE4072DBA40AC123CD0D24E84AA718232BC18A1BEF0FDE3FAD10F6BB2E709EA627F343C7DDD459B58856F0E45BC603594F5A135B915D4279FF0579437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v3] replication: the truncate method called from within a transaction 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: Serge Petrenko via Tarantool-patches Reply-To: Serge Petrenko Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 03.11.2021 13:03, Ян Штундер пишет: > Thanks for the comments! >  I corrected your remarks. Thanks for the changes! LGTM now. > > Diff: > > diff --git > a/test/replication-luatest/gh_6123_truncate_temp_space_test.lua > b/test/replication-luatest/gh_6123_truncate_temp_space_test.lua > new file mode 100644 > index 000000000..7940e722f > --- /dev/null > +++ b/test/replication-luatest/gh_6123_truncate_temp_space_test.lua > @@ -0,0 +1,62 @@ > +local t = require('luatest') > +local cluster = require('test.luatest_helpers.cluster') > +local helpers = require('test.luatest_helpers') > + > +local g = t.group('gh_6123', {{engine = 'memtx'}, {engine = 'vinyl'}}) > + > +g.before_each(function(cg) > +    local engine = cg.params.engine > + > +    cg.cluster = cluster:new({}) > + > +    local box_cfg = { > +        replication         = { > +            helpers.instance_uri('master') > +        }, > +        replication_timeout = 1, > +        read_only           = false > +    } > + > +    cg.master = cg.cluster:build_server({alias = 'master', engine = > engine, box_cfg = box_cfg}) > + > +    local box_cfg = { > +        replication         = { > +            helpers.instance_uri('master'), > +            helpers.instance_uri('replica') > +        }, > +        replication_timeout = 1, > +        replication_connect_timeout = 4, > +        read_only           = true > +    } > + > +    cg.replica = cg.cluster:build_server({alias = 'replica', engine = > engine, box_cfg = box_cfg}) > + > +    cg.cluster:add_server(cg.master) > +    cg.cluster:add_server(cg.replica) > +    cg.cluster:start() > +end) > + > + > +g.after_each(function(cg) > +    cg.cluster.servers = nil > +    cg.cluster:drop() > +end) > + > + > +g.test_truncate_is_local_transaction = function(cg) > +    cg.master:eval("s = box.schema.space.create('temp', {temporary = > true})") > +    cg.master:eval("s:create_index('pk')") > + > +    cg.master:eval("s:insert{1, 2}") > +    cg.master:eval("s:insert{4}") > +    t.assert_equals(cg.master:eval("return s:select()"), {{1, 2}, {4}}) > + > +    cg.master:eval("box.begin() box.space._schema:replace{'smth'} > s:truncate() box.commit()") > +    t.assert_equals(cg.master:eval("return s:select()"), {}) > +    t.assert_equals(cg.master:eval("return > box.space._schema:select{'smth'}"), {{'smth'}}) > + > +    -- Checking that replica has received the last transaction, > +    -- and that replication isn't broken. > +    t.assert_equals(cg.replica:eval("return > box.space._schema:select{'smth'}"), {{'smth'}}) > +    t.assert_equals(cg.replica:eval("return > box.info.replication[1].upstream.status"), 'follow') > +end > > -- > Yan Shtunder > > пт, 29 окт. 2021 г. в 12:20, Serge Petrenko : > > > > 28.10.2021 18:48, Ян Штундер пишет: > > Hi! Thank you for the review! > > I have fixed the errors > > > >     Please, find a better test name. > >     Like "gh_6123_truncate_temp_space_test.lua" > > > > > > gh_6123_test.lua -> gh_6123_truncate_temp_space_test.lua > > > > -- > > Yan Shtunder > > > > > > Thanks  for the fixes! > > One comment left. I'm pasting part of the patch here to review. > > In test: > > +g.after_each(function(cg) > +    cg.cluster.servers = nil > +    cg.cluster:drop() > +    fio.rmtree(cg.master.workdir) > +    fio.rmtree(cg.replica.workdir) > +end) > + > + > +g.before_test('test_truncate_is_local_transaction', function(cg) > +    cg.cluster:add_server(cg.master) > +    cg.cluster:add_server(cg.replica) > +    cg.cluster:start() > +end) > + > +g.after_test('test_truncate_is_local_transaction', function(cg) > +    cg.cluster:drop({cg.master, cg.replica}) > +end) > + > > You have a single test: test_truncate_is_local_transaction. > > So g.after_each and g.after_test both have the same meaning in > your case. > > I propose to dorp g.after_test, since g.after_each supersedes it. > > Also, you don't need to call fio.rmtree in g.after_each. > luatest does this for you, when needed. > > > > пн, 25 окт. 2021 г. в 16:34, Serge Petrenko > : > > > > > > > >     25.10.2021 13:44, Yan Shtunder via Tarantool-patches пишет: > >     > The truncate method could be called from within a transaction. > >     The flag > >     > of GROUP_LOCAL was set in truncate method after statement row > >     had been > >     > being checked on the GROUP_LOCAL. Accordingly, after a local > >     transaction > >     > NOP row was not appended. > >     > > >     > Closes #6123 > > > >     Thanks for the patch! > > > >     Please, find 2 comments below. > >     > --- > >     > Issue: https://github.com/tarantool/tarantool/issues/6123 > >     > Patch: > > > https://github.com/tarantool/tarantool/tree/yshtunder/gh-6123-truncate-is-local-transaction > >     > > >     >   src/box/alter.cc                          | 5 ++ > >     >   test/replication-luatest/gh_6123_test.lua | 62 > >     +++++++++++++++++++++++ > >     >   2 files changed, 67 insertions(+) > >     >   create mode 100644 test/replication-luatest/gh_6123_test.lua > >     > > >     > diff --git a/src/box/alter.cc b/src/box/alter.cc > >     > index 31ac82fc5..52b9ea4d2 100644 > >     > --- a/src/box/alter.cc > >     > +++ b/src/box/alter.cc > >     > @@ -2914,6 +2914,11 @@ on_replace_dd_truncate(struct trigger * > >     /* trigger */, void *event) > >     >       if (space_is_temporary(old_space) || > >     >           space_group_id(old_space) == GROUP_LOCAL) { > >     >               stmt->row->group_id = GROUP_LOCAL; > >     > +             /* > >     > +              * The trigger is invoked after > txn->n_local_rows > >     > +              * is counted, so don't forget to update it > here. > >     > +              */ > >     > +             ++txn->n_local_rows; > >     >       } > >     > > >     >       try { > > > >     The patch doesn't build on my machine. For the same reason > as the > >     other > >     patch. > >     I suppose you'll fix that with the other patch. > > > >     > diff --git a/test/replication-luatest/gh_6123_test.lua > >     b/test/replication-luatest/gh_6123_test.lua > >     > new file mode 100644 > >     > index 000000000..d0ffa0c8b > >     > --- /dev/null > >     > +++ b/test/replication-luatest/gh_6123_test.lua > > > >     Please, find a better test name. > >     Like "gh_6123_truncate_temp_space_test.lua" > >     > @@ -0,0 +1,62 @@ > >     > +local fio = require('fio') > >     > +local log = require('log') > >     > +local t = require('luatest') > >     > +local cluster = require('test.luatest_helpers.cluster') > >     > +local helpers = require('test.luatest_helpers.helpers') > >     > + > >     > +local g = t.group('gh-6123') > >     > + > >     > +g.before_test('test_truncate_is_local_transaction', > function() > >     > +    g.cluster = cluster:new({}) > >     > + > >     > +    local box_cfg = { > >     > +        replication         = { > >     > +            helpers.instance_uri('master') > >     > +        }, > >     > +        replication_timeout = 0.1, > >     > +        read_only           = false > >     > +    } > >     > + > >     > +    g.master = g.cluster:build_server({alias = 'master'}, > >     engine, box_cfg) > >     > + > >     > +    local box_cfg = { > >     > +        replication         = { > >     > +            helpers.instance_uri('master'), > >     > +            helpers.instance_uri('replica') > >     > +        }, > >     > +        replication_timeout = 0.1, > >     > +        replication_connect_timeout = 0.5, > >     > +        read_only           = true > >     > +    } > >     > + > >     > +    g.replica = g.cluster:build_server({alias = 'replica'}, > >     engine, box_cfg) > >     > + > >     > +    g.cluster:join_server(g.master) > >     > +    g.cluster:join_server(g.replica) > >     > +    g.cluster:start() > >     > + log.info ('Everything > is started') > >     > +end) > >     > + > >     > +g.after_test('test_truncate_is_local_transaction', function() > >     > +    g.cluster:stop() > >     > +    fio.rmtree(g.master.workdir) > >     > +    fio.rmtree(g.replica.workdir) > >     > +end) > >     > + > >     > +g.test_truncate_is_local_transaction = function() > >     > +    g.master:eval("s = box.schema.space.create('temp', > >     {temporary = true})") > >     > +    g.master:eval("s:create_index('pk')") > >     > + > >     > +    g.master:eval("s:insert{1, 2}") > >     > +    g.master:eval("s:insert{4}") > >     > +    t.assert_equals(g.master:eval("return s:select()"), {{1, > >     2}, {4}}) > >     > + > >     > +    g.master:eval("box.begin() > >     box.space._schema:replace{'smth'} s:truncate() box.commit()") > >     > +    t.assert_equals(g.master:eval("return s:select()"), {}) > >     > +    t.assert_equals(g.master:eval("return > >     box.space._schema:select{'smth'}"), {{'smth'}}) > >     > + > >     > +    -- Checking that replica has received the last > transaction, > >     > +    -- and that replication isn't broken. > >     > +    t.assert_equals(g.replica:eval("return > >     box.space._schema:select{'smth'}"), {{'smth'}}) > >     > +    t.assert_equals(g.replica:eval("return > >     box.info.replication[1].upstream.status"), 'follow') > >     > +end > >     > -- > >     > 2.25.1 > >     > > > > >     -- > >     Serge Petrenko > > > > -- > Serge Petrenko > -- Serge Petrenko