From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: Serge Petrenko <sergepetrenko@tarantool.org>, gorcunov@gmail.com Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v3 2/2] replication: append NOP as the last tx row Date: Thu, 2 Jul 2020 22:30:00 +0200 [thread overview] Message-ID: <112b0dfc-017d-f3ac-f1eb-cf6291f3d585@tarantool.org> (raw) In-Reply-To: <106b9cbcf8577d6e3afbde57b15bc11d366c108e.1593696889.git.sergepetrenko@tarantool.org> Hi! Thanks for the patch! See 3 comments 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? 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. > + (*(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. > + 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.
next prev parent reply other threads:[~2020-07-02 20:30 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-02 13:39 [Tarantool-patches] [PATCH v3 0/2] fix replication tx boundaries after local space rework Serge Petrenko 2020-07-02 13:39 ` [Tarantool-patches] [PATCH v3 1/2] wal: fix tx boundaries Serge Petrenko 2020-07-02 13:39 ` [Tarantool-patches] [PATCH v3 2/2] replication: append NOP as the last tx row Serge Petrenko 2020-07-02 14:35 ` Cyrill Gorcunov 2020-07-02 14:49 ` Cyrill Gorcunov 2020-07-02 15:37 ` Cyrill Gorcunov 2020-07-02 20:30 ` Vladislav Shpilevoy [this message] 2020-07-03 11:24 ` Serge Petrenko 2020-07-03 21:29 ` Vladislav Shpilevoy 2020-07-04 17:25 ` Serge Petrenko 2020-07-06 6:48 ` Vladislav Shpilevoy 2020-07-06 7:47 ` [Tarantool-patches] [PATCH v3 0/2] fix replication tx boundaries after local space rework Kirill Yukhin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=112b0dfc-017d-f3ac-f1eb-cf6291f3d585@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=gorcunov@gmail.com \ --cc=sergepetrenko@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v3 2/2] replication: append NOP as the last tx row' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox