[tarantool-patches] Re: [PATCH v2 2/8] Get rid of autocommit from a txn structure

[Konstantin Osipov <kostja@tarantool.org>]

* Georgy Kirichenko <georgy@tarantool.org> [19/05/23 12:14]:
  apply_initial_join_row(struct xrow_header *row)
>  {
> +	struct txn *txn = txn_begin();
> +	if (txn == NULL)
> +		return -1;
>  	struct request request;
>  	xrow_decode_dml(row, &request, dml_request_key_map(row->type));
> -	struct space *space = space_cache_find_xc(request.space_id);
> +	struct space *space = space_cache_find(request.space_id);
> +	if (space == NULL)
> +		return -1;

Missing txn_rollback().

>  	/* no access checks here - applier always works with admin privs */
> -	return space_apply_initial_join_row(space, &request);
> +	if (space_apply_initial_join_row(space, &request)) {
> +		txn_rollback();
> +		return -1;
> +	}
> +	int rc = txn_commit(txn);
> +	fiber_gc();
> +	return rc;
>  }
>  
>  /**
> @@ -189,8 +200,8 @@ static int
>  box_process_rw(struct request *request, struct space *space,
>  	       struct tuple **result)
>  {
> +	struct tuple *tuple = NULL;
> +	struct txn *txn = in_txn();
> +	bool is_autocommit = txn == NULL;
> +	if (is_autocommit && (txn = txn_begin()) == NULL)
> +		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(txn, space) == NULL)
> +		goto fail;
>  	if (space_execute_dml(space, txn, request, &tuple) != 0) {
> -		txn_rollback_stmt();
> -		return -1;
> +		txn_rollback_stmt(txn);
> +		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.
>  	 */

Please move the comment to its place, where we actually pin the
tuple.

> +	if (result != NULL)
> +		*result = tuple;
> +
> +	if (result == NULL || tuple == NULL) {
> +		if (!is_autocommit)
> +			return txn_commit_stmt(txn, request);
> +		/* Autocommit mode. */
> +		if (txn_commit_stmt(txn, request) != 0) {
> +			txn_rollback();
> +			return -1;
> +		}
> +		if (txn_commit(txn) != 0)
> +			return -1;
> +		fiber_gc();
> +		return 0;
> +	}

This code flow can be a bit more straightforward, I sent
a diff to the list in a separate email.

>  	tuple_ref(tuple);
> -	int rc = txn_commit_stmt(txn, request);
> -	if (rc == 0)
> -		tuple_bless(tuple);
> +
> +	if (txn_commit_stmt(txn, request)) {
> +		/* Unref tuple and rollback if autocommit. */
> +		tuple_unref(tuple);
> +		goto fail;
> +	}
> +	if (is_autocommit) {
> +		if (txn_commit(txn) != 0) {
> +			/* Unref tuple and exit. */
> +			tuple_unref(tuple);
> +			return -1;
> +		}
> +	        fiber_gc();
> +	}
> +	tuple_bless(tuple);
>  	tuple_unref(tuple);
> -	return rc;
> +	return 0;
> +
> +fail:
> +	if (is_autocommit)
> +		txn_rollback();
> +	return -1;
>  }
>  
>  void
> @@ -299,8 +330,12 @@ apply_wal_row(struct xstream *stream, struct xrow_header *row)
>  	xrow_decode_dml_xc(row, &request, dml_request_key_map(row->type));
>  	if (request.type != IPROTO_NOP) {
>  		struct space *space = space_cache_find_xc(request.space_id);
> -		if (box_process_rw(&request, space, NULL) != 0) {
> +		struct txn *txn = txn_begin();
> +		if (txn == NULL || box_process_rw(&request, space, NULL) != 0 ||
> +		    txn_commit(txn) != 0) {
>  			say_error("error applying row: %s", request_str(&request));
> +			if (txn != NULL)
> +				txn_rollback();

Missing fiber_gc() after txn_commit()

>  			diag_raise();
>  		}
>  	}
>  	txn_commit_ro_stmt(txn);
> @@ -1313,8 +1348,15 @@ box_sequence_reset(uint32_t seq_id)
>  static inline void
>  box_register_replica(uint32_t id, const struct tt_uuid *uuid)
>  {
> +	struct txn *txn = txn_begin();
> +	if (txn == NULL)
> +		diag_raise();
>  	if (boxk(IPROTO_INSERT, BOX_CLUSTER_ID, "[%u%s]",
> -		 (unsigned) id, tt_uuid_str(uuid)) != 0)
> +		 (unsigned) id, tt_uuid_str(uuid)) != 0) {
> +		txn_rollback();
> +		diag_raise();
> +	}
> +	if (txn_commit(txn) != 0)
>  		diag_raise();

Missing fiber_gc() after txn_commit().

>  	assert(replica_by_uuid(uuid)->id == id);
>  }
> @@ -1636,9 +1678,16 @@ box_set_replicaset_uuid(const struct tt_uuid *replicaset_uuid)
>  		uu = *replicaset_uuid;
>  	else
>  		tt_uuid_create(&uu);
> +	struct txn *txn = txn_begin();
> +	if (txn == NULL)
> +		diag_raise();
>  	/* Save replica set UUID in _schema */
>  	if (boxk(IPROTO_INSERT, BOX_SCHEMA_ID, "[%s%s]", "cluster",
> -		 tt_uuid_str(&uu)))
> +		 tt_uuid_str(&uu))) {
> +		txn_rollback();
> +		diag_raise();
> +	}
> +	if (txn_commit(txn) != 0)
>  		diag_raise();

Missing fiber_gc().

> --- a/src/box/journal.h
> +++ b/src/box/journal.h
> @@ -32,6 +32,7 @@
>   */
>  #include <stdint.h>
>  #include <stdbool.h>
> +#include "trigger.h"
>  #include "salad/stailq.h"

Stray change, please remove.
>  
> -	if (!txn->is_autocommit) {
> -		trigger_create(&txn->fiber_on_stop, txn_on_stop, NULL, NULL);
> -		trigger_add(&fiber()->on_stop, &txn->fiber_on_stop);
> -	}
> +	trigger_create(&txn->fiber_on_stop, txn_on_stop, NULL, NULL);
> +	trigger_add(&fiber()->on_stop, &txn->fiber_on_stop);

Come to think of it, I see no reason why txn->is_autocommit flag
is removed. We can pass is_autocommit property to txn, even though
it doesn't behave in a special way on autocommit. Or we could
rename the option to set_on_yield_on_stop. 

Now the triggers are set and cleared even for trivial statements
using memtx. This could be a performance issue. Did you benchmark
your patch, it may introduce a yet another regression. Please ask
@avitikhon to help with benchmarks, if there is a slow down from
setting the triggers all the time, we can fix the patch  to not do
it, otherwise it's ok to simply set the triggers always.

> --- a/src/box/wal.c
> +++ b/src/box/wal.c
> @@ -303,7 +303,6 @@ tx_schedule_rollback(struct cmsg *msg)
>  	 * in-memory database state.
>  	 */
>  	stailq_reverse(&writer->rollback);
> -	/* Must not yield. */
>  	tx_schedule_queue(&writer->rollback);
>  	stailq_create(&writer->rollback);
>  	if (msg != &writer->in_rollback)

Please explain in the changeset comment why you removed this
comment. Or is this a stray change?

-- 
Konstantin Osipov, Moscow, Russia