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 36DA56EC55; Wed, 25 Aug 2021 15:45:29 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 36DA56EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1629895529; bh=CNZTraXwEQKsK+G3EtNlNEnf74KEHIpjvMlpGhKiiSE=; h=To:Cc:References:Date:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=q3UvSJxPAGO4FVanmdGiaIsYGc4cyapGgdZWylauzHNiIqidy/x39HwX14ouVCRVU gwqd3RL8+rWYPD44YtV/D43ngaB1ZRvKYypusFYI/zLi4DWvcATgHjDPRqYII2i97B TTW0w4vwdRa/qmxh5+G+aw6/eIfQv5D+N93A/QLw= Received: from smtp61.i.mail.ru (smtp61.i.mail.ru [217.69.128.41]) (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 79B0C6EC55 for ; Wed, 25 Aug 2021 15:45:27 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 79B0C6EC55 Received: by smtp61.i.mail.ru with esmtpa (envelope-from ) id 1mIsHW-0000oJ-JT; Wed, 25 Aug 2021 15:45:27 +0300 To: Yan Shtunder Cc: tarantool-patches@dev.tarantool.org References: <20210825055451.115351-1-ya.shtunder@gmail.com> Message-ID: <0f8586e4-d5df-f720-de75-4af9f3fb0a3b@tarantool.org> Date: Wed, 25 Aug 2021 15:45:25 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: <20210825055451.115351-1-ya.shtunder@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD9F9A2272A1D086A28553D1D5C4B4124EF182A05F5380850407EF30E280390250E82A68F1109B7C77655DB59229172A70ECE791BBC5CB73E98 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7B8387EA37EC1BE7DEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637E5A8E5ECB3FF24018638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8F8C12EE33A84CD8C0134C02C0CA01AE9117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC2EE5AD8F952D28FBA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352026055571C92BF10FF6B57BC7E6449061A352F6E88A58FB86F5D81C698A659EA73AA81AA40904B5D9A18204E546F3947CE3786DD2C77EBDAA302FCEF25BFAB3454AD6D5ED66289B52698AB9A7B718F8C46E0066C2D8992A16725E5C173C3A84C3881E3AC70F3270D9BA3038C0950A5D36B5C8C57E37DE458B0BC6067A898B09E46D1867E19FE14079C09775C1D3CA48CF3D321E7403792E342EB15956EA79C166A417C69337E82CC275ECD9A6C639B01B78DA827A17800CE75B51C8FB0C3E748C731C566533BA786AA5CC5B56E945C8DA X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458834459D11680B5056AB06F7AD63949106A09717C6BBA9569 X-C1DE0DAB: 0D63561A33F958A5266BBA36D3FBD53372021A4C8ACD4A3A9420D9B8EF5A509ED59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA753177526CD55AFC11410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D344F2126FF337FA651B13210ED84BDC039EFB209CC7995EE40F3C406B70F41BC21CD3E8CEF4BD7CEBF1D7E09C32AA3244CF19631D6844E361001C3A86A789A47D7F165894D92D62706729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj4xwK62BfeftflmkSzKaO8Q== X-Mailru-Sender: 583F1D7ACE8F49BD31DE23046B3A8460F9E4D52401B5938AC7965B2F1FF7587BF599040F3E2389946BB2E709EA627F343C7DDD459B58856F0E45BC603594F5A135B915D4279FF0579437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH] 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" 25.08.2021 08:54, Yan Shtunder пишет: > 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 > --- Hi! Thanks for the patch! Please, find a couple of 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/txn.c | 16 +++-- > ...-6123-truncate-is-local-transaction.result | 69 +++++++++++++++++++ > ...123-truncate-is-local-transaction.test.lua | 27 ++++++++ > 3 files changed, 107 insertions(+), 5 deletions(-) > create mode 100644 test/replication/gh-6123-truncate-is-local-transaction.result > create mode 100644 test/replication/gh-6123-truncate-is-local-transaction.test.lua > > diff --git a/src/box/txn.c b/src/box/txn.c > index b80e722a4..d37bec7ae 100644 > --- a/src/box/txn.c > +++ b/src/box/txn.c > @@ -397,6 +397,7 @@ txn_is_distributed(struct txn *txn) > int > txn_commit_stmt(struct txn *txn, struct request *request) > { > + int rc = 0; > assert(txn->in_sub_stmt > 0); > /* > * Run on_replace triggers. For now, disallow mutation > @@ -415,9 +416,6 @@ txn_commit_stmt(struct txn *txn, struct request *request) > assert(stmt->row != NULL); > if (stmt->row->replica_id == 0) { > ++txn->n_new_rows; > - if (stmt->row->group_id == GROUP_LOCAL) > - ++txn->n_local_rows; > - > } else { > ++txn->n_applier_rows; > } Why can't you move this whole block lower, not just the if() body? I mean both if() {} and else {} > @@ -447,10 +445,18 @@ txn_commit_stmt(struct txn *txn, struct request *request) > */ > stmt->does_require_old_tuple = true; > > - if(trigger_run(&stmt->space->on_replace, txn) != 0) > - goto fail; > + rc = trigger_run(&stmt->space->on_replace, txn); > } > } > + > + if (stmt->row != NULL && stmt->row->replica_id == 0 && > + stmt->row->group_id == GROUP_LOCAL) { > + ++txn->n_local_rows; > + } > + > + if (rc != 0) > + goto fail; > + > --txn->in_sub_stmt; > return 0; This could be written simpler: if (rc != 0) {      // the code below the "fail" label goes here } else {     --txn->in_sub_stmt; } return rc; > fail: > diff --git a/test/replication/gh-6123-truncate-is-local-transaction.result b/test/replication/gh-6123-truncate-is-local-transaction.result > new file mode 100644 > index 000000000..0a4b990c1 > --- /dev/null > +++ b/test/replication/gh-6123-truncate-is-local-transaction.result > @@ -0,0 +1,69 @@ > +-- test-run result file version 2 > +test_run = require('test_run').new() > + | --- > + | ... > + > + > +-- Step 1 > +box.schema.user.grant("guest", "replication") > + | --- > + | ... > +s = box.schema.space.create("temp", {temporary=true}) > + | --- > + | ... > +_ = s:create_index('pk') > + | --- > + | ... > +s:insert{1,2} > + | --- > + | - [1, 2] > + | ... > +s:insert{4} > + | --- > + | - [4] > + | ... > +s:select() > + | --- > + | - - [1, 2] > + | - [4] > + | ... > + > + > +-- Step 2 > +test_run:cmd('create server replica with rpl_master=default,\ > + script="replication/replica.lua"') > + | --- > + | - true > + | ... > +test_run:cmd('start server replica') > + | --- > + | - true > + | ... > + > + > +-- Step 3 > +box.begin() box.space._schema:replace{"smth"} s:truncate() box.commit() > + | --- > + | ... > +s:select() > + | --- > + | - [] > + | ... > +box.space._schema:select{'smth'} > + | --- > + | - - ['smth'] > + | ... > + > + > +test_run:cmd('stop server replica') > + | --- > + | - true > + | ... > +test_run:cmd('cleanup server replica') > + | --- > + | - true > + | ... > +test_run:cmd('delete server replica') > + | --- > + | - true > + | ... > diff --git a/test/replication/gh-6123-truncate-is-local-transaction.test.lua b/test/replication/gh-6123-truncate-is-local-transaction.test.lua > new file mode 100644 > index 000000000..b68514d0b > --- /dev/null > +++ b/test/replication/gh-6123-truncate-is-local-transaction.test.lua > @@ -0,0 +1,27 @@ > +test_run = require('test_run').new() > + > + > +-- Step 1 > +box.schema.user.grant("guest", "replication") > +s = box.schema.space.create("temp", {temporary=true}) > +_ = s:create_index('pk') > +s:insert{1,2} > +s:insert{4} > +s:select() > + > + > +-- Step 2 > +test_run:cmd('create server replica with rpl_master=default,\ > + script="replication/replica.lua"') > +test_run:cmd('start server replica') > + > + > +-- Step 3 > +box.begin() box.space._schema:replace{"smth"} s:truncate() box.commit() > +s:select() > +box.space._schema:select{'smth'} > + > + > +test_run:cmd('stop server replica') > +test_run:cmd('cleanup server replica') > +test_run:cmd('delete server replica') > -- > 2.25.1 > The test looks good with one comment: Before stopping the replica, you should check that replica has received the last transaction, and that replication isn't broken. -- Serge Petrenko