Re: [Tarantool-patches] [PATCH v1 1/1] sql: do not reset region on select

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the fixes!

See 4 comments below.

> diff --git a/src/box/txn.h b/src/box/txn.h
> index dfa7c0ee4..1a5ec975e 100644
> --- a/src/box/txn.h
> +++ b/src/box/txn.h
> @@ -258,6 +258,13 @@ struct txn_savepoint {
>  	char name[1];
>  };
>  
> +struct txn_ro_savepoint {

1. Please, add a comment to this structure why does it exist.
So far there was no a single comment anywhere why do we need
RO statements/savepoints. Worth changing it while we are here.

> +	/** Region used during this transaction. */
> +	struct region *region;
> +	/** Savepoint for region. */
> +	size_t region_used;
> +};
> +
>  extern double too_long_threshold;
>  
>  /**
> @@ -557,24 +564,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. What exactly do the ro requests allocate on the fiber region?
I couldn't find any examples in memtx, but I didn't look too deep.

>  	}
>  }
>  
> diff --git a/test/sql/gh-5427-lua-func-changes-result.result b/test/sql/gh-5427-lua-func-changes-result.result
> new file mode 100644
> index 000000000..f2e777a0e
> --- /dev/null
> +++ b/test/sql/gh-5427-lua-func-changes-result.result
> @@ -0,0 +1,153 @@
> +-- test-run result file version 2
> +test_run = require('test_run').new()
> + | ---
> + | ...
> +engine = test_run:get_cfg('engine')
> + | ---
> + | ...
> +_ = box.space._session_settings:update('sql_default_engine', {{'=', 2, engine}})
> + | ---
> + | ...
> +
> +box.execute([[CREATE TABLE t (b STRING PRIMARY KEY);]])
> + | ---
> + | - row_count: 1
> + | ...
> +box.execute([[INSERT INTO t VALUES ('aaaaaaaaaaaa');]])
> + | ---
> + | - row_count: 1
> + | ...
> +test_run:cmd("setopt delimiter ';'");
> + | ---
> + | - true
> + | ...
> +box.schema.func.create('CORRUPT_SELECT', {
> +    language = 'LUA',
> +    returns = 'integer',
> +    body = [[
> +        function(x)
> +            box.space.T:select()
> +            return 1
> +        end]],
> +    is_sandboxed = false,
> +    param_list = { "string" },
> +    exports = { 'LUA', 'SQL' },
> +    is_deterministic = false,
> +    if_not_exists = true
> +});
> + | ---
> + | ...
> +box.schema.func.create('CORRUPT_GET', {
> +    language = 'LUA',
> +    returns = 'integer',
> +    body = [[
> +        function(x)
> +            box.space.T:get('aaaaaaaaaaaa')
> +            return 1
> +        end]],
> +    is_sandboxed = false,
> +    param_list = { "string" },
> +    exports = { 'LUA', 'SQL' },
> +    is_deterministic = false,
> +    if_not_exists = true
> +});
> + | ---
> + | ...
> +box.schema.func.create('CORRUPT_COUNT', {
> +    language = 'LUA',
> +    returns = 'integer',
> +    body = [[
> +        function(x)
> +            box.space.T:count()
> +            return 1
> +        end]],
> +    is_sandboxed = false,
> +    param_list = { "string" },
> +    exports = { 'LUA', 'SQL' },
> +    is_deterministic = false,
> +    if_not_exists = true
> +});
> + | ---
> + | ...
> +box.schema.func.create('CORRUPT_MAX', {
> +    language = 'LUA',
> +    returns = 'integer',
> +    body = [[
> +        function(x)
> +            box.space.T.index[0]:max()
> +            return 1
> +        end]],
> +    is_sandboxed = false,
> +    param_list = { "string" },
> +    exports = { 'LUA', 'SQL' },
> +    is_deterministic = false,
> +    if_not_exists = true
> +});
> + | ---
> + | ...
> +box.schema.func.create('CORRUPT_MIN', {
> +    language = 'LUA',
> +    returns = 'integer',
> +    body = [[
> +        function(x)
> +            box.space.T.index[0]:min()
> +            return 1
> +        end]],
> +    is_sandboxed = false,
> +    param_list = { "string" },
> +    exports = { 'LUA', 'SQL' },
> +    is_deterministic = false,
> +    if_not_exists = true
> +});
> + | ---
> + | ...
> +test_run:cmd("setopt delimiter ''");
> + | ---
> + | - true
> + | ...
> +
> +box.execute([[select CORRUPT_SELECT(t.b) from t where t.b = ? and t.b <= ? order by t.b;]], {"aaaaaaaaaaaa", "aaaaaaaaaaaa"})
> + | ---
> + | - metadata:
> + |   - name: COLUMN_1
> + |     type: integer
> + |   rows:
> + |   - [1]
> + | ...
> +box.execute([[select CORRUPT_GET(t.b) from t where t.b = ? and t.b <= ? order by t.b;]], {"aaaaaaaaaaaa", "aaaaaaaaaaaa"})
> + | ---
> + | - metadata:
> + |   - name: COLUMN_1
> + |     type: integer
> + |   rows:
> + |   - [1]
> + | ...
> +box.execute([[select CORRUPT_COUNT(t.b) from t where t.b = ? and t.b <= ? order by t.b;]], {"aaaaaaaaaaaa", "aaaaaaaaaaaa"})
> + | ---
> + | - metadata:
> + |   - name: COLUMN_1
> + |     type: integer
> + |   rows:
> + |   - [1]
> + | ...
> +box.execute([[select CORRUPT_MAX(t.b) from t where t.b = ? and t.b <= ? order by t.b;]], {"aaaaaaaaaaaa", "aaaaaaaaaaaa"})
> + | ---
> + | - metadata:
> + |   - name: COLUMN_1
> + |     type: integer
> + |   rows:
> + |   - [1]
> + | ...
> +box.execute([[select CORRUPT_MIN(t.b) from t where t.b = ? and t.b <= ? order by t.b;]], {"aaaaaaaaaaaa", "aaaaaaaaaaaa"})

3. Please, keep 80 symbols line width limit.

> + | ---
> + | - metadata:
> + |   - name: COLUMN_1
> + |     type: integer
> + |   rows:
> + |   - [1]
> + | ...
> +
> +box.execute([[DROP TABLE t;]])

4. You also need to drop the functions.

> + | ---
> + | - row_count: 1
> + | ...