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 012C76FC8F; Fri, 16 Apr 2021 11:57:28 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 012C76FC8F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1618563448; bh=SWd8zzPj9sRcePJdn06eUIYck7Q/UTvt+Ybsa5VgyGs=; 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=CAIGYakNhF5424IvcJzDL6sL9Lc0fntptPHYedgZRM+p7pRB/DcRACnRArFBjYwXZ GmWYDkP31C/ZUO3JCZOz+/Gm+vCRfsJ4UbpszGdz0s5ZVKAUHx5uD1wzzTOhJiR+wM NhoEd5H2QcX0SmRDh0bkKDZ/X4a3Uk2KJGPAVYVU= Received: from smtp31.i.mail.ru (smtp31.i.mail.ru [94.100.177.91]) (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 783836FC8F for ; Fri, 16 Apr 2021 11:57:26 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 783836FC8F Received: by smtp31.i.mail.ru with esmtpa (envelope-from ) id 1lXKI1-0001Cm-Jg; Fri, 16 Apr 2021 11:57:26 +0300 To: Vladislav Shpilevoy , gorcunov@gmail.com Cc: tarantool-patches@dev.tarantool.org References: <32099031-d261-5887-538a-86d770a15617@tarantool.org> <75ca15d4-9614-12e3-6871-10f36831a78a@tarantool.org> Message-ID: <55ed449c-c8f6-1d57-878f-20d8be6a0e1a@tarantool.org> Date: Fri, 16 Apr 2021 11:57:25 +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: <75ca15d4-9614-12e3-6871-10f36831a78a@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92FFCB8E6708E7480B1C8842CE613979723F2FB4628545A35182A05F53808504045CD913F2CFCD8920E4961AC4BD9FAB11A6C41B9F6C18A3EB1881D0E1CA71325 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE731B6267C8418DDA1EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637358384D8B7467A5C8638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B2675AD50E271DE361D1B9D65FAB154B8A4A8204C501E24153D2E47CDBA5A96583C09775C1D3CA48CFE478A468B35FE767117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE78C592797616C97AB9FA2833FD35BB23DF004C90652538430302FCEF25BFAB3454AD6D5ED66289B5278DA827A17800CE72193DCEE80A9F535D32BA5DBAC0009BE395957E7521B51C20BC6067A898B09E4090A508E0FED6299176DF2183F8FC7C0E3CB5054D96D84B0CD04E86FAF290E2D7E9C4E3C761E06A71DD303D21008E298D5E8D9A59859A8B6B372FE9A2E580EFC725E5C173C3A84C3C9EEE74C166EF7BC35872C767BF85DA2F004C90652538430E4A6367B16DE6309 X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A24209795067102C07E8F7B195E1C97831FAB21D6EA12E01A42DAC2A4A94C307B6 X-C1DE0DAB: 0D63561A33F958A5E557B5E170F8F2547EB448325F252601103CDA927135587FD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34FACF503ACC83041B017D5AC343D4B2094575351507203A7D77537B5F23144438F72A38CA6149F4971D7E09C32AA3244CDE001CFE7F82E3CB7C90D58CB38A01727C0C08F7987826B9FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj3S6P1v0GIqTOEz/miHdf1g== X-Mailru-Sender: 583F1D7ACE8F49BDD2846D59FC20E9F881D8639E95573B650884988DF055E601FE5731C938C2E39E424AE0EB1F3D1D21E2978F233C3FAE6EE63DB1732555E4A8EE80603BA4A5B0BC112434F685709FCF0DA7A0AF5A3A8387 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 10:08, Serge Petrenko via 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. Forget about that. Let's leave (row)->is_commit = true in wal_assign_lsn(). Otherwise I need to set entry->flags for synchro entries and raft entries, which aren't transactions, actually, so their journal entries do not go through txn_journal_entry_new(). ======================================== diff --git a/src/box/txn.c b/src/box/txn.c index 31f664aa0..a71ccadd0 100644 --- a/src/box/txn.c +++ b/src/box/txn.c @@ -676,9 +676,6 @@ txn_journal_entry_new(struct txn *txn)         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 53d896972..5b6200b81 100644 --- a/src/box/wal.c +++ b/src/box/wal.c @@ -997,8 +997,10 @@ 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) +                       if (row == end - 1) {                                 (*row)->flags = entry->flags; +                               (*row)->is_commit = true; +                       }                 } else {                         int64_t diff = (*row)->lsn - vclock_get(base, (*row)->replica_id);                         if (diff <= vclock_get(vclock_diff, > >> >>>   } >>>     /** >>> 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