Re: [Tarantool-patches] [PATCH 7/8] xrow: introduce CONFIRM entry

[Leonid Vasiliev <lvasiliev@tarantool.org>]

LGTM.
All the following comments can be skipped silently.

On 09.06.2020 15:20, Serge Petrenko wrote:
> Add methods to encode/decode CONFIRM entry.
> A CONFIRM entry will be written to WAL by synchronous replication master
> as soon as it finds that the transaction was applied on a quorum of
> replicas.
> CONFIRM rows share the same header with other rows in WAL,but their body
> differs: it's just a map containing replica_id and lsn of the last
> confirmed transaction.
> 
> Part-of #4847
> ---
>   src/box/iproto_constants.h |  3 ++
>   src/box/xrow.c             | 74 ++++++++++++++++++++++++++++++++++++++
>   src/box/xrow.h             | 23 ++++++++++++
>   3 files changed, 100 insertions(+)
> 
> diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h
> index f8eee0f3f..1466b456f 100644
> --- a/src/box/iproto_constants.h
> +++ b/src/box/iproto_constants.h
> @@ -219,6 +219,9 @@ enum iproto_type {
>   	/** The maximum typecode used for box.stat() */
>   	IPROTO_TYPE_STAT_MAX,
>   
> +	/** A confirmation message for synchronous transactions. */
> +	IPROTO_CONFIRM = 40,
> +

Seems like IPROTO_CONFIRM must be added to the documentation. If it's
true, please add @TarantoolBot.

>   	/** PING request */
>   	IPROTO_PING = 64,
>   	/** Replication JOIN command */
> diff --git a/src/box/xrow.c b/src/box/xrow.c
> index bb64864b2..f197e0d85 100644
> --- a/src/box/xrow.c
> +++ b/src/box/xrow.c
> @@ -878,6 +878,80 @@ xrow_encode_dml(const struct request *request, struct region *region,
>   	return iovcnt;
>   }
>   
> +int
> +xrow_encode_confirm(struct xrow_header *row, uint32_t replica_id, int64_t lsn)
> +{
> +	size_t len = mp_sizeof_map(2) + mp_sizeof_uint(IPROTO_REPLICA_ID) +
> +		     mp_sizeof_uint(replica_id) + mp_sizeof_uint(IPROTO_LSN) +
> +		     mp_sizeof_uint(lsn);
> +	char *buf = (char *)region_alloc(&fiber()->gc, len);
> +	if (buf == NULL) {
> +		diag_set(OutOfMemory, len, "region_alloc", "buf");
> +		return -1;
> +	}
> +	char *pos = buf;
> +
> +	pos = mp_encode_map(pos, 2);
> +	pos = mp_encode_uint(pos, IPROTO_REPLICA_ID);
> +	pos = mp_encode_uint(pos, replica_id);
> +	pos = mp_encode_uint(pos, IPROTO_LSN);
> +	pos = mp_encode_uint(pos, lsn);
> +
> +	row->body[0].iov_base = buf;
> +	row->body[0].iov_len = len;
> +
> +	row->type = IPROTO_CONFIRM;
> +
> +	return 1;
> +}

At the last version
memset(row, 0, sizeof(*row));
was added. But, usually it is initialized at the beginning of the
function, because otherwise it is possible to do return without
initializing of the row. It does not look like a problem, the decision
is yours.

> +
> +int
> +xrow_decode_confirm(struct xrow_header *row, uint32_t *replica_id, int64_t *lsn)
> +{
> +	if (row->bodycnt == 0) {
> +		diag_set(ClientError, ER_INVALID_MSGPACK, "request body");
> +		return -1;
> +	}
> +
> +	assert(row->bodycnt == 1);
> +
> +	const char * const data = (const char *)row->body[0].iov_base;
> +	const char * const end = data + row->body[0].iov_len;
> +	const char *d = data;
> +	if (mp_check(&d, end) != 0 || mp_typeof(*data) != MP_MAP) {
> +		xrow_on_decode_err(data, end, ER_INVALID_MSGPACK,
> +				   "request body");
> +		return -1;
> +	}
> +
> +	d = data;
> +	uint32_t map_size = mp_decode_map(&d);
> +	for (uint32_t i = 0; i < map_size; i++) {
> +		if (mp_typeof(*d) != MP_UINT) {
> +			mp_next(&d);
> +			mp_next(&d);
> +			continue;
> +		}
> +		uint8_t key = mp_decode_uint(&d);
> +		if (key >= IPROTO_KEY_MAX || iproto_key_type[key] !=
> +					     mp_typeof(*d)) {
> +				xrow_on_decode_err(data, end, ER_INVALID_MSGPACK,
> +						   "request body");
> +		}
> +		switch (key) {
> +		case IPROTO_REPLICA_ID:
> +			*replica_id = mp_decode_uint(&d);
> +			break;
> +		case IPROTO_LSN:
> +			*lsn = mp_decode_uint(&d);
> +			break;
> +		default:
> +			mp_next(&d);
> +		}
> +	}
> +	return 0;
> +}
> +
>   int
>   xrow_to_iovec(const struct xrow_header *row, struct iovec *out)
>   {
> diff --git a/src/box/xrow.h b/src/box/xrow.h
> index 2a0a9c852..75af71b77 100644
> --- a/src/box/xrow.h
> +++ b/src/box/xrow.h
> @@ -207,6 +207,29 @@ int
>   xrow_encode_dml(const struct request *request, struct region *region,
>   		struct iovec *iov);
>   
> +/**
> + * Encode the CONFIRM to row body and set row type to
> + * IPROTO_CONFIRM.
> + * @param row xrow header.
> + * @param replica_id master's instance id.
> + * @param lsn last confirmed lsn.
> + * @retval -1 on error.
> + * @retval > 0 xrow bodycnt.
> + */
> +int
> +xrow_encode_confirm(struct xrow_header *row, uint32_t replica_id, int64_t lsn);
> +
> +/**
> + * Decode the CONFIRM request body.
> + * @param row xrow header.
> + * @param[out] replica_id master's instance id.
> + * @param[out] lsn last confirmed lsn.
> + * @retwal -1 on error.
> + * @retwal 0 success.

The typo is fixed by you in "xrow: fix comment typo".

> + */
> +int
> +xrow_decode_confirm(struct xrow_header *row, uint32_t *replica_id, int64_t *lsn);
> +
>   /**
>    * CALL/EVAL request.
>    */
>