[tarantool-patches] Re: [PATCH v2 2/8] Get rid of autocommit from a txn structure
Konstantin Osipov
kostja at tarantool.org
Fri May 31 22:21:37 MSK 2019
* Georgy Kirichenko <georgy at 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
More information about the Tarantool-patches
mailing list