From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp57.i.mail.ru (smtp57.i.mail.ru [217.69.128.37]) (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 8CCD142EF5C for ; Fri, 3 Jul 2020 14:24:09 +0300 (MSK) References: <106b9cbcf8577d6e3afbde57b15bc11d366c108e.1593696889.git.sergepetrenko@tarantool.org> <112b0dfc-017d-f3ac-f1eb-cf6291f3d585@tarantool.org> From: Serge Petrenko Message-ID: <5959aee5-762d-a2bf-bac2-a7aff2c4a75f@tarantool.org> Date: Fri, 3 Jul 2020 14:24:08 +0300 MIME-Version: 1.0 In-Reply-To: <112b0dfc-017d-f3ac-f1eb-cf6291f3d585@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB Subject: Re: [Tarantool-patches] [PATCH v3 2/2] replication: append NOP as the last tx row List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy , gorcunov@gmail.com Cc: tarantool-patches@dev.tarantool.org 02.07.2020 23:30, Vladislav Shpilevoy пишет: > Hi! Thanks for the patch! > > See 3 comments below. Thanks  for  the review! Please find my answers and diff below. > >> diff --git a/src/box/txn.c b/src/box/txn.c >> index 123520166..9793109f3 100644 >> --- a/src/box/txn.c >> +++ b/src/box/txn.c >> @@ -517,6 +519,25 @@ txn_journal_entry_new(struct txn *txn) >> assert(remote_row == req->rows + txn->n_applier_rows); >> assert(local_row == remote_row + txn->n_new_rows); >> >> + /* >> + * Append a dummy NOP statement to preserve replication tx >> + * boundaries when the last tx row is a local one. >> + */ >> + if (txn->n_local_rows > 0 && txn->n_local_rows != txn->n_new_rows && > 1. Why do you need 'txn->n_local_rows != txn->n_new_rows' check? > Looks like local rows are always in the end. So if their count > 0, > you can look at *(local_row - 1) without any other checks, no? There are n_applier_rows, n_new_rows, and n_local_rows. A txn has a total of n_applier_rows + n_new_rows rows. First come the n_applier_rows of remote rows, if any, then n_new_rows "local" rows. Here local means that they're not from a remote instance. So, there are two things we need to check: that the last row is related to a local space, and that the transaction isn't fully local. I've just found out I made a mistake. Looks like that's a correct check: if (txn->n_local_rows > 0 && (txn->n_local_rows != txn->n_new_rows || txn->n_applier_rows > 0) && (*(local_row - 1))->group_id == GROUP_LOCAL) { > > Btw, I just noticed, that we already reorder rows - regardless of > what was the order of their appliance, txn_journal_entry_new() > anyway moves all remote rows first, and local rows second. This looks > like the same problem as I was talking about, when we discussed the > local reorder, and gave a FOREIGN KEY example. > > It means it is already broken, in theory. And perhaps that should be > changed. > > If we consider it not broken, then why can't we put local rows first, > and remote rows second? Then we don't need NOP here. We need a NOP when there are no remote rows anyway, since we decided to not reorder local and global rows. > >> + (*(local_row - 1))->group_id == GROUP_LOCAL) { >> + size_t size; >> + *local_row = region_alloc_object(&txn->region, >> + typeof(**local_row), &size); >> + if (*local_row == NULL) { >> + diag_set(OutOfMemory, size, "region_alloc_object", "row"); > 2. Out of 80. Fixed. > >> + return NULL; >> + } >> + memset(*local_row, 0, sizeof(**local_row)); >> + (*local_row)->type = IPROTO_NOP; >> + (*local_row)->group_id = GROUP_DEFAULT; >> + } else >> + --req->n_rows; > 3. Lets add {} to the 'else' branch. To be consistent with the 'if' > branch. Ok. diff --git a/src/box/txn.c b/src/box/txn.c index 9793109f3..765dbd2ce 100644 --- a/src/box/txn.c +++ b/src/box/txn.c @@ -521,22 +521,26 @@ txn_journal_entry_new(struct txn *txn)      /*       * Append a dummy NOP statement to preserve replication tx -     * boundaries when the last tx row is a local one. +     * boundaries when the last tx row is a local one, and the +     * transaction has at least one global row.       */ -    if (txn->n_local_rows > 0 && txn->n_local_rows != txn->n_new_rows && +    if (txn->n_local_rows > 0 && +        (txn->n_local_rows != txn->n_new_rows || txn->n_applier_rows > 0) &&          (*(local_row - 1))->group_id == GROUP_LOCAL) {          size_t size;          *local_row = region_alloc_object(&txn->region,                           typeof(**local_row), &size);          if (*local_row == NULL) { -            diag_set(OutOfMemory, size, "region_alloc_object", "row"); +            diag_set(OutOfMemory, size, "region_alloc_object", +                 "row");              return NULL;          }          memset(*local_row, 0, sizeof(**local_row));          (*local_row)->type = IPROTO_NOP;          (*local_row)->group_id = GROUP_DEFAULT; -    } else +    } else {          --req->n_rows; +    }      return req;  } -- Serge Petrenko