Re: [Tarantool-patches] [PATCH v3 2/2] replication: append NOP as the last tx row

[Vladislav Shpilevoy <v.shpilevoy@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 переупорядочиваются. Из этого происходит, что переупорядочивание
все равно есть. Независимо от того, внесем ли мы еще одно сейчас. А значит надо
либо это тоже чинить, чтоб вообще ничего не переупорядочивалось, либо тогда не
обязательно сдерживаться, и в этом патче тоже можно переупорядочить