[tarantool-patches] Re: [PATCH v2 3/4] wal: xrow buffer cursor

[Vladislav Shpilevoy]

Thanks for the patch!

See 10 comments below!

On 18/09/2019 11:36, Georgy Kirichenko wrote:
> This structure enables to find a xrow buffer row less than given vclock
> and then fetch row by row from the xrow forwards to the last appended ones.
> A xrow buffer cursor is essential to allow from memory replication because
> of relay which required to be able to fetch all log rows, stored in a wal
> memory (implemented as xrow buffer), from given position and then follow
> all new changes.
> 
> Prerequisites: #3794

1. The same as in previous patches - we usually don't use ':', please drop
it.

> ---
>  src/box/xrow_buf.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++
>  src/box/xrow_buf.h | 22 +++++++++++++++++
>  2 files changed, 81 insertions(+)
> 
> diff --git a/src/box/xrow_buf.c b/src/box/xrow_buf.c
> index d690ac4b7..c73e3acb2 100644
> --- a/src/box/xrow_buf.c
> +++ b/src/box/xrow_buf.c
> @@ -218,3 +218,62 @@ error:
>  	return -1;
>  }
>  
> +int
> +xrow_buf_cursor_create(struct xrow_buf *xrow_buf,
> +		      struct xrow_buf_cursor *xrow_buf_cursor,
> +		      struct vclock *vclock)

2. Something is wrong with the indentation. Below too.

> +{
> +	uint64_t chunk_gen;
> +	for (chunk_gen = xrow_buf->first_chunk_gen;
> +	     chunk_gen <= xrow_buf->last_chunk_gen;
> +	     ++chunk_gen) {
> +		struct xrow_buf_chunk *chunk =
> +			xrow_buf->chunk + chunk_gen % XROW_BUF_CHUNK_COUNT;
> +		int rc = vclock_compare(&chunk->vclock, vclock);
> +		if (rc != 0 && rc != -1)

3. I don't like the assumption that vclock_compare returns exact
values. Usually you can rely only on a sign of comparison result: > 0,
< 0, == 0. In this case you could just write

    if (vclock_compare(&chunk->vclock, vclock) > 0)

> +			break;
> +	}
> +	if (chunk_gen == xrow_buf->first_chunk_gen)
> +		return -1;
> +	xrow_buf_cursor->chunk_gen = chunk_gen - 1;
> +	xrow_buf_cursor->row_index = 0;
> +	return 0;> +}
> +
> +int
> +xrow_buf_cursor_next(struct xrow_buf *xrow_buf,
> +		    struct xrow_buf_cursor *xrow_buf_cursor,
> +		    struct xrow_header **row,
> +		    void **data,
> +		    size_t *size)
> +{
> +	if (xrow_buf->first_chunk_gen > xrow_buf_cursor->chunk_gen) {
> +		/* Buffer was discarded. */

4. Ok, perhaps now I understand the idea of why do you discard
chunks without any checks if they are in use. Looks cool.

But it means, that if a relay sends data too slow, it will fall
from the in-memory replication down to disk. Being fallen to a disk,
how can it return back? Isn't it a problem, that the chunks will
be being written to wal so many and fast, that the relay will never
return back to in-memory replication?

> +		return -1;
> +	}
> +
> +	struct xrow_buf_chunk *chunk;
> +
> +next_chunk:
> +	chunk = xrow_buf->chunk + xrow_buf_cursor->chunk_gen %
> +				  XROW_BUF_CHUNK_COUNT;
> +	size_t chunk_row_count = ibuf_used(&chunk->rows) /
> +				 sizeof(struct xrow_buf_row_info);

5. I didn't pay attention to this earlier, but looks like it
is time. You use ibuf here and in other places as just an array
of xrow_buf_row_info objects, but ibuf is not supposed to be
used for that. It is 'input buffer', for binary data usually.
For an array you cold declare a normal array, xrow_buf_row_info *,
with size field, with compile-time type checks to avoid the
problem which I mentioned earlier. When you reserved ibuf for
xrow_header, but used as xrow_buf_row_info. Please, use a normal
array.

> +	if (chunk_row_count == xrow_buf_cursor->row_index) {
> +		/* No more rows in the current buffer. */
> +		if (xrow_buf->last_chunk_gen == xrow_buf_cursor->chunk_gen)
> +			/* No more rows in the xrow buffer. */
> +			return 1;

6. Please, wrap multi-line bodies into {}.

> +		xrow_buf_cursor->row_index = 0;
> +		++xrow_buf_cursor->chunk_gen;
> +		goto next_chunk;
> +	}
> +	struct xrow_buf_row_info *row_info =
> +		(struct xrow_buf_row_info *)chunk->rows.rpos +
> +		xrow_buf_cursor->row_index;
> +	*row = &row_info->xrow;
> +	*data = row_info->data;
> +	*size = row_info->size;
> +	++xrow_buf_cursor->row_index;
> +	return 0;
> +}
> diff --git a/src/box/xrow_buf.h b/src/box/xrow_buf.h
> index 76f9bc6f8..c9baed360 100644
> --- a/src/box/xrow_buf.h
> +++ b/src/box/xrow_buf.h
> @@ -135,4 +135,26 @@ xrow_buf_write(struct xrow_buf *xrow_buf, struct xrow_header **begin,
>  	       struct xrow_header **end,
>  	       struct iovec *iovec);
>  
> +/* xrow buffer cursor. */
> +struct xrow_buf_cursor {

7. Sorry, you can't just replace '_' with ' ' in a
structure name and make it a comment. Please, write a
more descriptive comment. What is a purpose of objects
of this structure, what is their lifetime?

8. Please, use /** instead of /* for out-of-function-body
comments. Here and in all other places, in all other
patches. They are recognized by doxygen.

> +	/* Current buffer chunk generation. */

9. I still don't understand what is 'generation'.
Is it an absolute index in the ring of chunks?

Generation is a something like version, not index.
For index you could just use 'index' name.

> +	uint32_t chunk_gen;
> +	/* Current row index. */
> +	uint32_t row_index;
> +};
> +
> +/* Create a xrow buf cursor from the vclock position. */
> +int
> +xrow_buf_cursor_create(struct xrow_buf *xrow_buf,
> +		       struct xrow_buf_cursor *xrow_buf_cursor,
> +		       struct vclock *vclock);> +
> +/* Fetch next row and move a cursor forward. */
> +int
> +xrow_buf_cursor_next(struct xrow_buf *xrow_buf,
> +		     struct xrow_buf_cursor *xrow_buf_cursor,
> +		     struct xrow_header **row,
> +		     void **data,
> +		     size_t *size);

10. Please, don't put each argument on a separate line, when it
is not necessary. In the C file too:

diff --git a/src/box/xrow_buf.h b/src/box/xrow_buf.h
index c9baed360..7c67f8dbc 100644
--- a/src/box/xrow_buf.h
+++ b/src/box/xrow_buf.h
@@ -153,8 +153,6 @@ xrow_buf_cursor_create(struct xrow_buf *xrow_buf,
 int
 xrow_buf_cursor_next(struct xrow_buf *xrow_buf,
 		     struct xrow_buf_cursor *xrow_buf_cursor,
-		     struct xrow_header **row,
-		     void **data,
-		     size_t *size);
+		     struct xrow_header **row, void **data, size_t *size);