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 214626FC8F; Fri, 16 Apr 2021 10:08:05 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 214626FC8F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1618556885; bh=Ptqax7xFKHxvPS0OJg9s38j3rZLg8YXi80WEcKwxGpM=; 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=VL3nVW+iq94Pk1ftxYcLaIVKoy7X3V8afNDc6yxSKETwf4MbPMp20uu3lNRM5igi+ Q3edzm5GG7BTBU7y0ZNRVph2154Bef1inSa8RakQu9c+EsnocBX9bTDeqWiVTvG5FC PieZWNJmJTVAX5QBjhIuUrSayK1dOlm7CimbGEfo= Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (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 B77186FC8F for ; Fri, 16 Apr 2021 10:08:03 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B77186FC8F Received: by smtp49.i.mail.ru with esmtpa (envelope-from ) id 1lXIaA-0006cc-GM; Fri, 16 Apr 2021 10:08:02 +0300 To: Vladislav Shpilevoy , gorcunov@gmail.com Cc: tarantool-patches@dev.tarantool.org References: <32099031-d261-5887-538a-86d770a15617@tarantool.org> Message-ID: <75ca15d4-9614-12e3-6871-10f36831a78a@tarantool.org> Date: Fri, 16 Apr 2021 10:08:01 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.9.1 MIME-Version: 1.0 In-Reply-To: <32099031-d261-5887-538a-86d770a15617@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92FFCB8E6708E74809299FB6B3996B874F289A033C44AD400182A05F5380850402F36506FA24EB2DC8599E73AFC618E26928A9232833EEC72F73F7E6825240836 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE78C722B68A3D10D1CEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006372FD0E46BD6FDBDB0EA1F7E6F0F101C67CDEEF6D7F21E0D1D9295C2E9FA3191EE1B59CA4C82EFA658D9247AC1FEA9039314146A2CA518CD7DF6B57BC7E64490618DEB871D839B73339E8FC8737B5C224952D31B9D28593E51CC7F00164DA146DAFE8445B8C89999729449624AB7ADAF37F6B57BC7E64490611E7FA7ABCAF51C92176DF2183F8FC7C08794E14F7ADDB10D8941B15DA834481F9449624AB7ADAF37BA3038C0950A5D3613377AFFFEAFD2691661749BA6B977359291332E984E6D627B076A6E789B0E97A8DF7F3B2552694A1E7802607F20496D49FD398EE364050FB28585415E75ADA9A7B8E9D6D956BB52B3661434B16C20AC78D18283394535A9E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6D0C9BB9AE6BD5D69089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5E4A2A1AE80A192DA43AFC7E585C765D323118CC25CE714EED59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34EE6683D9E546CC6274CB97385BD0CF26B99AEA68DFADCA5E84EA8E831127C6991DFB80F06E8022F51D7E09C32AA3244C1631437E1C857CE5F457E25BC704EAF2853296C06374E602FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj3S6P1v0GIqTLy/Y0iNNpUw== X-Mailru-Sender: 583F1D7ACE8F49BDD2846D59FC20E9F8B073372D3D55269CB6D5A0CD42046C6D0779D808319B100D424AE0EB1F3D1D21E2978F233C3FAE6EE63DB1732555E4A8EE80603BA4A5B0BC112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v3 01/10] wal: enrich row's meta information with sync replication flags 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" 16.04.2021 02:18, Vladislav Shpilevoy пишет: > Good job on the patch! > > See 3 comments below. Hi! Thanks for the review! >> diff --git a/src/box/journal.h b/src/box/journal.h >> index 76c70c19f..3ce9c869e 100644 >> --- a/src/box/journal.h >> +++ b/src/box/journal.h >> @@ -63,6 +63,7 @@ struct journal_entry { >> * A journal entry completion callback argument. >> */ >> void *complete_data; >> + uint8_t opt_flags; > 1. I propose to call them just flags. There is no a third value > like 'no flag'. They are either set or not, am I right? Also the > member is missing a comment. The most important thing to say - > these flags are only for the last row. Ok, fixed. > >> /** >> * Asynchronous write completion function. >> */ >> @@ -97,6 +98,7 @@ journal_entry_create(struct journal_entry *entry, size_t n_rows, >> entry->approx_len = approx_len; >> entry->n_rows = n_rows; >> entry->res = -1; >> + entry->opt_flags = 0; > 2. You could initialize it with IPROTO_FLAG_COMMIT right here and > drop (*row)->is_commit = true from wal_assign_lsn. But this one up > to you. Maybe it is not a good idea. This would look better, indeed, but neither journal nor wal know about iproto constants. And I don't think it's a good idea to introduce such a dependency. I can add entry->flags |= IPROTO_FLAG_COMMIT to txn_journal_entry_new(). I actually like how this turned out. It's none of WAL's or journal's business which row is commit and which isn't. > >> } >> >> /** >> diff --git a/src/box/wal.c b/src/box/wal.c >> index 34af0bda6..4ec8034a3 100644 >> --- a/src/box/wal.c >> +++ b/src/box/wal.c >> @@ -962,14 +962,14 @@ out: >> */ >> static void >> wal_assign_lsn(struct vclock *vclock_diff, struct vclock *base, >> - struct xrow_header **row, >> - struct xrow_header **end) >> + struct journal_entry *entry) > 3. This part could be a separate commit, otherwise it is hard to > see the functional changes. Up to you if you want to split. Good idea, let's do that. Incremental diff for this commit is below and the extracted commit regarding wal_assign_lsn() refactoring is in reply to this email. >> { >> int64_t tsn = 0; >> - struct xrow_header **start = row; >> - struct xrow_header **first_glob_row = row; >> + struct xrow_header **start = entry->rows; >> + struct xrow_header **end = entry->rows + entry->n_rows; >> + struct xrow_header **first_glob_row = entry->rows; >> /** Assign LSN to all local rows. */ >> - for ( ; row < end; row++) { >> + for (struct xrow_header **row = start; row < end; row++) { >> if ((*row)->replica_id == 0) { >> /* >> * All rows representing local space data ======================================================== diff --git a/src/box/journal.h b/src/box/journal.h index 3ce9c869e..8f3d56a61 100644 --- a/src/box/journal.h +++ b/src/box/journal.h @@ -63,7 +63,8 @@ struct journal_entry {       * A journal entry completion callback argument.       */      void *complete_data; -    uint8_t opt_flags; +    /** Flags that should be set for the last entry row. */ +    uint8_t flags;      /**       * Asynchronous write completion function.       */ @@ -98,7 +99,7 @@ journal_entry_create(struct journal_entry *entry, size_t n_rows,      entry->approx_len    = approx_len;      entry->n_rows        = n_rows;      entry->res        = -1; -    entry->opt_flags    = 0; +    entry->flags        = 0;  }  /** diff --git a/src/box/txn.c b/src/box/txn.c index e090d58fc..31f664aa0 100644 --- a/src/box/txn.c +++ b/src/box/txn.c @@ -76,7 +76,7 @@ txn_add_redo(struct txn *txn, struct txn_stmt *stmt, struct request *request)          row->lsn = 0;          row->sync = 0;          row->tm = 0; -        row->opt_flags = 0; +        row->flags = 0;      }      /*       * Group ID should be set both for requests not having a @@ -668,13 +668,16 @@ txn_journal_entry_new(struct txn *txn)          --req->n_rows;      } -    static const uint8_t opt_flags_map[] = { +    static const uint8_t flags_map[] = {          [TXN_WAIT_SYNC] = IPROTO_FLAG_WAIT_SYNC,          [TXN_WAIT_ACK] = IPROTO_FLAG_WAIT_ACK,      }; -    req->opt_flags |= opt_flags_map[txn->flags & TXN_WAIT_SYNC]; -    req->opt_flags |= opt_flags_map[txn->flags & TXN_WAIT_ACK]; +    req->flags |= flags_map[txn->flags & TXN_WAIT_SYNC]; +    req->flags |= flags_map[txn->flags & TXN_WAIT_ACK]; + +    /* is_commit is always set for the last tx row. */ +    req->flags |= IPROTO_FLAG_COMMIT;      return req;  } diff --git a/src/box/wal.c b/src/box/wal.c index 4ec8034a3..53d896972 100644 --- a/src/box/wal.c +++ b/src/box/wal.c @@ -997,10 +997,8 @@ wal_assign_lsn(struct vclock *vclock_diff, struct vclock *base,              }              (*row)->tsn = tsn == 0 ? (*start)->lsn : tsn;              /* Tx meta is stored in the last tx row. */ -            if (row == end - 1) { -                (*row)->opt_flags = entry->opt_flags; -                (*row)->is_commit = true; -            } +            if (row == end - 1) +                (*row)->flags = entry->flags;          } else {              int64_t diff = (*row)->lsn - vclock_get(base, (*row)->replica_id);              if (diff <= vclock_get(vclock_diff, diff --git a/src/box/xrow.c b/src/box/xrow.c index ba121799b..35e1d1c20 100644 --- a/src/box/xrow.c +++ b/src/box/xrow.c @@ -183,7 +183,7 @@ error:              break;          case IPROTO_FLAGS:              flags = mp_decode_uint(pos); -            header->opt_flags = flags; +            header->flags = flags;              break;          default:              /* unknown header */ @@ -299,7 +299,7 @@ xrow_header_encode(const struct xrow_header *header, uint64_t sync,       *   flag to find transaction boundary (last row in the       *   transaction stream).       */ -    uint8_t flags_to_encode = header->opt_flags & ~IPROTO_FLAG_COMMIT; +    uint8_t flags_to_encode = header->flags & ~IPROTO_FLAG_COMMIT;      if (header->tsn != 0) {          if (header->tsn != header->lsn || !header->is_commit) {              /* diff --git a/src/box/xrow.h b/src/box/xrow.h index 0526e3cd9..5ea99e792 100644 --- a/src/box/xrow.h +++ b/src/box/xrow.h @@ -82,7 +82,7 @@ struct xrow_header {      int64_t tsn;      /** Transaction meta flags set only in the last transaction row. */      union { -        uint8_t opt_flags; +        uint8_t flags;          struct {              /**               * Is only encoded in the write ahead log for diff --git a/test/unit/xrow.cc b/test/unit/xrow.cc index 3d7d8bee1..b6018eed9 100644 --- a/test/unit/xrow.cc +++ b/test/unit/xrow.cc @@ -302,7 +302,7 @@ test_request_str()   * still we rely on it for convenience sake.   */  static void -test_xrow_opt_field() +test_xrow_fields()  {      plan(6); @@ -311,24 +311,24 @@ test_xrow_opt_field()      memset(&header, 0, sizeof(header));      header.is_commit = true; -    is(header.opt_flags, IPROTO_FLAG_COMMIT, "header.is_commit -> COMMIT"); +    is(header.flags, IPROTO_FLAG_COMMIT, "header.is_commit -> COMMIT");      header.is_commit = false;      header.wait_sync = true; -    is(header.opt_flags, IPROTO_FLAG_WAIT_SYNC, "header.wait_sync -> WAIT_SYNC"); +    is(header.flags, IPROTO_FLAG_WAIT_SYNC, "header.wait_sync -> WAIT_SYNC");      header.wait_sync = false;      header.wait_ack = true; -    is(header.opt_flags, IPROTO_FLAG_WAIT_ACK, "header.wait_ack -> WAIT_ACK"); +    is(header.flags, IPROTO_FLAG_WAIT_ACK, "header.wait_ack -> WAIT_ACK");      header.wait_ack = false; -    header.opt_flags = IPROTO_FLAG_COMMIT; +    header.flags = IPROTO_FLAG_COMMIT;      ok(header.is_commit && !header.wait_sync && !header.wait_ack, "COMMIT -> header.is_commit"); -    header.opt_flags = IPROTO_FLAG_WAIT_SYNC; +    header.flags = IPROTO_FLAG_WAIT_SYNC;      ok(!header.is_commit && header.wait_sync && !header.wait_ack, "WAIT_SYNC -> header.wait_sync"); -    header.opt_flags = IPROTO_FLAG_WAIT_ACK; +    header.flags = IPROTO_FLAG_WAIT_ACK;      ok(!header.is_commit && !header.wait_sync && header.wait_ack, "WAIT_ACK -> header.wait_ack");      check_plan(); @@ -347,7 +347,7 @@ main(void)      test_greeting();      test_xrow_header_encode_decode();      test_request_str(); -    test_xrow_opt_field(); +    test_xrow_fields();      random_free();      fiber_free(); -- Serge Petrenko