[tarantool-patches] Re: [PATCH v3 05/14] txn: get rid of autocommit from a txn structure

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev at gmail.com> [19/06/13 17:18]:
> On Sun, Jun 09, 2019 at 11:44:34PM +0300, Georgy Kirichenko wrote:
> > Move transaction auto start and auto commit behavior to the box level.
> > From now a transaction won't start and commit automatically without
> > txn_begin/txn_commit invocations. This is a part of a bigger transaction
> > refactoring in order to implement detachable transactions and a parallel
> > applier.
> 
> TBO I don't understand why you need to move autocommit logic out of txn.
> The applier code already calls txn_begin/txn_commit explicitly and AFAIU
> this is the only place you need to care about. The code looks less
> readable and more bulky after this change IMHO. Let's discuss f2f.

We duplicate autocommit logic on box and SQL level, this is
confusing. This is why we decided to get rid of it altogether in
box. I think when
the refactoring is finished the box code won't look that ugly,
especially after your suggestions.

> fiber_gc() must be called after txn_commit() while txn_rollback() calls
> it by itself. This is a mess.

It is temporary, the patch tries to do equivalent transformations
of the source code, the issue is addressed by a subsequent patch.

> why we need to call this function at all now, when we have txn->region
> which is used to allocate all txn stuff. AFAIU fiber->gc is only used
> for allocating temporary objects, in which case the caller is supposed
> to clean up by himself (using region_truncate). That said, I don't think
> we need to call fiber_gc() after txn_commit() at all.

Indeed, when most of transaction memory is allocated on txn gc,
txn_commit() will free that memory automatically.

> There's one exception though - it's vdbe_add_new_autoinc_id, which
> stores autogenerated ids on fiber->gc and doesn't delete them until
> the corresponding SQL request is complete (this is why all this mess
> with fiber_gc() started AFAIR). However, I think we could and should
> use a mempool for such allocations.
> 
> I think that before pushing this patch, we should clean up fiber_gc()
> usage, otherwise it's really easy to loose track where we must and where
> we mustn't call fiber_gc() now. Actually, I think we should do that
> irrespective of whether we commit this patch or not.

No, I disagree, that would be unnecessary work, we just need to
finish the patch set and push it.

> > @@ -403,8 +418,16 @@ applier_join(struct applier *applier)
> >  		applier->last_row_time = ev_monotonic_now(loop());
> >  		if (iproto_type_is_dml(row.type)) {
> >  			vclock_follow_xrow(&replicaset.vclock, &row);
> > -			if (apply_row(&row) != 0)
> > +			struct txn *txn = txn_begin();
> > +			if (txn == NULL)
> > +				diag_raise();
> > +			if (apply_row(&row) != 0) {
> > +				txn_rollback();
> > +				diag_raise();
> > +			}
> > +			if (txn_commit(txn) != 0)
> >  				diag_raise();
> > +			fiber_gc();
> 
> I'd wrap this into a helper function (apply_final_join_row).

Ok, why not.

> > diff --git a/src/box/box.cc b/src/box/box.cc
> > index 57419ee01..7f23716e5 100644
> > --- a/src/box/box.cc
> > +++ b/src/box/box.cc
> > @@ -169,34 +169,62 @@ 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)
> > +		*result = tuple;
> > +
> > +	if (result == NULL || tuple == NULL) {
> > +		if (txn_commit_stmt(txn, request) != 0)
> > +			goto fail;
> > +		if (is_autocommit) {
> > +			if (txn_commit(txn) != 0)
> > +				return -1;
> > +			fiber_gc();
> > +		}
> > +		return 0;
> >  	}
> > -	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)
> > -		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);
> 
> This looks like a lot of copy-n-paste. Please think on how to get rid of
> code duplication.

This is already after my changes are applied, I think the place
needs to be revisited when txn alloc is in, not in the middle of a
patch stack.


> > @@ -299,10 +327,20 @@ 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);
> > +		struct txn *txn = txn_begin();
> > +		if (txn == NULL) {
> > +			say_error("error applying row: %s", request_str(&request));
> > +			diag_raise();
> > +		}
> 
> Why? box_process_rw can handle autocommit statements.

Uhm, good point, Let's try to not add transaction management here. 

> 
> >  		if (box_process_rw(&request, space, NULL) != 0) {
> >  			say_error("error applying row: %s", request_str(&request));
> > +			txn_rollback();
> >  			diag_raise();
> >  		}
> > +		if (txn_commit(txn) != 0) {
> > +			diag_raise();
> > +		}
> > +		fiber_gc();
> >  	}
> >  	struct wal_stream *xstream =
> >  		container_of(stream, struct wal_stream, base);
> > @@ -1313,9 +1351,17 @@ 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]",
> 
> Again, boxk call box_process_rw, which handles autocommit. No need to
> call txn_begin/txn_commit here explicitly.

Agree.

> > @@ -1636,10 +1682,18 @@ 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",
> 
> Same here.

Agree.

> > diff --git a/src/box/index.cc b/src/box/index.cc
> > index 4a444e5d0..7f26c9bc2 100644
> > --- a/src/box/index.cc
> > +++ b/src/box/index.cc
> > @@ -274,7 +274,7 @@ box_index_min(uint32_t space_id, uint32_t index_id, const char *key,
> >  	if (txn_begin_ro_stmt(space, &txn) != 0)
> >  		return -1;
> >  	if (index_min(index, key, part_count, result) != 0) {
> > -		txn_rollback_stmt();
> > +		txn_rollback_stmt(txn);
> 
> In general I like that txn_rollback_stmt and txn_rollback now take txn.
> This makes the API more symmetric. Probably, worth doing in a separate
> patch, even if we realize we don't need this one. As a matter of fact,
> doing this in a separate patch would make review easier. I'd appreciate
> if you could split this patch.

Agree.

> > diff --git a/src/box/memtx_engine.c b/src/box/memtx_engine.c
> > index f4312484a..149215b87 100644
> > --- a/src/box/memtx_engine.c
> > +++ b/src/box/memtx_engine.c
> > @@ -272,16 +272,22 @@ memtx_engine_recover_snapshot_row(struct memtx_engine *memtx,
> >  		diag_set(ClientError, ER_CROSS_ENGINE_TRANSACTION);
> >  		return -1;
> >  	}
> > +	struct txn *txn = txn_begin();
> > +	if (txn == NULL)
> > +		return -1;
> >  	/* no access checks here - applier always works with admin privs */
> > -	if (space_apply_initial_join_row(space, &request) != 0)
> > +	if (space_apply_initial_join_row(space, &request) != 0) {
> > +		txn_rollback();
> >  		return -1;
> > +	}
> > +	int rc = txn_commit(txn);
> 
> This is the second place where you surround apply_initial_join_row with
> txn_begin/txn_commit. May be, worth doing it in the callback itself?
> Note, we only need this in case of memtx - vinyl doesn't create a txn
> object for initial recover or join.

Agree.

> 
> > diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c
> > index 1d209033c..1e630783b 100644
> > --- a/src/box/memtx_space.c
> > +++ b/src/box/memtx_space.c
> > @@ -310,10 +310,10 @@ memtx_space_apply_initial_join_row(struct space *space, struct request *request)
> >  		return -1;
> >  	}
> >  	request->header->replica_id = 0;
> > -	struct txn *txn = txn_begin_stmt(space);
> > -	if (txn == NULL)
> > +	struct txn *txn = in_txn();
> > +	struct txn_stmt *stmt = txn_begin_stmt(txn, space);
> > +	if (stmt == NULL)
> >  		return -1;
> > -	struct txn_stmt *stmt = txn_current_stmt(txn);
> 
> AFAICS this is the only place where you use txn_stmt returned by
> txn_begin_stmt() - in other places you simply check if it's NULL (which
> indicates an error). Why did you decide to make txn_begin_stmt() return
> txn_stmt rather than an error code? The API looks lopsided now:
> 
>   txn_begin() => txn
>   txn_begin_stmt(txn, request) => txn_stmt
>   txn_commit_stmt(txn) => int
>   txn_commit(txn) => int
> 
> Let's please make txn_begin_stmt return int and use txn_current_stmt
> here to get txn_stmt, as we used to. This shouldn't affect performance
> while the code would look neater this way.

Agree.

> 
> > diff --git a/src/box/txn.c b/src/box/txn.c
> > index 1d8271e51..21f34e526 100644
> > --- a/src/box/txn.c
> > +++ b/src/box/txn.c
> > @@ -441,14 +428,11 @@ fail:
> >  }
> >  
> >  void
> > -txn_rollback_stmt()
> > +txn_rollback_stmt(struct txn *txn)
> >  {
> > -	struct txn *txn = in_txn();
> >  	if (txn == NULL || txn->in_sub_stmt == 0)
> >  		return;
> 
> Hmm, how's that even possible now?

Nothing changed here, why would this become impossible?

-- 
Konstantin Osipov, Moscow, Russia