[tarantool-patches] Re: [PATCH 04/10] Get rid of autocommit from a txn structure

[Konstantin Osipov]

* Georgy Kirichenko <georgy at tarantool.org> [19/04/19 19:59]:
>  box_process_rw(struct request *request, struct space *space,
>  	       struct tuple **result)
>  {
> +	struct tuple *tuple = NULL;
> +	struct txn *txn = in_txn();
> +	bool autocommit = txn == NULL;
> +	if (txn == NULL && (txn = txn_begin()) == NULL)

nit: why not use if (autocommit && (txn == txn_begin()) == NULL)
instead
> +		return -1;
>  	assert(iproto_type_is_dml(request->type));
>  	rmean_collect(rmean_box, request->type, 1);
>  	if (access_check_space(space, PRIV_W) != 0)
> -		return -1;
> -	struct txn *txn = txn_begin_stmt(space);
> -	if (txn == NULL)
> -		return -1;
> -	struct tuple *tuple;
> +		goto fail;
> +	if (txn_begin_stmt(space) == NULL)
> +		goto fail;

I guess txn_begin_stmt can as well receive txn explicitly now, 
since we always fetch fiber->txn before calling txn_begin_stmt().
This will save us one fiber key fetch.

>  	if (space_execute_dml(space, txn, request, &tuple) != 0) {
>  		txn_rollback_stmt();
> -		return -1;
> +		goto fail;
>  	}
> -	if (result == NULL)
> -		return txn_commit_stmt(txn, request);
> -	*result = tuple;
> -	if (tuple == NULL)
> -		return txn_commit_stmt(txn, request);
>  	/*
>  	 * Pin the tuple locally before the commit,
>  	 * otherwise it may go away during yield in
>  	 * when WAL is written in autocommit mode.
>  	 */
> -	tuple_ref(tuple);
> -	int rc = txn_commit_stmt(txn, request);
> -	if (rc == 0)
> +	if (tuple != NULL)
> +		tuple_ref(tuple);
> +
> +	if (result != NULL)
> +		*result = tuple;
> +
> +	if (txn_commit_stmt(txn, request) != 0)
> +		goto fail;
> +	if (autocommit) {
> +		if (txn_commit(txn) != 0)
> +			goto fail;
> +		fiber_gc();
> +	}
> +
> +	if (tuple != NULL && result != NULL) {
>  		tuple_bless(tuple);
> -	tuple_unref(tuple);

The code here now looks quite messy. I think it would be easier to
read if we split it between if (autocommit) ... else ... branches:

if (tuple == NULL) {
    return is_autocommit ? txn_commit_stmt() : txn_commit();
}
tuple_ref(tuple);

if (txn_commit_stmt(txn, request))
    goto fail;
if (is_autocommit)
    if (txn_commit())
        goto fail;
    fiber_gc();
}
tuple_bless(tuple);
if (result)
    *result = tuple;
tuple_unref(tuple);
return 0;

The patch is generally looking good, please fix the above nits and
it will be good to go.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov