Re: [Tarantool-patches] [PATCH v7 4/8] qsync: direct write of CONFIRM/ROLLBACK into a journal

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

See 3 comments below.

On 14.08.2020 23:14, Cyrill Gorcunov wrote:
> When we need to write CONFIRM or ROLLBACK message (which is
> a binary record in msgpack format) into a journal we use txn code
> to allocate a new transaction, encode there a message and pass it
> to walk the long txn path before it hit the journal. This is not
> only resource wasting but also somehow strange from architectural
> point of view.
> 
> Instead lets encode a record on the stack and write it to the journal
> directly.
> 
> Part-of #5129
> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
> ---
>  src/box/txn_limbo.c | 64 ++++++++++++++++++++++-----------------------
>  1 file changed, 32 insertions(+), 32 deletions(-)
> 
> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
> index ed8c10419..447630d23 100644
> --- a/src/box/txn_limbo.c
> +++ b/src/box/txn_limbo.c
> @@ -284,46 +296,34 @@ txn_limbo_write_synchro(struct txn_limbo *limbo, uint32_t type, int64_t lsn)
>  
>  	/*
>  	 * This is a synchronous commit so we can
> -	 * use body and row allocated on a stack.
> +	 * allocate everything on a stack.
>  	 */

1. Unnecessary change.

>  	struct synchro_body_bin body;
>  	struct xrow_header row;
> -	struct request request = {
> -		.header = &row,
> -	};
> +	char buf[sizeof(struct journal_entry) +
> +		 sizeof(struct xrow_header *)];

2. Is there a guarantee, that 'buf' will be aligned by at least
8 bytes?

>  
> -	struct txn *txn = txn_begin();
> -	if (txn == NULL)
> -		goto rollback;
> +	struct journal_entry *entry = (struct journal_entry *)buf;
> +	entry->rows[0] = &row;
>  
>  	xrow_encode_synchro(&row, &body, &req);
>  
> -	/*
> -	 * This is not really a transaction. It just uses txn API
> -	 * to put the data into WAL. And obviously it should not
> -	 * go to the limbo and block on the very same sync
> -	 * transaction which it tries to confirm now.
> -	 */
> -	txn_set_flag(txn, TXN_FORCE_ASYNC);
> -
> -	if (txn_begin_stmt(txn, NULL) != 0)
> -		goto rollback;
> -	if (txn_commit_stmt(txn, &request) != 0)
> -		goto rollback;
> -	if (txn_commit(txn) != 0)
> -		goto rollback;
> -	return;
> +	journal_entry_create(entry, 1, xrow_approx_len(&row),
> +			     txn_limbo_write_cb, fiber());
>  
> -rollback:
> -	/*
> -	 * XXX: the stub is supposed to be removed once it is defined what to do
> -	 * when a synchro request WAL write fails. One of the possible
> -	 * solutions: log the error, keep the limbo queue as is and probably put
> -	 * in rollback mode. Then provide a hook to call manually when WAL
> -	 * problems are fixed. Or retry automatically with some period.
> -	 */
> -	panic("Could not write a synchro request to WAL: lsn = %lld, type = "
> -	      "%s\n", lsn, iproto_type_name(type));
> +	if (journal_write(entry) != 0 || entry->res < 0) {
> +		diag_set(ClientError, ER_WAL_IO);
> +		diag_log();
> +		/*
> +		 * XXX: the stub is supposed to be removed once it is defined what to do
> +		 * when a synchro request WAL write fails. One of the possible
> +		 * solutions: log the error, keep the limbo queue as is and probably put
> +		 * in rollback mode. Then provide a hook to call manually when WAL
> +		 * problems are fixed. Or retry automatically with some period.

3. Out of 80 symbols.

> +		 */
> +		panic("Could not write a synchro request to WAL: lsn = %lld, type = "
> +		      "%s\n", lsn, type == IPROTO_CONFIRM ? "CONFIRM" : "ROLLBACK");
> +	}
>  }
>  
>  /**
>