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

[Georgy Kirichenko]

On Friday, September 20, 2019 11:23:07 AM MSK Konstantin Osipov wrote:
> * Georgy Kirichenko <georgy at tarantool.org> [19/09/18 12:39]:
> > +int
> > +wal_relay(struct wal_relay *wal_relay, struct vclock *vclock,
> > +	  wal_relay_cb on_wal_relay, void *cb_data, const char *endpoint_name)
> > +{
> > +	wal_relay->vclock = vclock;
> > +	wal_relay->on_wal_relay = on_wal_relay;
> > +	wal_relay->cb_data = cb_data;
> > +	diag_create(&wal_relay->diag);
> > +	wal_relay->cancel_msg.route = NULL;
> > +
> > +	fiber_cond_create(&wal_relay->done_cond);
> > +	wal_relay->done = false;
> > +
> > +	/* Establish a connection with wal thread. */
> > +	cbus_pair("wal", endpoint_name, &wal_relay->wal_pipe,
> > +		  &wal_relay->relay_pipe,
> > +		  wal_relay_attach, wal_relay, cbus_process);
> > +
> > +	while (!wal_relay->done) {
> > +		if (fiber_is_cancelled() &&
> > +		    wal_relay->cancel_msg.route == NULL) {
> > +			/* Send a cancel message to a wal relay fiber. */
> > +			static struct cmsg_hop cancel_route[]= {
> > +				{wal_relay_cancel, NULL}};
> > +			cmsg_init(&wal_relay->cancel_msg, cancel_route);
> > +			cpipe_push(&wal_relay->wal_pipe, &wal_relay-
>cancel_msg);
> > +		}
> > +		fiber_cond_wait(&wal_relay->done_cond);
> > +	}
> > +
> > +	/* Disconnect from wal thread. */
> > +	cbus_unpair(&wal_relay->wal_pipe, &wal_relay->relay_pipe,
> > +		    NULL, NULL, cbus_process);
> > +
> > +	if (!diag_is_empty(&wal_relay->diag)) {
> > +		diag_move(&wal_relay->diag, &fiber()->diag);
> > +		return -1;
> > +	}
> > +	if (fiber_is_cancelled()) {
> > +		diag_set(FiberIsCancelled);
> > +		return -1;
> > +	}
> > +	return 0;
> 
> Very, very, nice. This hunk made me think, why do you create a new
> cbus pair each time? While this puts a very nice stress on the
> pairing code, which few people understand and even some suspect to
> be buggy :), isn't it easier to pair when creating the relay and unpair
> on destroy? What happens when relay is stopped while it's
> pairing? I see that you added pthread_setcancelstate(), but there
> are other reasons a relay may stop, not just cancel. Sounds like
> we need a clear protocol/API for stopping a paired thread, e.g.
> document that before a cord exits, it must unpair all pipes first.
> This can be done by keeping a list of all pairings in the cord and
> asserting that the list is empty in the end of cord function.
> Thoughts?
I do not see any issue here, in normal circumstances relay will have only one 
wal attaching attempt. Also pthread_cancel is called only on tarantool exit 
and affects tx->relay pairing as well.
I think we should completely redesign cbus communication, for instance create 
only one endpoint per each cord and free all pipes and the endpoint on cord 
exit. But this is not in the scope of the current task.