Re: [Tarantool-patches] [PATCH] box: do not reset region on select

["Alexander V. Tikhonov" <avtikhon@tarantool.org>]

Hi All, thanks for the patch, as I see no new degradation found in
gitlab-ci testing commit criteria pipeline [1], patch LGTM.

[1] - https://gitlab.com/tarantool/tarantool/-/pipelines/235247720

On Tue, Dec 29, 2020 at 04:46:35AM +0300, Alexander Turenko wrote:
> From: Mergen Imeev <imeevma@gmail.com>
> 
> A read only transaction should not invalidate the fiber region.
> 
> The problem was found in context of calling a Lua function from SQL on
> tarantool 2.* (#5427).
> 
> Another problem (#5623) was fixed by this commit on tarantool 2.*. Since
> 2.2.0-469-g707e58a3f ('box: rework box_lua_{call, eval} to use input
> port') arguments for a C stored procedure is allocated on a fiber
> region. The region is truncated at the end of the call. When the
> procedure performs a read only transaction, fiber_gc() may be called
> and so region_truncate() will fail on assertion (on Debug build).
> 
> Despite that all found cases are specific for tarantool 2.*, it is
> possible to be hit by the problem on 1.10 as well. It is enough to
> allocate something on a region (explicitly or implicitly) and perform a
> read only transaction.
> 
> So it looks worthful to backport the fix to 1.10 as well.
> 
> (cherry picked from commit d0d668fa014763adefb71bfeddab1ae5a5350fce)
> 
> The original commit message:
> 
>  | sql: do not reset region on select
>  |
>  | Prior to this patch, region on fiber was reset during select(), get(),
>  | count(), max(), or min(). This would result in an error if one of these
>  | operations was used in a user-defined function in SQL. After this patch,
>  | these functions truncate region instead of resetting it.
>  |
>  | Closes #5427
> ---
> 
> Hi!
> 
> Mergen, Vlad, Alexander, Kirill,
> 
> I want to fix the problem on 1.10 as well, prior to 1.10.9 release. Are
> there any objections?
> 
> The patch is cherry picked without changes, except removed src/box/sql
> files, removed test (it is SQL specific) and rewritten commit message.
> 
> https://github.com/tarantool/tarantool/issues/5427
> https://github.com/tarantool/tarantool/issues/5623
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-5427-ro-txn-dont-invalidate-fiber-region-1.10
> 
> Fails on GitHub Actions runners on
> app-tap/gh-4983-tnt-e-assert-false-hangs.test.lua are expected (and we
> work on them).
> 
> @ChangeLog (somewhere in 'Core'): Don't invalidate the fiber region
> (the box region) on a read only transaction (gh-5427, gh-5623).
> 
>  src/box/box.cc   |  5 +++--
>  src/box/index.cc | 25 +++++++++++++++----------
>  src/box/txn.h    | 23 ++++++++++++++++++++---
>  3 files changed, 38 insertions(+), 15 deletions(-)
> 
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 58011d44c..0f6761cf6 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -1070,7 +1070,8 @@ box_select(uint32_t space_id, uint32_t index_id,
>  	});
>  
>  	struct txn *txn;
> -	if (txn_begin_ro_stmt(space, &txn) != 0)
> +	struct txn_ro_savepoint svp;
> +	if (txn_begin_ro_stmt(space, &txn, &svp) != 0)
>  		return -1;
>  
>  	struct iterator *it = index_create_iterator(index, type,
> @@ -1104,7 +1105,7 @@ box_select(uint32_t space_id, uint32_t index_id,
>  		txn_rollback_stmt();
>  		return -1;
>  	}
> -	txn_commit_ro_stmt(txn);
> +	txn_commit_ro_stmt(txn, &svp);
>  	return 0;
>  }
>  
> diff --git a/src/box/index.cc b/src/box/index.cc
> index 9c4acec1d..bb218061f 100644
> --- a/src/box/index.cc
> +++ b/src/box/index.cc
> @@ -246,13 +246,14 @@ box_index_get(uint32_t space_id, uint32_t index_id, const char *key,
>  		return -1;
>  	/* Start transaction in the engine. */
>  	struct txn *txn;
> -	if (txn_begin_ro_stmt(space, &txn) != 0)
> +	struct txn_ro_savepoint svp;
> +	if (txn_begin_ro_stmt(space, &txn, &svp) != 0)
>  		return -1;
>  	if (index_get(index, key, part_count, result) != 0) {
>  		txn_rollback_stmt();
>  		return -1;
>  	}
> -	txn_commit_ro_stmt(txn);
> +	txn_commit_ro_stmt(txn, &svp);
>  	/* Count statistics. */
>  	rmean_collect(rmean_box, IPROTO_SELECT, 1);
>  	if (*result != NULL)
> @@ -280,13 +281,14 @@ box_index_min(uint32_t space_id, uint32_t index_id, const char *key,
>  		return -1;
>  	/* Start transaction in the engine. */
>  	struct txn *txn;
> -	if (txn_begin_ro_stmt(space, &txn) != 0)
> +	struct txn_ro_savepoint svp;
> +	if (txn_begin_ro_stmt(space, &txn, &svp) != 0)
>  		return -1;
>  	if (index_min(index, key, part_count, result) != 0) {
>  		txn_rollback_stmt();
>  		return -1;
>  	}
> -	txn_commit_ro_stmt(txn);
> +	txn_commit_ro_stmt(txn, &svp);
>  	if (*result != NULL)
>  		tuple_bless(*result);
>  	return 0;
> @@ -312,13 +314,14 @@ box_index_max(uint32_t space_id, uint32_t index_id, const char *key,
>  		return -1;
>  	/* Start transaction in the engine. */
>  	struct txn *txn;
> -	if (txn_begin_ro_stmt(space, &txn) != 0)
> +	struct txn_ro_savepoint svp;
> +	if (txn_begin_ro_stmt(space, &txn, &svp) != 0)
>  		return -1;
>  	if (index_max(index, key, part_count, result) != 0) {
>  		txn_rollback_stmt();
>  		return -1;
>  	}
> -	txn_commit_ro_stmt(txn);
> +	txn_commit_ro_stmt(txn, &svp);
>  	if (*result != NULL)
>  		tuple_bless(*result);
>  	return 0;
> @@ -345,14 +348,15 @@ box_index_count(uint32_t space_id, uint32_t index_id, int type,
>  		return -1;
>  	/* Start transaction in the engine. */
>  	struct txn *txn;
> -	if (txn_begin_ro_stmt(space, &txn) != 0)
> +	struct txn_ro_savepoint svp;
> +	if (txn_begin_ro_stmt(space, &txn, &svp) != 0)
>  		return -1;
>  	ssize_t count = index_count(index, itype, key, part_count);
>  	if (count < 0) {
>  		txn_rollback_stmt();
>  		return -1;
>  	}
> -	txn_commit_ro_stmt(txn);
> +	txn_commit_ro_stmt(txn, &svp);
>  	return count;
>  }
>  
> @@ -381,7 +385,8 @@ box_index_iterator(uint32_t space_id, uint32_t index_id, int type,
>  	if (key_validate(index->def, itype, key, part_count))
>  		return NULL;
>  	struct txn *txn;
> -	if (txn_begin_ro_stmt(space, &txn) != 0)
> +	struct txn_ro_savepoint svp;
> +	if (txn_begin_ro_stmt(space, &txn, &svp) != 0)
>  		return NULL;
>  	struct iterator *it = index_create_iterator(index, itype,
>  						    key, part_count);
> @@ -389,7 +394,7 @@ box_index_iterator(uint32_t space_id, uint32_t index_id, int type,
>  		txn_rollback_stmt();
>  		return NULL;
>  	}
> -	txn_commit_ro_stmt(txn);
> +	txn_commit_ro_stmt(txn, &svp);
>  	rmean_collect(rmean_box, IPROTO_SELECT, 1);
>  	return it;
>  }
> diff --git a/src/box/txn.h b/src/box/txn.h
> index c366f3cab..822d5a7a9 100644
> --- a/src/box/txn.h
> +++ b/src/box/txn.h
> @@ -96,6 +96,20 @@ struct txn_savepoint {
>  	struct stailq_entry *stmt;
>  };
>  
> +/**
> + * Read-only transaction savepoint object. After completing a read-only
> + * transaction, we must clear the region. However, if we just reset the region,
> + * we may corrupt the data that was placed in the region before the read-only
> + * transaction began. To avoid this, we should use truncation. This structure
> + * contains the information required for truncation.
> + */
> +struct txn_ro_savepoint {
> +	/** Region used during this transaction. */
> +	struct region *region;
> +	/** Savepoint for region. */
> +	size_t region_used;
> +};
> +
>  extern double too_long_threshold;
>  
>  struct txn {
> @@ -236,24 +250,27 @@ txn_begin_in_engine(struct engine *engine, struct txn *txn);
>   * select.
>   */
>  static inline int
> -txn_begin_ro_stmt(struct space *space, struct txn **txn)
> +txn_begin_ro_stmt(struct space *space, struct txn **txn,
> +		  struct txn_ro_savepoint *svp)
>  {
>  	*txn = in_txn();
>  	if (*txn != NULL) {
>  		struct engine *engine = space->engine;
>  		return txn_begin_in_engine(engine, *txn);
>  	}
> +	svp->region = &fiber()->gc;
> +	svp->region_used = region_used(svp->region);
>  	return 0;
>  }
>  
>  static inline void
> -txn_commit_ro_stmt(struct txn *txn)
> +txn_commit_ro_stmt(struct txn *txn, struct txn_ro_savepoint *svp)
>  {
>  	assert(txn == in_txn());
>  	if (txn) {
>  		/* nothing to do */
>  	} else {
> -		fiber_gc();
> +		region_truncate(svp->region, svp->region_used);
>  	}
>  }
>  
> -- 
> 2.25.0
>