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

[Vladislav Shpilevoy]

Hi! Thanks for the answers!

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

For an incomparable result there is a special value -
VCLOCK_ORDER_UNDEFINED = INT_MAX. If result != VCLOCK_ORDER_UNDEFINED,
you can use a sign, no?

Btw, taking into account, that VCLOCK_ORDER_UNDEFINED > 0, your code
still is equivalent to just checking > 0. Because vclock_compare() returns
one of the values -1, 0, 1, INT_MAX. You check for -1 and 0. Other values
(1, INT_MAX) are > 0. Therefore, you could use <= 0 as a break condition.

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

Have you asked Alexander Tikh. to bench you branch? I think
we need to test how realistic is the case, when WAL writes much faster
than replicates, and the replicas just never can keep master's pace.
Is it correct, that relay doesn't send a next transaction until a
previous is ACKed?

>>
>>> +		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 beg you, current solution looks much more hard and complex than
keeping a strictly typed pointer and a couple of numbers. Usage of a
binary buffer as an array backend is not convenient at all, without
a properly typed wrapper.

> I would like not to reimplement a 
> bicycle but I will try to introduce an statically typed array.

If you think it is a so often appearing task, you can implement a
macros-library with array operations.

    No-struct version, keeping each attribute:

        #define array_create(begin, size, capacity) ...
        #define array_append(begin, size, capacity, new_element) ...
        #define array_reserve(begin, size, capacity, size_to_add) ...
        etc ...

    Or a version using a struct:

        struct array {
            void *buffer;
            int size;
            int capacity;
        };

        #define array_create(arr, initial_size)
        #define array_append(arr, type, new_value)
        #define array_reserve(arr, type, size_to_add)
        etc ...

    Or more intricate

        #define ARRAY(type) {   \
            type *buffer;       \
            int size;           \
            int capacity;       \
        };

        #define array_create(arr, initial_size)
        #define array_append(arr, new_value)
        #define array_reserve(arr, size_to_add)
        etc ...

        Usage is a bit tricky
        
        ARRAY(xrow_header) headers;
        array_create(headers, 100);
        ...

        But looks more robust, because a type is
        not a part of each macros.

Or open a ticket on it. Strangely how we have not
implemented it yet. My point is to just keep compile
time type checks.

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

So it is just an absolute index. I would rather use name 'index',
it would allow to avoid so difficult explanations. But up to you.