Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v2 0/4] In-memory wal replication

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v2 0/4] In-memory wal replication

[Vladislav Shpilevoy]

On 20/09/2019 10:28, Georgy Kirichenko wrote:
> On Friday, September 20, 2019 10:52:49 AM MSK Konstantin Osipov wrote:
>> * Vladislav Shpilevoy <v.shpilevoy@tarantool.org> [19/09/19 00:44]:
>>>> The last patch makes a relay able to read from wal memory using
>>>> a xrow_buffer cursor. When relay went out of wal memory window it turns
>>>> back to file mode. After last known xlog file is relayed the a relay
>>>> returns to wal memory.
>>>
>>> Please, ask Alexander to bench your branch to check if there are no
>>> performance problems.
>>
>> the whole series should boost the performance quite a bit, after
>> all we are adding an in-memory cache of wal rows and remove file
>> I/O in the most common case.
>>
>> But tx overhead is increased by more eager gc_advance, so it has
>> to be benched independently.
> It is not the latest branch, just after this I plan to move relay into wal, 
> which allows us to process the whole xlog collection logic in the wal thread. 
> 

It will take unknown amount of time, what means, that this
not the latest branch might be on top of the master for quite
a long time, and may become a part of a release. We need it
benched.

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v2 2/4] wal: wal memory buffer

[Vladislav Shpilevoy]

>>>>>> +	/* Decoded xrow header. */
>>>>>> +	struct xrow_header xrow;
>>>>>> +	/* Pointer to the xrow encoded raw data. */
>>>>>> +	void *data;
>>>>>> +	/* xrow raw data size. */
>>>>>> +	size_t size;
>>>>>> +};
>>>>>> +
>>>>>> +/*
>>>>>> + * Xrow memory data chunk info contains
>>>>>> + *  a vclock just before the first stored row,
>>>>>> + *  an ibuf with row descriptors
>>>>>> + *  an obuf with encoded data
>>>>>
>>>>> 29. I don't think it really makes sense to just dupicate here
>>>>> the comments of each attribute of the structure. Please,
>>>>> better use that space to explain, why you need vclock, why
>>>>> 'before first row' instead of just 'first row', or 'last row'.
>>>>>
>>>>> What is a 'row descriptor'?
>>>>>
>>>>> Why do you need both initial xrows and encoded? I thought, that
>>>>> xrow_buf stores only encoded rows.
>>>>
>>>> Relay will need initial xrows to perform filtering (replication group, lsn
>>>> and e.t.c.) without xrow encoded body to decode.
>>>
>>> Hm, it looks quite expensive just for filtering. But assume it is ok.
>>> 1) Why can't you store only a header instead of the whole xrow with a
>>> body? As I understand, for any filtering the header is enough. Or not?
>> I should also store xrow bodies and then encode the xrow as many times as 
>> count of replicas I have.
> 
> Why do you need to encode it multiple times? Why not to send the same
> encoded record?
> Also, I don't see where do you encode more than once.
> 

Please, say anything. I still don't understand it.

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v2 2/4] wal: wal memory buffer

[Vladislav Shpilevoy]

>> +enum {
>> +	/*
>> +	 * Xrow memory object contains some count of rotating data chunks.
>> +	 * Every rotation estimated decrease in amount of stored rows is
>> +	 * about 1/(COUNT OF CHUNKS). However the bigger value makes rotation
>> +	 * more frequent, the decrease would be smoother and size of
>> +	 * a wal memory more stable.
> 
> 27. Please, limit max length of a comment string to 66 symbols. It
> helps reading, no jokes. In the C file too.

Sorry, still out of 66. In many other places too.

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

[Vladislav Shpilevoy]

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

Any news here?

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v2 4/4] replication: use wal memory buffer to fetch rows

[Vladislav Shpilevoy]

See 5 comments below.

>> diff --git a/src/box/relay.cc b/src/box/relay.cc
>> index 21674119d..1e65d6d56 100644
>> --- a/src/box/relay.cc
>> +++ b/src/box/relay.cc
>> @@ -161,9 +163,9 @@ relay_new(struct replica *replica)
>>  	}
>>  	relay->replica = replica;
>>  	relay->last_row_time = ev_monotonic_now(loop());
>> -	fiber_cond_create(&relay->reader_cond);
>>  	diag_create(&relay->diag);
>>  	relay->state = RELAY_OFF;
>> +	relay->r = NULL;
> 
> 1. Why? 'relay' is allocated using 'calloc' a few lines
> above, 'r' is NULL already.
> 

1. Why did you ignore that comment?

>> +	relay->wal_dir = strdup(cfg_gets("wal_dir"));
>> +	if (relay->wal_dir == NULL) {
>> +		diag_set(OutOfMemory, strlen(cfg_gets("wal_dir")),
> 
> 10. I know, I am a fucking bore, but 'strlen() + 1', for terminating
> zero :)
> 

2. Why did you ignore that comment?

>> +			 "runtime", "wal_dir");
> 
> 11. There is a strict rule what arguments we pass to diag_raise -
> it is size, allocator function, variable name. Allocator function
> here is 'strdup', not 'runtime'.
> 

3. Why did you ignore that comment?

>> +	struct fiber_cond xrow_buf_cond;
>>  };
>>  
>>  struct wal_msg {
>> @@ -1131,6 +1134,7 @@ wal_writer_f(va_list ap)
>>  	(void) ap;
>>  	struct wal_writer *writer = &wal_writer_singleton;
>>  	xrow_buf_create(&writer->xrow_buf);
>> +	fiber_cond_create(&writer->xrow_buf_cond);
> 
> 18. Where do you call fiber_cond_destroy()?
> 

4. Why did you ignore that comment?

>> +
>>  #if defined(__cplusplus)
>>  } /* extern "C" */
>>  #endif /* defined(__cplusplus) */
>> diff --git a/src/lib/core/cbus.c b/src/lib/core/cbus.c
>> index b3b1280e7..b7e6d769b 100644
>> --- a/src/lib/core/cbus.c
>> +++ b/src/lib/core/cbus.c
>> @@ -284,6 +284,9 @@ cpipe_flush_cb(ev_loop *loop, struct ev_async *watcher, int events)
>>  	/* Trigger task processing when the queue becomes non-empty. */
>>  	bool output_was_empty;
>>  
>> +	int old_cancel_state;
>> +	pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &old_cancel_state);
> 
> 28. Why?
> 

5. Why did you ignore that comment?

>> +
>>  	tt_pthread_mutex_lock(&endpoint->mutex);
>>  	output_was_empty = stailq_empty(&endpoint->output);
>>  	/** Flush input */
>> @@ -297,6 +300,7 @@ cpipe_flush_cb(ev_loop *loop, struct ev_async *watcher, int events)
>>  
>>  		ev_async_send(endpoint->consumer, &endpoint->async);
>>  	}
>> +	pthread_setcancelstate(old_cancel_state, NULL);
>>  }
> 
> 29. I will review the tests later, when I will fully understand the
> code. But so few tests for such a serious feature looks not enough.
> Could you test it more rigorous?
> 

6. So have you succeed in that hacking? I am mostly
interested in tests about slow replication with fast
WAL writing.

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v2 4/4] replication: use wal memory buffer to fetch rows

[Vladislav Shpilevoy]

On 23/09/2019 23:51, Vladislav Shpilevoy wrote:
> Thanks for the answers.
> 
>>>> +			struct vclock *send_vclock;
>>>> +			if (relay->version_id < version_id(1, 7, 4))
>>>> +				send_vclock = &relay->r->vclock;
>>>
>>> 3. I know, this is old code existed before your patch, and it will
>>> be dropped, but why do you send relay->r->vclock and don't decode it
>>> anywhere? Above you decoded a received vclock into relay->recv_vclock.
>>> relay->r->vclock is not updated, and nonetheless you send it.
>> Before 1.7.4 we used recovery vclock as replica vclock. So recovery does the 
>> work.
> 
> It *did* the work, but now it doesn't. Recovery vclock was being
> updated in relay_process_wal_event(). Now you relay directly
> from memory, and recovery vclock is not changed until you fall
> to disk. Moreover, looks like recovery object does not exist until
> you fall to disk. It means, that to 1.7.4 you would either crash or
> send the same vclock until a next fall to disk, wouldn't you?
> 

Any news here?