Re: [Tarantool-patches] [PATCH v3 07/13] txm: save does_require_old_tuple flag in txn_stmt

[Vladislav Shpilevoy <v.shpilevoy@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;