From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp55.i.mail.ru (smtp55.i.mail.ru [217.69.128.35]) (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 0F27E42EF5C for ; Sat, 4 Jul 2020 00:30:01 +0300 (MSK) References: <106b9cbcf8577d6e3afbde57b15bc11d366c108e.1593696889.git.sergepetrenko@tarantool.org> <112b0dfc-017d-f3ac-f1eb-cf6291f3d585@tarantool.org> <5959aee5-762d-a2bf-bac2-a7aff2c4a75f@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Fri, 3 Jul 2020 23:29:58 +0200 MIME-Version: 1.0 In-Reply-To: <5959aee5-762d-a2bf-bac2-a7aff2c4a75f@tarantool.org> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: Serge Petrenko , gorcunov@gmail.com Cc: tarantool-patches@dev.tarantool.org Привет. >>> 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) { Я все равно не понимаю. Почему недостаточно тупо проверить последний local_row? txn->n_local_rows != txn->n_new_rows - это нужно только за тем, что вся транзакция может быть 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. Что если прилетела транзакция, и аплаер ее целиком переписал через before_replace триггеры + вставил что-то в локальные спейсы? Тогда получится, что мастер не получит никакого подтверждения от реплики, что она транзакцию в целом получила? Потому как лсн она от мастера не сможет ниоткуда взять в txn_commit_async() - все стейтменты будут локальные. По поводу since we decided to not reorder local and global rows - все так, да. Но мой поинт в том, что у нас оказывается реордеринг **уже** есть. У транзакции remote и local rows переупорядочиваются. Из этого происходит, что переупорядочивание все равно есть. Независимо от того, внесем ли мы еще одно сейчас. А значит надо либо это тоже чинить, чтоб вообще ничего не переупорядочивалось, либо тогда не обязательно сдерживаться, и в этом патче тоже можно переупорядочить