From: Serge Petrenko <sergepetrenko@tarantool.org> To: Vladislav Shpilevoy <v.shpilevoy@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: Sat, 4 Jul 2020 20:25:45 +0300 [thread overview] Message-ID: <cb3bc117-e31e-5c03-a48e-c05da20e46c5@tarantool.org> (raw) In-Reply-To: <f09b8fab-4500-087e-2b4e-e743d821eadf@tarantool.org> 04.07.2020 00:29, Vladislav Shpilevoy пишет: > Привет. Привет. Обсудили уже в личке, продублирую здесь. > >>>> 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 и тогда ноп не нужен? Да, проверка на то что последний row - GROUP_LOCAL, и на то что транзакция не вся целиком 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() - все стейтменты будут локальные. Выяснилось, что мастер получит ак, потому что на каждый стейтмент, который в апплаере заменили на "NOP", т е вернули new_tuple = old_tuple, действительно в вал запишется NOP. > > По поводу since we decided to not reorder local and global rows - все так, да. > Но мой поинт в том, что у нас оказывается реордеринг **уже** есть. У транзакции > remote и local rows переупорядочиваются. Из этого происходит, что переупорядочивание > все равно есть. Независимо от того, внесем ли мы еще одно сейчас. А значит надо > либо это тоже чинить, чтоб вообще ничего не переупорядочивалось, либо тогда не > обязательно сдерживаться, и в этом патче тоже можно переупорядочить Если мы сейчас сделаем НОП вместо переупорядочивания, то мы как минимум не сломаем это всё ещё сильнее. Не знаю, зачем переупорядочивать remote и local записи. Это наверное появилось вместе с транзакционным апплаером, но зачем - непонятно. -- Serge Petrenko
next prev parent reply other threads:[~2020-07-04 17:25 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 2020-07-03 11:24 ` Serge Petrenko 2020-07-03 21:29 ` Vladislav Shpilevoy 2020-07-04 17:25 ` Serge Petrenko [this message] 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=cb3bc117-e31e-5c03-a48e-c05da20e46c5@tarantool.org \ --to=sergepetrenko@tarantool.org \ --cc=gorcunov@gmail.com \ --cc=tarantool-patches@dev.tarantool.org \ --cc=v.shpilevoy@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