[tarantool-patches] Re: [PATCH v2 3/8] Get rid of fiber_gc from txn_rollback

[Konstantin Osipov]

* Georgy Kirichenko <georgy at tarantool.org> [19/05/23 12:14]:
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 2a738fa36..4d6515dd4 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -197,7 +197,7 @@ box_process_rw(struct request *request, struct space *space,
>  			return txn_commit_stmt(txn, request);
>  		/* Autocommit mode. */
>  		if (txn_commit_stmt(txn, request) != 0) {
> -			txn_rollback();
> +			txn_rollback(txn);

Please add fiber_gc() here.

>  			return -1;
>  		}
>  		if (txn_commit(txn) != 0)
> --- a/src/box/call.c
> +++ b/src/box/call.c
> @@ -209,13 +209,17 @@ box_process_call(struct call_request *request, struct port *port)
>  		fiber_set_user(fiber(), orig_credentials);
>  
>  	if (rc != 0) {
> -		txn_rollback();
> +		if (in_txn() != NULL)
> +			txn_rollback(in_txn());
> +		fiber_gc();
>  		return -1;
>  	}
>  
>  	if (in_txn()) {
>  		diag_set(ClientError, ER_FUNCTION_TX_ACTIVE);
> -		txn_rollback();
> +		if (in_txn() != NULL)
> +			txn_rollback(in_txn());

in_txn() is expensive and can not be optimzied out, please use a
local variable.

> +		fiber_gc();
>  		return -1;
>  	}
>  
> @@ -230,13 +234,17 @@ box_process_eval(struct call_request *request, struct port *port)
>  	if (access_check_universe(PRIV_X) != 0)
>  		return -1;
>  	if (box_lua_eval(request, port) != 0) {
> -		txn_rollback();
> +		if (in_txn() != NULL)
> +			txn_rollback(in_txn());
> +		fiber_gc();

Same here and in other places.

>  		return -1;
>  	}
>  
> index 87fd8b3bc..f677e1d33 100644
> --- a/src/box/txn.c
> +++ b/src/box/txn.c
> @@ -395,7 +395,7 @@ txn_commit(struct txn *txn)
>  	if (txn->n_new_rows + txn->n_applier_rows > 0) {
>  		txn->signature = txn_write_to_wal(txn);
>  		if (txn->signature < 0)
> -			goto fail;
> +			return -1;

Why this change?

>  	}
>  	/*
>  	 * The transaction is in the binary log. No action below
> @@ -423,7 +423,7 @@ txn_commit(struct txn *txn)
>  	txn_free(txn);
>  	return 0;
>  fail:
> -	txn_rollback();
> +	txn_rollback(txn);
>  	return -1;
>  }
>  
> @@ -437,11 +437,9 @@ txn_rollback_stmt(struct txn *txn)
>  }
>  
> @@ -534,11 +530,14 @@ int
>  box_txn_rollback()
>  {
>  	struct txn *txn = in_txn();
> +	if (txn == NULL)
> +		return 0;
>  	if (txn && txn->in_sub_stmt) {
>  		diag_set(ClientError, ER_ROLLBACK_IN_SUB_STMT);
>  		return -1;
>  	}
> -	txn_rollback(); /* doesn't throw */
> +	txn_rollback(txn); /* doesn't throw */
> +	fiber_gc();

box_txn_rollback() is used in a bunch of places in sql. please fix
it to use txn_rollback() and an explicit fiber_gc as well in a
follow up patch.

Thanks!

-- 
Konstantin Osipov, Moscow, Russia