[tarantool-patches] [PATCH v3 05/14] txn: get rid of autocommit from a txn structure
Vladimir Davydov
vdavydov.dev at gmail.com
Thu Jun 13 17:11:02 MSK 2019
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.
See a few comments inline.
>
> Prerequisites: #1254
> ---
> src/box/applier.cc | 35 +++++++++++++---
> src/box/box.cc | 94 +++++++++++++++++++++++++++++++++---------
> src/box/index.cc | 10 ++---
> src/box/memtx_engine.c | 10 ++++-
> src/box/memtx_space.c | 8 ++--
> src/box/sql.c | 2 +-
> src/box/txn.c | 46 +++++++--------------
> src/box/txn.h | 16 +++----
> src/box/vinyl.c | 12 ++----
> src/box/vy_scheduler.c | 6 +--
> 10 files changed, 148 insertions(+), 91 deletions(-)
>
> diff --git a/src/box/applier.cc b/src/box/applier.cc
> index 373e1feb9..e3203a4c8 100644
> --- a/src/box/applier.cc
> +++ b/src/box/applier.cc
> @@ -172,11 +172,26 @@ applier_writer_f(va_list ap)
> static int
> 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)
> + goto 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))
> + goto rollback;
> + int rc;
> + rc = txn_commit(txn);
> + if (rc < 0)
> + return -1;
> + fiber_gc();
fiber_gc() must be called after txn_commit() while txn_rollback() calls
it by itself. This is a mess. As a matter of fact, I don't understand
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.
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.
> @@ -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).
> 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.
> @@ -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.
> 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.
> @@ -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.
> 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.
> 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.
> 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.
> 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?
More information about the Tarantool-patches
mailing list