From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 9E9F5445320 for ; Thu, 16 Jul 2020 03:09:05 +0300 (MSK) References: <1594821336-14468-1-git-send-email-alyapunov@tarantool.org> <1594821336-14468-8-git-send-email-alyapunov@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Thu, 16 Jul 2020 02:09:04 +0200 MIME-Version: 1.0 In-Reply-To: <1594821336-14468-8-git-send-email-alyapunov@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v3 07/13] txm: save does_require_old_tuple flag in txn_stmt List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Aleksandr Lyapunov , tarantool-patches@dev.tarantool.org Thanks for the patch! See 3 comments below. On 15.07.2020 15:55, Aleksandr Lyapunov wrote: > That flag is needed for transactional conflict manager - if any > other transaction replaces old_tuple before current one and the flag > is set - the current transaction will be aborted. > For example REPLACE just replaces a key, no matter what tuple > lays in the index and thus does_require_old_tuple = false. > In contrast, UPDATE makes new tuple using old_tuple and thus > the statement will require old_tuple (does_require_old_tuple = true). > INSERT also does_require_old_tuple = true because it requires > old_tuple to be NULL. > > Part of #4897 > --- > src/box/memtx_space.c | 17 +++++++++++++++++ > src/box/txn.c | 3 +++ > src/box/txn.h | 13 +++++++++++++ > 3 files changed, 33 insertions(+) > > diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c > index 8452ab4..e48ed3a 100644 > --- a/src/box/memtx_space.c > +++ b/src/box/memtx_space.c > @@ -316,6 +316,10 @@ memtx_space_execute_replace(struct space *space, struct txn *txn, > if (stmt->new_tuple == NULL) > return -1; > tuple_ref(stmt->new_tuple); > + > + if (mode == DUP_INSERT) > + stmt->does_require_old_tuple = true; 1. Yeah, now I understand. NULL is also considered a tuple in txm. Would be nice to have a comment here about it. In other places too, btw. This is not so trivial, and it won't harm to write some explanations. > + > if (memtx_space->replace(space, NULL, stmt->new_tuple, > mode, &stmt->old_tuple) != 0) > return -1; > diff --git a/src/box/txn.c b/src/box/txn.c > index 0372047..d254edb 100644 > --- a/src/box/txn.c > +++ b/src/box/txn.c > @@ -360,6 +361,8 @@ txn_commit_stmt(struct txn *txn, struct request *request) > */ > if (stmt->space != NULL && !rlist_empty(&stmt->space->on_replace) && > stmt->space->run_triggers && (stmt->old_tuple || stmt->new_tuple)) { > + /* Triggers see old_tuple and that tuple must remain the same */ > + stmt->does_require_old_tuple = true; 2. What about before_replace triggers? > int rc = 0; > if(!space_is_temporary(stmt->space)) { > rc = trigger_run(&stmt->space->on_replace, txn); > diff --git a/src/box/txn.h b/src/box/txn.h > index e261852..962ada0 100644 > --- a/src/box/txn.h > +++ b/src/box/txn.h > @@ -175,6 +175,19 @@ struct txn_stmt { > struct xrow_header *row; > /** on_commit and/or on_rollback list is not empty. */ > bool has_triggers; > + /** > + * Whether the stmt requires to replace exactly old_tuple (member). > + * That flag is needed for transactional conflict manager - if any > + * other transaction replaces old_tuple before current one and the flag > + * is set - the current transaction will be aborted. 3. Do you mean the other transaction *commits* replacement of the old_tuple? Because if there was another transaction touched the old tuple, but the current one managed to commit earlier, the current txn won't be aborted. The other txn will be. > + * For example REPLACE just replaces a key, no matter what tuple > + * lays in the index and thus does_require_old_tuple = false. > + * In contrast, UPDATE makes new tuple using old_tuple and thus > + * the statement will require old_tuple (does_require_old_tuple = true). > + * INSERT also does_require_old_tuple = true because it requires > + * old_tuple to be NULL. > + */ > + bool does_require_old_tuple; > /** Commit/rollback triggers associated with this statement. */ > struct rlist on_commit; > struct rlist on_rollback;