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

[Georgy Kirichenko]

On Sunday, September 22, 2019 7:14:22 PM MSK Vladislav Shpilevoy wrote:
> Thanks for the patch!
> 
> See 10 comments below!
> 
> On 18/09/2019 11:36, Georgy Kirichenko wrote:
> > Prerequisites: #3794
> 
> 1. The same as in previous patches - we usually don't use ':', please drop
> it.
Ok
> 
> > ---
> > +		      struct vclock *vclock)
> 
> 2. Something is wrong with the indentation. Below too.
> 
Ok
> > +{
> > +		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)
> 
The problem is that vclock could be incomparable - so I could not use < / > 
with checking the incomparable result.
> > +			break;
> > +		/* 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?
If it is a temporary slowdown and a relay will process data faster than a 
master writes then the relay will return to in-memory replication. In the 
opposite case, if the relay is lower that the master, then the relay will be 
discarded because of collected wall file - in this case there is nothing we can 
do.
> 
> > +		return -1;
> > +				 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.
Yes, I used ibuf because it is more convenient in comparison of tracking count 
and capacity and reallocating the memory. I would like not to reimplement a 
bicycle but I will try to introduce an statically typed 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 {}.
Ok
> 
> > +		xrow_buf_cursor->row_index = 0;
> > +		++xrow_buf_cursor->chunk_gen;
> >  	       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.
Ok
> 
> > +	/* 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.
> 
Generation - is the serial number of the chunk. A xrow buffer track two 
generation values - first and last. At the start both values are equal to zero. 
Each time a chunk is inserted to a ring head a last generation is increased. 
Each time a chunk is discarded a first generation is increased. So the xrow 
buffer has chunks with generations: first, first + 1, .., last. To determine the 
index of chunk into a ring you should just wrap its generation around the ring 
size: index = generation mod ring_size.

> > +	uint32_t chunk_gen;
> > +	/* Current row index. */
*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:
Ok
> 
> 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);