Tarantool development patches archive
 help / color / mirror / Atom feed
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

  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