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 CD8D06F3C8; Sat, 27 Mar 2021 20:34:39 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org CD8D06F3C8 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1616866479; bh=cPYVg6SfpEqCiXJBnp1k5iJuuAHVQMVyiOHXFqxM4AQ=; 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=xtWTvg7EE9ewGa3cUojhPG+nqX8c5eLHfxyB0mPQVAqtJJj/HS2GyPu7wr0516sP9 YLS9u0eRdtVHXjucY/5zdgpFwz5zYReR5GMcQ1MWRYnG+/0zzmvZPYvkWILAAEu5oP +3bOiMcd42jxSHAtWk1wPy2/YZJNlzJpmsuRaGYo= Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (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 D4FBB6F3C8 for ; Sat, 27 Mar 2021 20:34:37 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org D4FBB6F3C8 Received: by smtp34.i.mail.ru with esmtpa (envelope-from ) id 1lQCpZ-0003a5-1V; Sat, 27 Mar 2021 20:34:37 +0300 To: Vladislav Shpilevoy , gorcunov@gmail.com Cc: tarantool-patches@dev.tarantool.org References: <2e081c11-ed0c-506a-af8a-57ef6707f7a9@tarantool.org> Message-ID: <0fee8e8f-45b1-10d5-56fb-1c860b8bc514@tarantool.org> Date: Sat, 27 Mar 2021 20:34:36 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.16; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: <2e081c11-ed0c-506a-af8a-57ef6707f7a9@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD9ED7173E37F4E32941B7C4A78AC10F96A7797F60C25BD4B06182A05F538085040E1C68AC2F1956B14A8571CEF74C48C169F8679D2A072340BDA4FE56EB8DECDFF X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE74323F140F3EE5B6AC2099A533E45F2D0395957E7521B51C2CFCAF695D4D8E9FCEA1F7E6F0F101C6778DA827A17800CE7A33E1178EA603666EA1F7E6F0F101C67CDEEF6D7F21E0D1D174C73DBBBFC76644B9776E5C9D404B3193BF6FD24BA9BEFA6B605DF7F9F4FCD389733CBF5DBD5E913377AFFFEAFD269A417C69337E82CC2CC7F00164DA146DAFE8445B8C89999729449624AB7ADAF37F6B57BC7E64490611E7FA7ABCAF51C92A417C69337E82CC2CC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB8D32BA5DBAC0009BE9E8FC8737B5C22493DCBAA9B946E255976E601842F6C81A12EF20D2F80756B5F7E9C4E3C761E06A776E601842F6C81A127C277FBC8AE2E8B29285E2B17063EF73AA81AA40904B5D9DBF02ECDB25306B2201CA6A4E26CD07C3BBE47FD9DD3FB595F5C1EE8F4F765FCA83251EDC214901ED5E8D9A59859A8B6ACFD5AC65D14EB01089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2368A440D3B0F6089093C9A16E5BC824AC8B6CDF511875BC4E8F7B195E1C97831CE4587F69497DD3E7A82484DD75C3D90 X-C1DE0DAB: 0D63561A33F958A5C933AE47512C59BF9652BCD8B8F49B742DC0996CC182A1D7D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34CAFBC0A7A4BEEE01FE81B1B4E2D93086934A2DABC4803F84D7514A9B397DF35BDFA9434B7BC2DF081D7E09C32AA3244CF103EE3EA3AEF5C78578053CC0869B13F2F5F14F68F1805BFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojhfg4BOnpz0oGWz96HbUumQ== X-Mailru-Sender: 583F1D7ACE8F49BDD2846D59FC20E9F89C448697B9EDE911DB2971E89D969B3A6CF69EAD92D2B995424AE0EB1F3D1D21E2978F233C3FAE6EE63DB1732555E4A8EE80603BA4A5B0BC112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v2 3/7] applier: extract plain tx application from applier_apply_tx() 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" 26.03.2021 23:47, Vladislav Shpilevoy пишет: > Thanks for the patch! > > See 4 comments below. Thanks for the review! > > On 24.03.2021 13:24, Serge Petrenko wrote: >> The new routine, called apply_plain_tx(), may be used not only by >> applier_apply_tx(), but also by final join, once we make it >> transactional, and recovery, once it's also turned transactional. >> >> Also, while we're at it. Remove excess fiber_gc() call from >> applier_subscribe loop. Let's better make sure fiber_gc() is called on >> any return from applier_apply_tx(). >> >> Prerequisite #5874 >> Part of #5566 >> --- >> src/box/applier.cc | 188 ++++++++++++++++++++++----------------------- >> 1 file changed, 93 insertions(+), 95 deletions(-) >> >> diff --git a/src/box/applier.cc b/src/box/applier.cc >> index 65afa5e98..07e557a51 100644 >> --- a/src/box/applier.cc >> +++ b/src/box/applier.cc >> @@ -905,6 +905,90 @@ applier_handle_raft(struct applier *applier, struct xrow_header *row) >> return box_raft_process(&req, applier->instance_id); >> } >> >> +static inline int >> +apply_plain_tx(struct stailq *rows, bool skip_conflict, bool use_triggers) >> +{ >> + /** > 1. Inside of functions for comment first line we use /*, not /**. Sure, fixed. > >> + * Explicitly begin the transaction so that we can >> + * control fiber->gc life cycle and, in case of apply >> + * conflict safely access failed xrow object and allocate >> + * IPROTO_NOP on gc. >> + */ >> + struct txn *txn = txn_begin(); >> + struct applier_tx_row *item; >> + if (txn == NULL) >> + return -1; >> + >> + stailq_foreach_entry(item, rows, next) { >> + struct xrow_header *row = &item->row; >> + int res = apply_row(row); >> + if (res != 0 && skip_conflict) { >> + struct error *e = diag_last_error(diag_get()); >> + /* >> + * In case of ER_TUPLE_FOUND error and enabled >> + * replication_skip_conflict configuration >> + * option, skip applying the foreign row and >> + * replace it with NOP in the local write ahead >> + * log. >> + */ >> + if (e->type == &type_ClientError && >> + box_error_code(e) == ER_TUPLE_FOUND && >> + replication_skip_conflict) { > 2. That looks kind of confusing - you pass skip_conflict option but > also use replication_skip_conflict. You could calculate skip_conflict > based on replication_skip_conflict in your patch. Yes, indeed. Thanks for noticing! > >> + diag_clear(diag_get()); >> + row->type = IPROTO_NOP; >> + row->bodycnt = 0; >> + res = apply_row(row); >> + } >> + } >> + if (res != 0) >> + goto fail; >> + } >> + >> + /* >> + * We are going to commit so it's a high time to check if >> + * the current transaction has non-local effects. >> + */ >> + if (txn_is_distributed(txn)) { >> + /* >> + * A transaction mixes remote and local rows. >> + * Local rows must be replicated back, which >> + * doesn't make sense since the master likely has >> + * new changes which local rows may overwrite. >> + * Raise an error. >> + */ >> + diag_set(ClientError, ER_UNSUPPORTED, "Replication", >> + "distributed transactions"); >> + goto fail; >> + } >> + >> + if (use_triggers) { >> + /* We are ready to submit txn to wal. */ >> + struct trigger *on_rollback, *on_wal_write; >> + size_t size; >> + on_rollback = region_alloc_object(&txn->region, typeof(*on_rollback), >> + &size); >> + on_wal_write = region_alloc_object(&txn->region, typeof(*on_wal_write), >> + &size); >> + if (on_rollback == NULL || on_wal_write == NULL) { >> + diag_set(OutOfMemory, size, "region_alloc_object", >> + "on_rollback/on_wal_write"); >> + goto fail; >> + } >> + >> + trigger_create(on_rollback, applier_txn_rollback_cb, NULL, NULL); >> + txn_on_rollback(txn, on_rollback); >> + >> + trigger_create(on_wal_write, applier_txn_wal_write_cb, NULL, NULL); >> + txn_on_wal_write(txn, on_wal_write); >> + } >> + >> + return txn_commit_try_async(txn); >> +fail: >> + txn_rollback(txn); >> + return -1; >> +} >> @@ -974,103 +1058,18 @@ applier_apply_tx(struct applier *applier, struct stailq *rows) >> assert(first_row == last_row); >> if (apply_synchro_row(first_row) != 0) >> diag_raise(); > 3. Hm. Isn't it a bug that we raise an error here, but don't unlock the > latch and don't call fiber_gc()? Looks like a separate bug. Could you > fix it please, and probably with a test? Can it be related to the > hang you fix in the previous commit? It is a bug, yes. Will fix in a commit on top. It's not related to the hang we spoke of in the previous letter though. > >> - goto success; >> - } >> - >> - /** >> - * Explicitly begin the transaction so that we can >> - * control fiber->gc life cycle and, in case of apply >> - * conflict safely access failed xrow object and allocate >> - * IPROTO_NOP on gc. >> - */ >> - struct txn *txn; >> - txn = txn_begin(); >> - struct applier_tx_row *item; >> - if (txn == NULL) { >> - latch_unlock(latch); >> - return -1; >> - } >> - stailq_foreach_entry(item, rows, next) { >> - struct xrow_header *row = &item->row; >> - int res = apply_row(row); >> - if (res != 0) { >> - struct error *e = diag_last_error(diag_get()); >> - /* >> - * In case of ER_TUPLE_FOUND error and enabled >> - * replication_skip_conflict configuration >> - * option, skip applying the foreign row and >> - * replace it with NOP in the local write ahead >> - * log. >> - */ >> - if (e->type == &type_ClientError && >> - box_error_code(e) == ER_TUPLE_FOUND && >> - replication_skip_conflict) { >> - diag_clear(diag_get()); >> - row->type = IPROTO_NOP; >> - row->bodycnt = 0; >> - res = apply_row(row); >> - } >> - } >> - if (res != 0) >> - goto rollback; >> - } >> - /* >> - * We are going to commit so it's a high time to check if >> - * the current transaction has non-local effects. >> - */ >> - if (txn_is_distributed(txn)) { >> - /* >> - * A transaction mixes remote and local rows. >> - * Local rows must be replicated back, which >> - * doesn't make sense since the master likely has >> - * new changes which local rows may overwrite. >> - * Raise an error. >> - */ >> - diag_set(ClientError, ER_UNSUPPORTED, >> - "Replication", "distributed transactions"); >> - goto rollback; >> + goto written; >> } >> >> - /* We are ready to submit txn to wal. */ >> - struct trigger *on_rollback, *on_wal_write; >> - size_t size; >> - on_rollback = region_alloc_object(&txn->region, typeof(*on_rollback), >> - &size); >> - on_wal_write = region_alloc_object(&txn->region, typeof(*on_wal_write), >> - &size); >> - if (on_rollback == NULL || on_wal_write == NULL) { >> - diag_set(OutOfMemory, size, "region_alloc_object", >> - "on_rollback/on_wal_write"); >> - goto rollback; >> + if ((rc = apply_plain_tx(rows, true, true)) == 0) { >> +written: >> + vclock_follow(&replicaset.applier.vclock, last_row->replica_id, >> + last_row->lsn); >> } >> - >> - trigger_create(on_rollback, applier_txn_rollback_cb, NULL, NULL); >> - txn_on_rollback(txn, on_rollback); >> - >> - trigger_create(on_wal_write, applier_txn_wal_write_cb, NULL, NULL); >> - txn_on_wal_write(txn, on_wal_write); >> - >> - if (txn_commit_try_async(txn) < 0) >> - goto fail; >> - >> -success: >> - /* >> - * The transaction was sent to journal so promote vclock. >> - * >> - * Use the lsn of the last row to guard from 1.10 >> - * instances, which send every single tx row as a separate >> - * transaction. >> - */ >> - vclock_follow(&replicaset.applier.vclock, last_row->replica_id, >> - last_row->lsn); >> - latch_unlock(latch); >> - return 0; >> -rollback: >> - txn_rollback(txn); >> -fail: >> +no_write: > 4. You go to this label even when write was done. Maybe rename to > 'end' or 'finish'? > > Consider this diff: > > ==================== > @@ -1027,7 +1027,7 @@ applier_apply_tx(struct applier *applier, struct stailq *rows) > latch_lock(latch); > if (vclock_get(&replicaset.applier.vclock, > last_row->replica_id) >= last_row->lsn) { > - goto no_write; > + goto finish; > } else if (vclock_get(&replicaset.applier.vclock, > first_row->replica_id) >= first_row->lsn) { > /* > @@ -1058,15 +1058,12 @@ applier_apply_tx(struct applier *applier, struct stailq *rows) > assert(first_row == last_row); > if (apply_synchro_row(first_row) != 0) > diag_raise(); > - goto written; > + } else if ((rc = apply_plain_tx(rows, true, true)) != 0) { > + goto finish; > } > - > - if ((rc = apply_plain_tx(rows, true, true)) == 0) { > -written: > - vclock_follow(&replicaset.applier.vclock, last_row->replica_id, > - last_row->lsn); > - } > -no_write: > + vclock_follow(&replicaset.applier.vclock, last_row->replica_id, > + last_row->lsn); > +finish: > latch_unlock(latch); > fiber_gc(); > return rc; > ==================== Looks good, applied. Incremental diff below. ======================================== diff --git a/src/box/applier.cc b/src/box/applier.cc index f396e43a8..e6d9673dd 100644 --- a/src/box/applier.cc +++ b/src/box/applier.cc @@ -908,7 +908,7 @@ applier_handle_raft(struct applier *applier, struct xrow_header *row)  static inline int  apply_plain_tx(struct stailq *rows, bool skip_conflict, bool use_triggers)  { -       /** +       /*          * Explicitly begin the transaction so that we can          * control fiber->gc life cycle and, in case of apply          * conflict safely access failed xrow object and allocate @@ -932,8 +932,7 @@ apply_plain_tx(struct stailq *rows, bool skip_conflict, bool use_triggers)                          * log.                          */                         if (e->type == &type_ClientError && -                           box_error_code(e) == ER_TUPLE_FOUND && -                           replication_skip_conflict) { +                           box_error_code(e) == ER_TUPLE_FOUND) {                                 diag_clear(diag_get());                                 row->type = IPROTO_NOP;                                 row->bodycnt = 0; @@ -1027,7 +1026,7 @@ applier_apply_tx(struct applier *applier, struct stailq *rows)         latch_lock(latch);         if (vclock_get(&replicaset.applier.vclock,                        last_row->replica_id) >= last_row->lsn) { -               goto no_write; +               goto finish;         } else if (vclock_get(&replicaset.applier.vclock,                               first_row->replica_id) >= first_row->lsn) {                 /* @@ -1058,15 +1057,13 @@ applier_apply_tx(struct applier *applier, struct stailq *rows)                 assert(first_row == last_row);                 if (apply_synchro_row(first_row) != 0)                         diag_raise(); -               goto written; -       } - -       if ((rc = apply_plain_tx(rows, true, true)) == 0) { -written: -               vclock_follow(&replicaset.applier.vclock, last_row->replica_id, -                             last_row->lsn); +       } else if ((rc = apply_plain_tx(rows, replication_skip_conflict, +                                       true)) != 0) { +               goto finish;         } -no_write: +       vclock_follow(&replicaset.applier.vclock, last_row->replica_id, +                     last_row->lsn); +finish:         latch_unlock(latch);         fiber_gc();         return rc; ======================================== > >> latch_unlock(latch); >> fiber_gc(); >> - return -1; >> + return rc; >> } -- Serge Petrenko