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

[Serge Petrenko]

02.07.2020 23:30, Vladislav Shpilevoy пишет:
> Hi! Thanks for the patch!
>
> See 3 comments below.

Thanks  for  the review!

Please find my answers and diff 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?

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) {

>
> 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.

>
>> +	    (*(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.
Fixed.
>
>> +			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.
Ok.
diff --git a/src/box/txn.c b/src/box/txn.c
index 9793109f3..765dbd2ce 100644
--- a/src/box/txn.c
+++ b/src/box/txn.c
@@ -521,22 +521,26 @@ txn_journal_entry_new(struct txn *txn)

      /*
       * Append a dummy NOP statement to preserve replication tx
-     * boundaries when the last tx row is a local one.
+     * boundaries when the last tx row is a local one, and the
+     * transaction has at least one global row.
       */
-    if (txn->n_local_rows > 0 && txn->n_local_rows != txn->n_new_rows &&
+    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) {
          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");
+            diag_set(OutOfMemory, size, "region_alloc_object",
+                 "row");
              return NULL;
          }
          memset(*local_row, 0, sizeof(**local_row));
          (*local_row)->type = IPROTO_NOP;
          (*local_row)->group_id = GROUP_DEFAULT;
-    } else
+    } else {
          --req->n_rows;
+    }

      return req;
  }


-- Serge Petrenko