Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH v1 1/1] sql: do not reset region on select
@ 2020-11-02  9:49 imeevma
  2020-11-04 22:14 ` Vladislav Shpilevoy
  0 siblings, 1 reply; 10+ messages in thread
From: imeevma @ 2020-11-02  9:49 UTC (permalink / raw)
  To: v.shpilevoy, sergos; +Cc: tarantool-patches

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
---
https://github.com/tarantool/tarantool/issues/5427
https://github.com/tarantool/tarantool/tree/imeevma/gh-5427-lua-func-changes-result

@ChangeLog
 - Region truncated instead of resetting on select() (gh-5427).

 src/box/box.cc                                |   5 +-
 src/box/index.cc                              |  20 +++
 src/box/txn.h                                 |   6 +-
 .../gh-5427-lua-func-changes-result.result    | 153 ++++++++++++++++++
 .../gh-5427-lua-func-changes-result.test.lua  |  86 ++++++++++
 5 files changed, 264 insertions(+), 6 deletions(-)
 create mode 100644 test/sql/gh-5427-lua-func-changes-result.result
 create mode 100644 test/sql/gh-5427-lua-func-changes-result.test.lua

diff --git a/src/box/box.cc b/src/box/box.cc
index 18568df3b..473ef52ad 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -1388,7 +1388,8 @@ box_select(uint32_t space_id, uint32_t index_id,
 	   struct port *port)
 {
 	(void)key_end;
-
+	struct region *region = &fiber()->gc;
+	size_t used = region_used(region);
 	rmean_collect(rmean_box, IPROTO_SELECT, 1);
 
 	if (iterator < 0 || iterator >= iterator_type_MAX) {
@@ -1453,6 +1454,8 @@ box_select(uint32_t space_id, uint32_t index_id,
 		return -1;
 	}
 	txn_commit_ro_stmt(txn);
+	if (txn == NULL)
+		region_truncate(region, used);
 	return 0;
 }
 
diff --git a/src/box/index.cc b/src/box/index.cc
index c2fc00867..e54a64fdd 100644
--- a/src/box/index.cc
+++ b/src/box/index.cc
@@ -228,6 +228,8 @@ box_index_get(uint32_t space_id, uint32_t index_id, const char *key,
 	mp_tuple_assert(key, key_end);
 	struct space *space;
 	struct index *index;
+	struct region *region = &fiber()->gc;
+	size_t used = region_used(region);
 	if (check_index(space_id, index_id, &space, &index) != 0)
 		return -1;
 	if (!index->def->opts.is_unique) {
@@ -246,6 +248,8 @@ box_index_get(uint32_t space_id, uint32_t index_id, const char *key,
 		return -1;
 	}
 	txn_commit_ro_stmt(txn);
+	if (txn == NULL)
+		region_truncate(region, used);
 	/* Count statistics. */
 	rmean_collect(rmean_box, IPROTO_SELECT, 1);
 	if (*result != NULL)
@@ -261,6 +265,8 @@ box_index_min(uint32_t space_id, uint32_t index_id, const char *key,
 	mp_tuple_assert(key, key_end);
 	struct space *space;
 	struct index *index;
+	struct region *region = &fiber()->gc;
+	size_t used = region_used(region);
 	if (check_index(space_id, index_id, &space, &index) != 0)
 		return -1;
 	if (index->def->type != TREE) {
@@ -280,6 +286,8 @@ box_index_min(uint32_t space_id, uint32_t index_id, const char *key,
 		return -1;
 	}
 	txn_commit_ro_stmt(txn);
+	if (txn == NULL)
+		region_truncate(region, used);
 	if (*result != NULL)
 		tuple_bless(*result);
 	return 0;
@@ -293,6 +301,8 @@ box_index_max(uint32_t space_id, uint32_t index_id, const char *key,
 	assert(result != NULL);
 	struct space *space;
 	struct index *index;
+	struct region *region = &fiber()->gc;
+	size_t used = region_used(region);
 	if (check_index(space_id, index_id, &space, &index) != 0)
 		return -1;
 	if (index->def->type != TREE) {
@@ -312,6 +322,8 @@ box_index_max(uint32_t space_id, uint32_t index_id, const char *key,
 		return -1;
 	}
 	txn_commit_ro_stmt(txn);
+	if (txn == NULL)
+		region_truncate(region, used);
 	if (*result != NULL)
 		tuple_bless(*result);
 	return 0;
@@ -331,6 +343,8 @@ box_index_count(uint32_t space_id, uint32_t index_id, int type,
 	enum iterator_type itype = (enum iterator_type) type;
 	struct space *space;
 	struct index *index;
+	struct region *region = &fiber()->gc;
+	size_t used = region_used(region);
 	if (check_index(space_id, index_id, &space, &index) != 0)
 		return -1;
 	uint32_t part_count = mp_decode_array(&key);
@@ -346,6 +360,8 @@ box_index_count(uint32_t space_id, uint32_t index_id, int type,
 		return -1;
 	}
 	txn_commit_ro_stmt(txn);
+	if (txn == NULL)
+		region_truncate(region, used);
 	return count;
 }
 
@@ -367,6 +383,8 @@ box_index_iterator(uint32_t space_id, uint32_t index_id, int type,
 	enum iterator_type itype = (enum iterator_type) type;
 	struct space *space;
 	struct index *index;
+	struct region *region = &fiber()->gc;
+	size_t used = region_used(region);
 	if (check_index(space_id, index_id, &space, &index) != 0)
 		return NULL;
 	assert(mp_typeof(*key) == MP_ARRAY); /* checked by Lua */
@@ -383,6 +401,8 @@ box_index_iterator(uint32_t space_id, uint32_t index_id, int type,
 		return NULL;
 	}
 	txn_commit_ro_stmt(txn);
+	if (txn == NULL)
+		region_truncate(region, used);
 	rmean_collect(rmean_box, IPROTO_SELECT, 1);
 	return it;
 }
diff --git a/src/box/txn.h b/src/box/txn.h
index a51bdf8e6..e7c8766b7 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -568,11 +568,7 @@ static inline void
 txn_commit_ro_stmt(struct txn *txn)
 {
 	assert(txn == in_txn());
-	if (txn) {
-		/* nothing to do */
-	} else {
-		fiber_gc();
-	}
+	(void)txn;
 }
 
 /**
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"})
+ | ---
+ | - metadata:
+ |   - name: COLUMN_1
+ |     type: integer
+ |   rows:
+ |   - [1]
+ | ...
+
+box.execute([[DROP TABLE t;]])
+ | ---
+ | - row_count: 1
+ | ...
diff --git a/test/sql/gh-5427-lua-func-changes-result.test.lua b/test/sql/gh-5427-lua-func-changes-result.test.lua
new file mode 100644
index 000000000..f2699dbe7
--- /dev/null
+++ b/test/sql/gh-5427-lua-func-changes-result.test.lua
@@ -0,0 +1,86 @@
+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);]])
+box.execute([[INSERT INTO t VALUES ('aaaaaaaaaaaa');]])
+test_run:cmd("setopt delimiter ';'");
+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 ''");
+
+box.execute([[select CORRUPT_SELECT(t.b) from t where t.b = ? and t.b <= ? order by t.b;]], {"aaaaaaaaaaaa", "aaaaaaaaaaaa"})
+box.execute([[select CORRUPT_GET(t.b) from t where t.b = ? and t.b <= ? order by t.b;]], {"aaaaaaaaaaaa", "aaaaaaaaaaaa"})
+box.execute([[select CORRUPT_COUNT(t.b) from t where t.b = ? and t.b <= ? order by t.b;]], {"aaaaaaaaaaaa", "aaaaaaaaaaaa"})
+box.execute([[select CORRUPT_MAX(t.b) from t where t.b = ? and t.b <= ? order by t.b;]], {"aaaaaaaaaaaa", "aaaaaaaaaaaa"})
+box.execute([[select CORRUPT_MIN(t.b) from t where t.b = ? and t.b <= ? order by t.b;]], {"aaaaaaaaaaaa", "aaaaaaaaaaaa"})
+
+box.execute([[DROP TABLE t;]])
-- 
2.25.1

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: do not reset region on select
  2020-11-02  9:49 [Tarantool-patches] [PATCH v1 1/1] sql: do not reset region on select imeevma
@ 2020-11-04 22:14 ` Vladislav Shpilevoy
  2020-12-16  5:34   ` Mergen Imeev
  0 siblings, 1 reply; 10+ messages in thread
From: Vladislav Shpilevoy @ 2020-11-04 22:14 UTC (permalink / raw)
  To: imeevma, sergos; +Cc: tarantool-patches

Hi! Thanks for the patch!

See 2 comments below.

On 02.11.2020 10:49, imeevma@tarantool.org wrote:
> 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
> ---
> https://github.com/tarantool/tarantool/issues/5427
> https://github.com/tarantool/tarantool/tree/imeevma/gh-5427-lua-func-changes-result
> 
> @ChangeLog
>  - Region truncated instead of resetting on select() (gh-5427).

1. I suggest to make the message more understandable for users. They have
no idea what a region is. For instance, consider

	Memory corruption when SQL called Lua functions with box calls inside

>  src/box/box.cc                                |   5 +-
>  src/box/index.cc                              |  20 +++
>  src/box/txn.h                                 |   6 +-
>  .../gh-5427-lua-func-changes-result.result    | 153 ++++++++++++++++++
>  .../gh-5427-lua-func-changes-result.test.lua  |  86 ++++++++++
>  5 files changed, 264 insertions(+), 6 deletions(-)
>  create mode 100644 test/sql/gh-5427-lua-func-changes-result.result
>  create mode 100644 test/sql/gh-5427-lua-func-changes-result.test.lua
> 
> diff --git a/src/box/box.cc b/src/box/box.cc
> index 18568df3b..473ef52ad 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -1388,7 +1388,8 @@ box_select(uint32_t space_id, uint32_t index_id,
>  	   struct port *port)
>  {
>  	(void)key_end;
> -
> +	struct region *region = &fiber()->gc;
> +	size_t used = region_used(region);
>  	rmean_collect(rmean_box, IPROTO_SELECT, 1);

2. I must say I couldn't come up with a better idea so far. I remember I
also proposed to start a transaction for each VDBE statement, but not sure
it is a right thing to do.

It means, we probably should go for this one, but a bit polished.

In the current solution I don't like, that you basically just inlined
txn_commit_ro_stmt(), but instead I would rather want you to patch it to
make it work.

So for example txn_begin_ro_stmt inside remembers the current region size,
and returns it via an out parameter struct txn_ro_savepoint *svp. Which
you create on the stack.

Then, after you are done with the statement, you call txn_commit_ro_stmt,
which takes const struct txn_ro_savepoint *svp, and does the region_truncate
inside.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: do not reset region on select
  2020-11-04 22:14 ` Vladislav Shpilevoy
@ 2020-12-16  5:34   ` Mergen Imeev
  2020-12-17 22:04     ` Vladislav Shpilevoy
  0 siblings, 1 reply; 10+ messages in thread
From: Mergen Imeev @ 2020-12-16  5:34 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Hi! Thank you for the review. Sorry for such late reply. My answers and
new patch below.

On Wed, Nov 04, 2020 at 11:14:38PM +0100, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> See 2 comments below.
> 
> On 02.11.2020 10:49, imeevma@tarantool.org wrote:
> > 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
> > ---
> > https://github.com/tarantool/tarantool/issues/5427
> > https://github.com/tarantool/tarantool/tree/imeevma/gh-5427-lua-func-changes-result
> > 
> > @ChangeLog
> >  - Region truncated instead of resetting on select() (gh-5427).
> 
> 1. I suggest to make the message more understandable for users. They have
> no idea what a region is. For instance, consider
> 
> 	Memory corruption when SQL called Lua functions with box calls inside
> 
Thanks, added.

> >  src/box/box.cc                                |   5 +-
> >  src/box/index.cc                              |  20 +++
> >  src/box/txn.h                                 |   6 +-
> >  .../gh-5427-lua-func-changes-result.result    | 153 ++++++++++++++++++
> >  .../gh-5427-lua-func-changes-result.test.lua  |  86 ++++++++++
> >  5 files changed, 264 insertions(+), 6 deletions(-)
> >  create mode 100644 test/sql/gh-5427-lua-func-changes-result.result
> >  create mode 100644 test/sql/gh-5427-lua-func-changes-result.test.lua
> > 
> > diff --git a/src/box/box.cc b/src/box/box.cc
> > index 18568df3b..473ef52ad 100644
> > --- a/src/box/box.cc
> > +++ b/src/box/box.cc
> > @@ -1388,7 +1388,8 @@ box_select(uint32_t space_id, uint32_t index_id,
> >  	   struct port *port)
> >  {
> >  	(void)key_end;
> > -
> > +	struct region *region = &fiber()->gc;
> > +	size_t used = region_used(region);
> >  	rmean_collect(rmean_box, IPROTO_SELECT, 1);
> 
> 2. I must say I couldn't come up with a better idea so far. I remember I
> also proposed to start a transaction for each VDBE statement, but not sure
> it is a right thing to do.
> 
> It means, we probably should go for this one, but a bit polished.
> 
> In the current solution I don't like, that you basically just inlined
> txn_commit_ro_stmt(), but instead I would rather want you to patch it to
> make it work.
> 
> So for example txn_begin_ro_stmt inside remembers the current region size,
> and returns it via an out parameter struct txn_ro_savepoint *svp. Which
> you create on the stack.
> 
> Then, after you are done with the statement, you call txn_commit_ro_stmt,
> which takes const struct txn_ro_savepoint *svp, and does the region_truncate
> inside.
Thanks, fixed.



From adc5b1096409b941daae0b91b8b1d7b0118fca05 Mon Sep 17 00:00:00 2001
Message-Id: <adc5b1096409b941daae0b91b8b1d7b0118fca05.1608096646.git.imeevma@gmail.com>
From: Mergen Imeev <imeevma@gmail.com>
Date: Thu, 22 Oct 2020 14:21:28 +0300
Subject: [PATCH v1 1/1] 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
---
https://github.com/tarantool/tarantool/issues/5427
https://github.com/tarantool/tarantool/tree/imeevma/gh-5427-lua-func-changes-result

@ChangeLog
 - Fixed memory corruption when SQL called Lua functions with box calls inside (gh-5427).

 src/box/box.cc                                |   5 +-
 src/box/index.cc                              |  25 +--
 src/box/sql.c                                 |   5 +-
 src/box/txn.h                                 |  16 +-
 .../gh-5427-lua-func-changes-result.result    | 153 ++++++++++++++++++
 .../gh-5427-lua-func-changes-result.test.lua  |  86 ++++++++++
 6 files changed, 273 insertions(+), 17 deletions(-)
 create mode 100644 test/sql/gh-5427-lua-func-changes-result.result
 create mode 100644 test/sql/gh-5427-lua-func-changes-result.test.lua

diff --git a/src/box/box.cc b/src/box/box.cc
index a8bc3471d..7470f882c 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -1394,7 +1394,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,
@@ -1428,7 +1429,7 @@ box_select(uint32_t space_id, uint32_t index_id,
 		txn_rollback_stmt(txn);
 		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 c2fc00867..179cc190f 100644
--- a/src/box/index.cc
+++ b/src/box/index.cc
@@ -239,13 +239,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(txn);
 		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)
@@ -273,13 +274,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(txn);
 		return -1;
 	}
-	txn_commit_ro_stmt(txn);
+	txn_commit_ro_stmt(txn, &svp);
 	if (*result != NULL)
 		tuple_bless(*result);
 	return 0;
@@ -305,13 +307,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(txn);
 		return -1;
 	}
-	txn_commit_ro_stmt(txn);
+	txn_commit_ro_stmt(txn, &svp);
 	if (*result != NULL)
 		tuple_bless(*result);
 	return 0;
@@ -338,14 +341,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(txn);
 		return -1;
 	}
-	txn_commit_ro_stmt(txn);
+	txn_commit_ro_stmt(txn, &svp);
 	return count;
 }
 
@@ -374,7 +378,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);
@@ -382,7 +387,7 @@ box_index_iterator(uint32_t space_id, uint32_t index_id, int type,
 		txn_rollback_stmt(txn);
 		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/sql.c b/src/box/sql.c
index c55d0f4e5..3d968e56a 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -901,7 +901,8 @@ cursor_seek(BtCursor *pCur, int *pRes)
 
 	struct space *space = pCur->space;
 	struct txn *txn = NULL;
-	if (space->def->id != 0 && txn_begin_ro_stmt(space, &txn) != 0)
+	struct txn_ro_savepoint svp;
+	if (space->def->id != 0 && txn_begin_ro_stmt(space, &txn, &svp) != 0)
 		return -1;
 	struct iterator *it =
 		index_create_iterator(pCur->index, pCur->iter_type, key,
@@ -913,7 +914,7 @@ cursor_seek(BtCursor *pCur, int *pRes)
 		return -1;
 	}
 	if (txn != NULL)
-		txn_commit_ro_stmt(txn);
+		txn_commit_ro_stmt(txn, &svp);
 	pCur->iter = it;
 	pCur->eState = CURSOR_VALID;
 
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 {
+	/** 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);
 	}
 }
 
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"})
+ | ---
+ | - metadata:
+ |   - name: COLUMN_1
+ |     type: integer
+ |   rows:
+ |   - [1]
+ | ...
+
+box.execute([[DROP TABLE t;]])
+ | ---
+ | - row_count: 1
+ | ...
diff --git a/test/sql/gh-5427-lua-func-changes-result.test.lua b/test/sql/gh-5427-lua-func-changes-result.test.lua
new file mode 100644
index 000000000..f2699dbe7
--- /dev/null
+++ b/test/sql/gh-5427-lua-func-changes-result.test.lua
@@ -0,0 +1,86 @@
+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);]])
+box.execute([[INSERT INTO t VALUES ('aaaaaaaaaaaa');]])
+test_run:cmd("setopt delimiter ';'");
+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 ''");
+
+box.execute([[select CORRUPT_SELECT(t.b) from t where t.b = ? and t.b <= ? order by t.b;]], {"aaaaaaaaaaaa", "aaaaaaaaaaaa"})
+box.execute([[select CORRUPT_GET(t.b) from t where t.b = ? and t.b <= ? order by t.b;]], {"aaaaaaaaaaaa", "aaaaaaaaaaaa"})
+box.execute([[select CORRUPT_COUNT(t.b) from t where t.b = ? and t.b <= ? order by t.b;]], {"aaaaaaaaaaaa", "aaaaaaaaaaaa"})
+box.execute([[select CORRUPT_MAX(t.b) from t where t.b = ? and t.b <= ? order by t.b;]], {"aaaaaaaaaaaa", "aaaaaaaaaaaa"})
+box.execute([[select CORRUPT_MIN(t.b) from t where t.b = ? and t.b <= ? order by t.b;]], {"aaaaaaaaaaaa", "aaaaaaaaaaaa"})
+
+box.execute([[DROP TABLE t;]])
-- 
2.25.1

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: do not reset region on select
  2020-12-16  5:34   ` Mergen Imeev
@ 2020-12-17 22:04     ` Vladislav Shpilevoy
  2020-12-22 19:30       ` Mergen Imeev
  0 siblings, 1 reply; 10+ messages in thread
From: Vladislav Shpilevoy @ 2020-12-17 22:04 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

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
> + | ...

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: do not reset region on select
  2020-12-17 22:04     ` Vladislav Shpilevoy
@ 2020-12-22 19:30       ` Mergen Imeev
  2020-12-23 14:58         ` Vladislav Shpilevoy
  0 siblings, 1 reply; 10+ messages in thread
From: Mergen Imeev @ 2020-12-22 19:30 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Hi! Thank you for the review. My answers and new patch below.

On Thu, Dec 17, 2020 at 11:04:45PM +0100, Vladislav Shpilevoy wrote:
> 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.
> 
Added.

> > +	/** 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.
> 
I found one in function vy_point_lookup_scan_slices(). There may be more.

To find it I applied this diff:

diff --git a/src/box/txn.h b/src/box/txn.h
index fca9bc1d0..039643ab8 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -591,6 +591,7 @@ txn_commit_ro_stmt(struct txn *txn, struct txn_ro_savepoint *svp)
        if (txn) {
                /* nothing to do */
        } else {
+               assert(region_used(svp->region) == svp->region_used);
                region_truncate(svp->region, svp->region_used);
        }
 }

After I run tests, 24 of the failed. All 24 used vinyl engine. I think that
region is not used in memtx, only in vinyl during RO transactions. However, not
100% sure.

Example that I used to find this function:

_ = box.schema.space.create('test', {engine='vinyl'})
_ = box.space.test:create_index('pk', {parts={1,'integer'}})
box.space.test:insert{1, 1}
_ = box.space.test:create_index('sk', {parts={2, 'integer'}})
box.space.test.index.sk:get{}


> >  	}
> >  }
> >  
> > 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.
> 
Fixed.

> > + | ---
> > + | - metadata:
> > + |   - name: COLUMN_1
> > + |     type: integer
> > + |   rows:
> > + |   - [1]
> > + | ...
> > +
> > +box.execute([[DROP TABLE t;]])
> 
> 4. You also need to drop the functions.
> 
Fixed.

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



New patch:


From c00107eca1b17e870188f9de3f2cfebb8518d3f7 Mon Sep 17 00:00:00 2001
Message-Id: <c00107eca1b17e870188f9de3f2cfebb8518d3f7.1608665299.git.imeevma@gmail.com>
From: Mergen Imeev <imeevma@gmail.com>
Date: Thu, 22 Oct 2020 14:21:28 +0300
Subject: [PATCH v1 1/1] 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
---
https://github.com/tarantool/tarantool/issues/5427
https://github.com/tarantool/tarantool/tree/imeevma/gh-5427-lua-func-changes-result

@ChangeLog
 - Fixed memory corruption when SQL called Lua functions with box calls inside (gh-5427).

 src/box/box.cc                                |   5 +-
 src/box/index.cc                              |  25 ++-
 src/box/sql.c                                 |   5 +-
 src/box/txn.h                                 |  23 ++-
 .../gh-5427-lua-func-changes-result.result    | 174 ++++++++++++++++++
 .../gh-5427-lua-func-changes-result.test.lua  |  93 ++++++++++
 6 files changed, 308 insertions(+), 17 deletions(-)
 create mode 100644 test/sql/gh-5427-lua-func-changes-result.result
 create mode 100644 test/sql/gh-5427-lua-func-changes-result.test.lua

diff --git a/src/box/box.cc b/src/box/box.cc
index 306db7c37..7f23487ed 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -1413,7 +1413,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,
@@ -1447,7 +1448,7 @@ box_select(uint32_t space_id, uint32_t index_id,
 		txn_rollback_stmt(txn);
 		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 c2fc00867..179cc190f 100644
--- a/src/box/index.cc
+++ b/src/box/index.cc
@@ -239,13 +239,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(txn);
 		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)
@@ -273,13 +274,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(txn);
 		return -1;
 	}
-	txn_commit_ro_stmt(txn);
+	txn_commit_ro_stmt(txn, &svp);
 	if (*result != NULL)
 		tuple_bless(*result);
 	return 0;
@@ -305,13 +307,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(txn);
 		return -1;
 	}
-	txn_commit_ro_stmt(txn);
+	txn_commit_ro_stmt(txn, &svp);
 	if (*result != NULL)
 		tuple_bless(*result);
 	return 0;
@@ -338,14 +341,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(txn);
 		return -1;
 	}
-	txn_commit_ro_stmt(txn);
+	txn_commit_ro_stmt(txn, &svp);
 	return count;
 }
 
@@ -374,7 +378,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);
@@ -382,7 +387,7 @@ box_index_iterator(uint32_t space_id, uint32_t index_id, int type,
 		txn_rollback_stmt(txn);
 		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/sql.c b/src/box/sql.c
index c55d0f4e5..3d968e56a 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -901,7 +901,8 @@ cursor_seek(BtCursor *pCur, int *pRes)
 
 	struct space *space = pCur->space;
 	struct txn *txn = NULL;
-	if (space->def->id != 0 && txn_begin_ro_stmt(space, &txn) != 0)
+	struct txn_ro_savepoint svp;
+	if (space->def->id != 0 && txn_begin_ro_stmt(space, &txn, &svp) != 0)
 		return -1;
 	struct iterator *it =
 		index_create_iterator(pCur->index, pCur->iter_type, key,
@@ -913,7 +914,7 @@ cursor_seek(BtCursor *pCur, int *pRes)
 		return -1;
 	}
 	if (txn != NULL)
-		txn_commit_ro_stmt(txn);
+		txn_commit_ro_stmt(txn, &svp);
 	pCur->iter = it;
 	pCur->eState = CURSOR_VALID;
 
diff --git a/src/box/txn.h b/src/box/txn.h
index dfa7c0ee4..fca9bc1d0 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -258,6 +258,20 @@ struct txn_savepoint {
 	char name[1];
 };
 
+/**
+ * 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;
 
 /**
@@ -557,24 +571,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);
 	}
 }
 
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..25a958bc2
--- /dev/null
+++ b/test/sql/gh-5427-lua-func-changes-result.result
@@ -0,0 +1,174 @@
+-- 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
+ | ...
+
+values = {"aaaaaaaaaaaa", "aaaaaaaaaaaa"}
+ | ---
+ | ...
+query = [[select %s(t.b) from t where t.b = ? and t.b <= ? order by t.b;]]
+ | ---
+ | ...
+box.execute(string.format(query, 'CORRUPT_SELECT'), values)
+ | ---
+ | - metadata:
+ |   - name: COLUMN_1
+ |     type: integer
+ |   rows:
+ |   - [1]
+ | ...
+box.execute(string.format(query, 'CORRUPT_GET'), values)
+ | ---
+ | - metadata:
+ |   - name: COLUMN_1
+ |     type: integer
+ |   rows:
+ |   - [1]
+ | ...
+box.execute(string.format(query, 'CORRUPT_COUNT'), values)
+ | ---
+ | - metadata:
+ |   - name: COLUMN_1
+ |     type: integer
+ |   rows:
+ |   - [1]
+ | ...
+box.execute(string.format(query, 'CORRUPT_MAX'), values)
+ | ---
+ | - metadata:
+ |   - name: COLUMN_1
+ |     type: integer
+ |   rows:
+ |   - [1]
+ | ...
+box.execute(string.format(query, 'CORRUPT_MIN'), values)
+ | ---
+ | - metadata:
+ |   - name: COLUMN_1
+ |     type: integer
+ |   rows:
+ |   - [1]
+ | ...
+box.execute([[DROP TABLE t;]])
+ | ---
+ | - row_count: 1
+ | ...
+
+_ = box.space._func.index['name']:delete('CORRUPT_SELECT')
+ | ---
+ | ...
+_ = box.space._func.index['name']:delete('CORRUPT_GET')
+ | ---
+ | ...
+_ = box.space._func.index['name']:delete('CORRUPT_COUNT')
+ | ---
+ | ...
+_ = box.space._func.index['name']:delete('CORRUPT_MAX')
+ | ---
+ | ...
+_ = box.space._func.index['name']:delete('CORRUPT_MIN')
+ | ---
+ | ...
diff --git a/test/sql/gh-5427-lua-func-changes-result.test.lua b/test/sql/gh-5427-lua-func-changes-result.test.lua
new file mode 100644
index 000000000..98959de0b
--- /dev/null
+++ b/test/sql/gh-5427-lua-func-changes-result.test.lua
@@ -0,0 +1,93 @@
+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);]])
+box.execute([[INSERT INTO t VALUES ('aaaaaaaaaaaa');]])
+test_run:cmd("setopt delimiter ';'");
+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 ''");
+
+values = {"aaaaaaaaaaaa", "aaaaaaaaaaaa"}
+query = [[select %s(t.b) from t where t.b = ? and t.b <= ? order by t.b;]]
+box.execute(string.format(query, 'CORRUPT_SELECT'), values)
+box.execute(string.format(query, 'CORRUPT_GET'), values)
+box.execute(string.format(query, 'CORRUPT_COUNT'), values)
+box.execute(string.format(query, 'CORRUPT_MAX'), values)
+box.execute(string.format(query, 'CORRUPT_MIN'), values)
+box.execute([[DROP TABLE t;]])
+
+_ = box.space._func.index['name']:delete('CORRUPT_SELECT')
+_ = box.space._func.index['name']:delete('CORRUPT_GET')
+_ = box.space._func.index['name']:delete('CORRUPT_COUNT')
+_ = box.space._func.index['name']:delete('CORRUPT_MAX')
+_ = box.space._func.index['name']:delete('CORRUPT_MIN')
-- 
2.25.1

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: do not reset region on select
  2020-12-22 19:30       ` Mergen Imeev
@ 2020-12-23 14:58         ` Vladislav Shpilevoy
  2020-12-23 19:20           ` Mergen Imeev
  0 siblings, 1 reply; 10+ messages in thread
From: Vladislav Shpilevoy @ 2020-12-23 14:58 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches

Hi! Thanks for the fixes!

Consider below some review fixes, which I also pushed on top
of the branch in a separate commit. Please, review, and if
you agree, then squash and proceed to a second review with LGTM
from me.

Talking of your findings about region being used only by vinyl,
I think it is worth dropping these RO statements entirely then.
But better do it in scope of
https://github.com/tarantool/tarantool/issues/5501, if it will
be even possible.

====================
diff --git a/test/sql/gh-5427-lua-func-changes-result.result b/test/sql/gh-5427-lua-func-changes-result.result
index 25a958bc2..7175ea78a 100644
--- a/test/sql/gh-5427-lua-func-changes-result.result
+++ b/test/sql/gh-5427-lua-func-changes-result.result
@@ -22,82 +22,57 @@ test_run:cmd("setopt delimiter ';'");
  | - true
  | ...
 box.schema.func.create('CORRUPT_SELECT', {
-    language = 'LUA',
     returns = 'integer',
     body = [[
-        function(x)
+        function()
             box.space.T:select()
             return 1
         end]],
-    is_sandboxed = false,
-    param_list = { "string" },
-    exports = { 'LUA', 'SQL' },
-    is_deterministic = false,
-    if_not_exists = true
+    exports = {'LUA', 'SQL'},
 });
  | ---
  | ...
 box.schema.func.create('CORRUPT_GET', {
-    language = 'LUA',
     returns = 'integer',
     body = [[
-        function(x)
+        function()
             box.space.T:get('aaaaaaaaaaaa')
             return 1
         end]],
-    is_sandboxed = false,
-    param_list = { "string" },
-    exports = { 'LUA', 'SQL' },
-    is_deterministic = false,
-    if_not_exists = true
+    exports = {'LUA', 'SQL'},
 });
  | ---
  | ...
 box.schema.func.create('CORRUPT_COUNT', {
-    language = 'LUA',
     returns = 'integer',
     body = [[
-        function(x)
+        function()
             box.space.T:count()
             return 1
         end]],
-    is_sandboxed = false,
-    param_list = { "string" },
-    exports = { 'LUA', 'SQL' },
-    is_deterministic = false,
-    if_not_exists = true
+    exports = {'LUA', 'SQL'},
 });
  | ---
  | ...
 box.schema.func.create('CORRUPT_MAX', {
-    language = 'LUA',
     returns = 'integer',
     body = [[
-        function(x)
+        function()
             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
+    exports = {'LUA', 'SQL'},
 });
  | ---
  | ...
 box.schema.func.create('CORRUPT_MIN', {
-    language = 'LUA',
     returns = 'integer',
     body = [[
-        function(x)
+        function()
             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
+    exports = {'LUA', 'SQL'},
 });
  | ---
  | ...
@@ -109,7 +84,7 @@ test_run:cmd("setopt delimiter ''");
 values = {"aaaaaaaaaaaa", "aaaaaaaaaaaa"}
  | ---
  | ...
-query = [[select %s(t.b) from t where t.b = ? and t.b <= ? order by t.b;]]
+query = [[select %s() from t where t.b = ? and t.b <= ? order by t.b;]]
  | ---
  | ...
 box.execute(string.format(query, 'CORRUPT_SELECT'), values)
@@ -157,18 +132,18 @@ box.execute([[DROP TABLE t;]])
  | - row_count: 1
  | ...
 
-_ = box.space._func.index['name']:delete('CORRUPT_SELECT')
+box.func.CORRUPT_SELECT:drop()
  | ---
  | ...
-_ = box.space._func.index['name']:delete('CORRUPT_GET')
+box.func.CORRUPT_GET:drop()
  | ---
  | ...
-_ = box.space._func.index['name']:delete('CORRUPT_COUNT')
+box.func.CORRUPT_COUNT:drop()
  | ---
  | ...
-_ = box.space._func.index['name']:delete('CORRUPT_MAX')
+box.func.CORRUPT_MAX:drop()
  | ---
  | ...
-_ = box.space._func.index['name']:delete('CORRUPT_MIN')
+box.func.CORRUPT_MIN:drop()
  | ---
  | ...
diff --git a/test/sql/gh-5427-lua-func-changes-result.test.lua b/test/sql/gh-5427-lua-func-changes-result.test.lua
index 98959de0b..edbbc9ee4 100644
--- a/test/sql/gh-5427-lua-func-changes-result.test.lua
+++ b/test/sql/gh-5427-lua-func-changes-result.test.lua
@@ -6,79 +6,54 @@ box.execute([[CREATE TABLE t (b STRING PRIMARY KEY);]])
 box.execute([[INSERT INTO t VALUES ('aaaaaaaaaaaa');]])
 test_run:cmd("setopt delimiter ';'");
 box.schema.func.create('CORRUPT_SELECT', {
-    language = 'LUA',
     returns = 'integer',
     body = [[
-        function(x)
+        function()
             box.space.T:select()
             return 1
         end]],
-    is_sandboxed = false,
-    param_list = { "string" },
-    exports = { 'LUA', 'SQL' },
-    is_deterministic = false,
-    if_not_exists = true
+    exports = {'LUA', 'SQL'},
 });
 box.schema.func.create('CORRUPT_GET', {
-    language = 'LUA',
     returns = 'integer',
     body = [[
-        function(x)
+        function()
             box.space.T:get('aaaaaaaaaaaa')
             return 1
         end]],
-    is_sandboxed = false,
-    param_list = { "string" },
-    exports = { 'LUA', 'SQL' },
-    is_deterministic = false,
-    if_not_exists = true
+    exports = {'LUA', 'SQL'},
 });
 box.schema.func.create('CORRUPT_COUNT', {
-    language = 'LUA',
     returns = 'integer',
     body = [[
-        function(x)
+        function()
             box.space.T:count()
             return 1
         end]],
-    is_sandboxed = false,
-    param_list = { "string" },
-    exports = { 'LUA', 'SQL' },
-    is_deterministic = false,
-    if_not_exists = true
+    exports = {'LUA', 'SQL'},
 });
 box.schema.func.create('CORRUPT_MAX', {
-    language = 'LUA',
     returns = 'integer',
     body = [[
-        function(x)
+        function()
             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
+    exports = {'LUA', 'SQL'},
 });
 box.schema.func.create('CORRUPT_MIN', {
-    language = 'LUA',
     returns = 'integer',
     body = [[
-        function(x)
+        function()
             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
+    exports = {'LUA', 'SQL'},
 });
 test_run:cmd("setopt delimiter ''");
 
 values = {"aaaaaaaaaaaa", "aaaaaaaaaaaa"}
-query = [[select %s(t.b) from t where t.b = ? and t.b <= ? order by t.b;]]
+query = [[select %s() from t where t.b = ? and t.b <= ? order by t.b;]]
 box.execute(string.format(query, 'CORRUPT_SELECT'), values)
 box.execute(string.format(query, 'CORRUPT_GET'), values)
 box.execute(string.format(query, 'CORRUPT_COUNT'), values)
@@ -86,8 +61,8 @@ box.execute(string.format(query, 'CORRUPT_MAX'), values)
 box.execute(string.format(query, 'CORRUPT_MIN'), values)
 box.execute([[DROP TABLE t;]])
 
-_ = box.space._func.index['name']:delete('CORRUPT_SELECT')
-_ = box.space._func.index['name']:delete('CORRUPT_GET')
-_ = box.space._func.index['name']:delete('CORRUPT_COUNT')
-_ = box.space._func.index['name']:delete('CORRUPT_MAX')
-_ = box.space._func.index['name']:delete('CORRUPT_MIN')
+box.func.CORRUPT_SELECT:drop()
+box.func.CORRUPT_GET:drop()
+box.func.CORRUPT_COUNT:drop()
+box.func.CORRUPT_MAX:drop()
+box.func.CORRUPT_MIN:drop()

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: do not reset region on select
  2020-12-23 14:58         ` Vladislav Shpilevoy
@ 2020-12-23 19:20           ` Mergen Imeev
  0 siblings, 0 replies; 10+ messages in thread
From: Mergen Imeev @ 2020-12-23 19:20 UTC (permalink / raw)
  To: Vladislav Shpilevoy; +Cc: tarantool-patches

Hi! Thank you for the review and patch! I squashed your patch and sent
this patch to Nikita.

I will think about dropping RO statements. If the issue will be given to
me I will send a patch, othewise I will add a comment to the issue with
fix, if I find one.


On Wed, Dec 23, 2020 at 03:58:13PM +0100, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
> 
> Consider below some review fixes, which I also pushed on top
> of the branch in a separate commit. Please, review, and if
> you agree, then squash and proceed to a second review with LGTM
> from me.
> 
> Talking of your findings about region being used only by vinyl,
> I think it is worth dropping these RO statements entirely then.
> But better do it in scope of
> https://github.com/tarantool/tarantool/issues/5501, if it will
> be even possible.
> 
> ====================
> diff --git a/test/sql/gh-5427-lua-func-changes-result.result b/test/sql/gh-5427-lua-func-changes-result.result
> index 25a958bc2..7175ea78a 100644
> --- a/test/sql/gh-5427-lua-func-changes-result.result
> +++ b/test/sql/gh-5427-lua-func-changes-result.result
> @@ -22,82 +22,57 @@ test_run:cmd("setopt delimiter ';'");
>   | - true
>   | ...
>  box.schema.func.create('CORRUPT_SELECT', {
> -    language = 'LUA',
>      returns = 'integer',
>      body = [[
> -        function(x)
> +        function()
>              box.space.T:select()
>              return 1
>          end]],
> -    is_sandboxed = false,
> -    param_list = { "string" },
> -    exports = { 'LUA', 'SQL' },
> -    is_deterministic = false,
> -    if_not_exists = true
> +    exports = {'LUA', 'SQL'},
>  });
>   | ---
>   | ...
>  box.schema.func.create('CORRUPT_GET', {
> -    language = 'LUA',
>      returns = 'integer',
>      body = [[
> -        function(x)
> +        function()
>              box.space.T:get('aaaaaaaaaaaa')
>              return 1
>          end]],
> -    is_sandboxed = false,
> -    param_list = { "string" },
> -    exports = { 'LUA', 'SQL' },
> -    is_deterministic = false,
> -    if_not_exists = true
> +    exports = {'LUA', 'SQL'},
>  });
>   | ---
>   | ...
>  box.schema.func.create('CORRUPT_COUNT', {
> -    language = 'LUA',
>      returns = 'integer',
>      body = [[
> -        function(x)
> +        function()
>              box.space.T:count()
>              return 1
>          end]],
> -    is_sandboxed = false,
> -    param_list = { "string" },
> -    exports = { 'LUA', 'SQL' },
> -    is_deterministic = false,
> -    if_not_exists = true
> +    exports = {'LUA', 'SQL'},
>  });
>   | ---
>   | ...
>  box.schema.func.create('CORRUPT_MAX', {
> -    language = 'LUA',
>      returns = 'integer',
>      body = [[
> -        function(x)
> +        function()
>              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
> +    exports = {'LUA', 'SQL'},
>  });
>   | ---
>   | ...
>  box.schema.func.create('CORRUPT_MIN', {
> -    language = 'LUA',
>      returns = 'integer',
>      body = [[
> -        function(x)
> +        function()
>              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
> +    exports = {'LUA', 'SQL'},
>  });
>   | ---
>   | ...
> @@ -109,7 +84,7 @@ test_run:cmd("setopt delimiter ''");
>  values = {"aaaaaaaaaaaa", "aaaaaaaaaaaa"}
>   | ---
>   | ...
> -query = [[select %s(t.b) from t where t.b = ? and t.b <= ? order by t.b;]]
> +query = [[select %s() from t where t.b = ? and t.b <= ? order by t.b;]]
>   | ---
>   | ...
>  box.execute(string.format(query, 'CORRUPT_SELECT'), values)
> @@ -157,18 +132,18 @@ box.execute([[DROP TABLE t;]])
>   | - row_count: 1
>   | ...
>  
> -_ = box.space._func.index['name']:delete('CORRUPT_SELECT')
> +box.func.CORRUPT_SELECT:drop()
>   | ---
>   | ...
> -_ = box.space._func.index['name']:delete('CORRUPT_GET')
> +box.func.CORRUPT_GET:drop()
>   | ---
>   | ...
> -_ = box.space._func.index['name']:delete('CORRUPT_COUNT')
> +box.func.CORRUPT_COUNT:drop()
>   | ---
>   | ...
> -_ = box.space._func.index['name']:delete('CORRUPT_MAX')
> +box.func.CORRUPT_MAX:drop()
>   | ---
>   | ...
> -_ = box.space._func.index['name']:delete('CORRUPT_MIN')
> +box.func.CORRUPT_MIN:drop()
>   | ---
>   | ...
> diff --git a/test/sql/gh-5427-lua-func-changes-result.test.lua b/test/sql/gh-5427-lua-func-changes-result.test.lua
> index 98959de0b..edbbc9ee4 100644
> --- a/test/sql/gh-5427-lua-func-changes-result.test.lua
> +++ b/test/sql/gh-5427-lua-func-changes-result.test.lua
> @@ -6,79 +6,54 @@ box.execute([[CREATE TABLE t (b STRING PRIMARY KEY);]])
>  box.execute([[INSERT INTO t VALUES ('aaaaaaaaaaaa');]])
>  test_run:cmd("setopt delimiter ';'");
>  box.schema.func.create('CORRUPT_SELECT', {
> -    language = 'LUA',
>      returns = 'integer',
>      body = [[
> -        function(x)
> +        function()
>              box.space.T:select()
>              return 1
>          end]],
> -    is_sandboxed = false,
> -    param_list = { "string" },
> -    exports = { 'LUA', 'SQL' },
> -    is_deterministic = false,
> -    if_not_exists = true
> +    exports = {'LUA', 'SQL'},
>  });
>  box.schema.func.create('CORRUPT_GET', {
> -    language = 'LUA',
>      returns = 'integer',
>      body = [[
> -        function(x)
> +        function()
>              box.space.T:get('aaaaaaaaaaaa')
>              return 1
>          end]],
> -    is_sandboxed = false,
> -    param_list = { "string" },
> -    exports = { 'LUA', 'SQL' },
> -    is_deterministic = false,
> -    if_not_exists = true
> +    exports = {'LUA', 'SQL'},
>  });
>  box.schema.func.create('CORRUPT_COUNT', {
> -    language = 'LUA',
>      returns = 'integer',
>      body = [[
> -        function(x)
> +        function()
>              box.space.T:count()
>              return 1
>          end]],
> -    is_sandboxed = false,
> -    param_list = { "string" },
> -    exports = { 'LUA', 'SQL' },
> -    is_deterministic = false,
> -    if_not_exists = true
> +    exports = {'LUA', 'SQL'},
>  });
>  box.schema.func.create('CORRUPT_MAX', {
> -    language = 'LUA',
>      returns = 'integer',
>      body = [[
> -        function(x)
> +        function()
>              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
> +    exports = {'LUA', 'SQL'},
>  });
>  box.schema.func.create('CORRUPT_MIN', {
> -    language = 'LUA',
>      returns = 'integer',
>      body = [[
> -        function(x)
> +        function()
>              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
> +    exports = {'LUA', 'SQL'},
>  });
>  test_run:cmd("setopt delimiter ''");
>  
>  values = {"aaaaaaaaaaaa", "aaaaaaaaaaaa"}
> -query = [[select %s(t.b) from t where t.b = ? and t.b <= ? order by t.b;]]
> +query = [[select %s() from t where t.b = ? and t.b <= ? order by t.b;]]
>  box.execute(string.format(query, 'CORRUPT_SELECT'), values)
>  box.execute(string.format(query, 'CORRUPT_GET'), values)
>  box.execute(string.format(query, 'CORRUPT_COUNT'), values)
> @@ -86,8 +61,8 @@ box.execute(string.format(query, 'CORRUPT_MAX'), values)
>  box.execute(string.format(query, 'CORRUPT_MIN'), values)
>  box.execute([[DROP TABLE t;]])
>  
> -_ = box.space._func.index['name']:delete('CORRUPT_SELECT')
> -_ = box.space._func.index['name']:delete('CORRUPT_GET')
> -_ = box.space._func.index['name']:delete('CORRUPT_COUNT')
> -_ = box.space._func.index['name']:delete('CORRUPT_MAX')
> -_ = box.space._func.index['name']:delete('CORRUPT_MIN')
> +box.func.CORRUPT_SELECT:drop()
> +box.func.CORRUPT_GET:drop()
> +box.func.CORRUPT_COUNT:drop()
> +box.func.CORRUPT_MAX:drop()
> +box.func.CORRUPT_MIN:drop()

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: do not reset region on select
  2020-12-24 10:48 ` Nikita Pettik
@ 2020-12-24 10:58   ` Nikita Pettik
  0 siblings, 0 replies; 10+ messages in thread
From: Nikita Pettik @ 2020-12-24 10:58 UTC (permalink / raw)
  To: imeevma, tarantool-patches

On 24 Dec 10:48, Nikita Pettik via Tarantool-patches wrote:
> On 23 Dec 22:15, imeevma@tarantool.org wrote:
> > 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
> > ---
> > https://github.com/tarantool/tarantool/issues/5427
> > https://github.com/tarantool/tarantool/tree/imeevma/gh-5427-lua-func-changes-result
> > 
> > @ChangeLog
> >  - Fixed memory corruption when SQL called Lua functions with box calls inside (gh-5427).
> 
> Nice fix, LGTM.
>

Pushed to master, 2.6 and 2.5; updated chagelogs correspondingly; branch
is dropped. Thx.
  

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Tarantool-patches] [PATCH v1 1/1] sql: do not reset region on select
  2020-12-23 19:15 imeevma
@ 2020-12-24 10:48 ` Nikita Pettik
  2020-12-24 10:58   ` Nikita Pettik
  0 siblings, 1 reply; 10+ messages in thread
From: Nikita Pettik @ 2020-12-24 10:48 UTC (permalink / raw)
  To: imeevma; +Cc: tarantool-patches

On 23 Dec 22:15, imeevma@tarantool.org wrote:
> 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
> ---
> https://github.com/tarantool/tarantool/issues/5427
> https://github.com/tarantool/tarantool/tree/imeevma/gh-5427-lua-func-changes-result
> 
> @ChangeLog
>  - Fixed memory corruption when SQL called Lua functions with box calls inside (gh-5427).

Nice fix, LGTM.
 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Tarantool-patches] [PATCH v1 1/1] sql: do not reset region on select
@ 2020-12-23 19:15 imeevma
  2020-12-24 10:48 ` Nikita Pettik
  0 siblings, 1 reply; 10+ messages in thread
From: imeevma @ 2020-12-23 19:15 UTC (permalink / raw)
  To: korablev; +Cc: tarantool-patches

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
---
https://github.com/tarantool/tarantool/issues/5427
https://github.com/tarantool/tarantool/tree/imeevma/gh-5427-lua-func-changes-result

@ChangeLog
 - Fixed memory corruption when SQL called Lua functions with box calls inside (gh-5427).

 src/box/box.cc                                |   5 +-
 src/box/index.cc                              |  25 +--
 src/box/sql.c                                 |   5 +-
 src/box/txn.h                                 |  23 ++-
 .../gh-5427-lua-func-changes-result.result    | 149 ++++++++++++++++++
 .../gh-5427-lua-func-changes-result.test.lua  |  68 ++++++++
 6 files changed, 258 insertions(+), 17 deletions(-)
 create mode 100644 test/sql/gh-5427-lua-func-changes-result.result
 create mode 100644 test/sql/gh-5427-lua-func-changes-result.test.lua

diff --git a/src/box/box.cc b/src/box/box.cc
index 306db7c37..7f23487ed 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -1413,7 +1413,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,
@@ -1447,7 +1448,7 @@ box_select(uint32_t space_id, uint32_t index_id,
 		txn_rollback_stmt(txn);
 		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 c2fc00867..179cc190f 100644
--- a/src/box/index.cc
+++ b/src/box/index.cc
@@ -239,13 +239,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(txn);
 		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)
@@ -273,13 +274,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(txn);
 		return -1;
 	}
-	txn_commit_ro_stmt(txn);
+	txn_commit_ro_stmt(txn, &svp);
 	if (*result != NULL)
 		tuple_bless(*result);
 	return 0;
@@ -305,13 +307,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(txn);
 		return -1;
 	}
-	txn_commit_ro_stmt(txn);
+	txn_commit_ro_stmt(txn, &svp);
 	if (*result != NULL)
 		tuple_bless(*result);
 	return 0;
@@ -338,14 +341,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(txn);
 		return -1;
 	}
-	txn_commit_ro_stmt(txn);
+	txn_commit_ro_stmt(txn, &svp);
 	return count;
 }
 
@@ -374,7 +378,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);
@@ -382,7 +387,7 @@ box_index_iterator(uint32_t space_id, uint32_t index_id, int type,
 		txn_rollback_stmt(txn);
 		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/sql.c b/src/box/sql.c
index c55d0f4e5..3d968e56a 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -901,7 +901,8 @@ cursor_seek(BtCursor *pCur, int *pRes)
 
 	struct space *space = pCur->space;
 	struct txn *txn = NULL;
-	if (space->def->id != 0 && txn_begin_ro_stmt(space, &txn) != 0)
+	struct txn_ro_savepoint svp;
+	if (space->def->id != 0 && txn_begin_ro_stmt(space, &txn, &svp) != 0)
 		return -1;
 	struct iterator *it =
 		index_create_iterator(pCur->index, pCur->iter_type, key,
@@ -913,7 +914,7 @@ cursor_seek(BtCursor *pCur, int *pRes)
 		return -1;
 	}
 	if (txn != NULL)
-		txn_commit_ro_stmt(txn);
+		txn_commit_ro_stmt(txn, &svp);
 	pCur->iter = it;
 	pCur->eState = CURSOR_VALID;
 
diff --git a/src/box/txn.h b/src/box/txn.h
index dfa7c0ee4..fca9bc1d0 100644
--- a/src/box/txn.h
+++ b/src/box/txn.h
@@ -258,6 +258,20 @@ struct txn_savepoint {
 	char name[1];
 };
 
+/**
+ * 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;
 
 /**
@@ -557,24 +571,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);
 	}
 }
 
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..7175ea78a
--- /dev/null
+++ b/test/sql/gh-5427-lua-func-changes-result.result
@@ -0,0 +1,149 @@
+-- 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', {
+    returns = 'integer',
+    body = [[
+        function()
+            box.space.T:select()
+            return 1
+        end]],
+    exports = {'LUA', 'SQL'},
+});
+ | ---
+ | ...
+box.schema.func.create('CORRUPT_GET', {
+    returns = 'integer',
+    body = [[
+        function()
+            box.space.T:get('aaaaaaaaaaaa')
+            return 1
+        end]],
+    exports = {'LUA', 'SQL'},
+});
+ | ---
+ | ...
+box.schema.func.create('CORRUPT_COUNT', {
+    returns = 'integer',
+    body = [[
+        function()
+            box.space.T:count()
+            return 1
+        end]],
+    exports = {'LUA', 'SQL'},
+});
+ | ---
+ | ...
+box.schema.func.create('CORRUPT_MAX', {
+    returns = 'integer',
+    body = [[
+        function()
+            box.space.T.index[0]:max()
+            return 1
+        end]],
+    exports = {'LUA', 'SQL'},
+});
+ | ---
+ | ...
+box.schema.func.create('CORRUPT_MIN', {
+    returns = 'integer',
+    body = [[
+        function()
+            box.space.T.index[0]:min()
+            return 1
+        end]],
+    exports = {'LUA', 'SQL'},
+});
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
+ | ...
+
+values = {"aaaaaaaaaaaa", "aaaaaaaaaaaa"}
+ | ---
+ | ...
+query = [[select %s() from t where t.b = ? and t.b <= ? order by t.b;]]
+ | ---
+ | ...
+box.execute(string.format(query, 'CORRUPT_SELECT'), values)
+ | ---
+ | - metadata:
+ |   - name: COLUMN_1
+ |     type: integer
+ |   rows:
+ |   - [1]
+ | ...
+box.execute(string.format(query, 'CORRUPT_GET'), values)
+ | ---
+ | - metadata:
+ |   - name: COLUMN_1
+ |     type: integer
+ |   rows:
+ |   - [1]
+ | ...
+box.execute(string.format(query, 'CORRUPT_COUNT'), values)
+ | ---
+ | - metadata:
+ |   - name: COLUMN_1
+ |     type: integer
+ |   rows:
+ |   - [1]
+ | ...
+box.execute(string.format(query, 'CORRUPT_MAX'), values)
+ | ---
+ | - metadata:
+ |   - name: COLUMN_1
+ |     type: integer
+ |   rows:
+ |   - [1]
+ | ...
+box.execute(string.format(query, 'CORRUPT_MIN'), values)
+ | ---
+ | - metadata:
+ |   - name: COLUMN_1
+ |     type: integer
+ |   rows:
+ |   - [1]
+ | ...
+box.execute([[DROP TABLE t;]])
+ | ---
+ | - row_count: 1
+ | ...
+
+box.func.CORRUPT_SELECT:drop()
+ | ---
+ | ...
+box.func.CORRUPT_GET:drop()
+ | ---
+ | ...
+box.func.CORRUPT_COUNT:drop()
+ | ---
+ | ...
+box.func.CORRUPT_MAX:drop()
+ | ---
+ | ...
+box.func.CORRUPT_MIN:drop()
+ | ---
+ | ...
diff --git a/test/sql/gh-5427-lua-func-changes-result.test.lua b/test/sql/gh-5427-lua-func-changes-result.test.lua
new file mode 100644
index 000000000..edbbc9ee4
--- /dev/null
+++ b/test/sql/gh-5427-lua-func-changes-result.test.lua
@@ -0,0 +1,68 @@
+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);]])
+box.execute([[INSERT INTO t VALUES ('aaaaaaaaaaaa');]])
+test_run:cmd("setopt delimiter ';'");
+box.schema.func.create('CORRUPT_SELECT', {
+    returns = 'integer',
+    body = [[
+        function()
+            box.space.T:select()
+            return 1
+        end]],
+    exports = {'LUA', 'SQL'},
+});
+box.schema.func.create('CORRUPT_GET', {
+    returns = 'integer',
+    body = [[
+        function()
+            box.space.T:get('aaaaaaaaaaaa')
+            return 1
+        end]],
+    exports = {'LUA', 'SQL'},
+});
+box.schema.func.create('CORRUPT_COUNT', {
+    returns = 'integer',
+    body = [[
+        function()
+            box.space.T:count()
+            return 1
+        end]],
+    exports = {'LUA', 'SQL'},
+});
+box.schema.func.create('CORRUPT_MAX', {
+    returns = 'integer',
+    body = [[
+        function()
+            box.space.T.index[0]:max()
+            return 1
+        end]],
+    exports = {'LUA', 'SQL'},
+});
+box.schema.func.create('CORRUPT_MIN', {
+    returns = 'integer',
+    body = [[
+        function()
+            box.space.T.index[0]:min()
+            return 1
+        end]],
+    exports = {'LUA', 'SQL'},
+});
+test_run:cmd("setopt delimiter ''");
+
+values = {"aaaaaaaaaaaa", "aaaaaaaaaaaa"}
+query = [[select %s() from t where t.b = ? and t.b <= ? order by t.b;]]
+box.execute(string.format(query, 'CORRUPT_SELECT'), values)
+box.execute(string.format(query, 'CORRUPT_GET'), values)
+box.execute(string.format(query, 'CORRUPT_COUNT'), values)
+box.execute(string.format(query, 'CORRUPT_MAX'), values)
+box.execute(string.format(query, 'CORRUPT_MIN'), values)
+box.execute([[DROP TABLE t;]])
+
+box.func.CORRUPT_SELECT:drop()
+box.func.CORRUPT_GET:drop()
+box.func.CORRUPT_COUNT:drop()
+box.func.CORRUPT_MAX:drop()
+box.func.CORRUPT_MIN:drop()
-- 
2.25.1

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-12-24 10:58 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-02  9:49 [Tarantool-patches] [PATCH v1 1/1] sql: do not reset region on select imeevma
2020-11-04 22:14 ` Vladislav Shpilevoy
2020-12-16  5:34   ` Mergen Imeev
2020-12-17 22:04     ` Vladislav Shpilevoy
2020-12-22 19:30       ` Mergen Imeev
2020-12-23 14:58         ` Vladislav Shpilevoy
2020-12-23 19:20           ` Mergen Imeev
2020-12-23 19:15 imeevma
2020-12-24 10:48 ` Nikita Pettik
2020-12-24 10:58   ` Nikita Pettik

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox