From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
Received: from smtp48.i.mail.ru (smtp48.i.mail.ru [94.100.177.108])
 (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 242BB42EF5C
 for <tarantool-patches@dev.tarantool.org>;
 Thu,  2 Jul 2020 23:30:01 +0300 (MSK)
References: <cover.1593696889.git.sergepetrenko@tarantool.org>
 <106b9cbcf8577d6e3afbde57b15bc11d366c108e.1593696889.git.sergepetrenko@tarantool.org>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <112b0dfc-017d-f3ac-f1eb-cf6291f3d585@tarantool.org>
Date: Thu, 2 Jul 2020 22:30:00 +0200
MIME-Version: 1.0
In-Reply-To: <106b9cbcf8577d6e3afbde57b15bc11d366c108e.1593696889.git.sergepetrenko@tarantool.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [Tarantool-patches] [PATCH v3 2/2] replication: append NOP as
 the last tx row
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Serge Petrenko <sergepetrenko@tarantool.org>, gorcunov@gmail.com
Cc: tarantool-patches@dev.tarantool.org

Hi! Thanks for the patch!

See 3 comments 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?

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.

> +	    (*(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.

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