From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 1AAEB45C304 for ; Fri, 18 Dec 2020 01:04:47 +0300 (MSK) References: <46f1c499-b84b-7c4a-d8a1-189d3c5965d9@tarantool.org> <20201216053456.GA526090@tarantool.org> From: Vladislav Shpilevoy Message-ID: <60e8b082-4678-4e02-13e0-22ad74cc1c91@tarantool.org> Date: Thu, 17 Dec 2020 23:04:45 +0100 MIME-Version: 1.0 In-Reply-To: <20201216053456.GA526090@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH v1 1/1] sql: do not reset region on select List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Mergen Imeev Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the fixes! See 4 comments below. > diff --git a/src/box/txn.h b/src/box/txn.h > index dfa7c0ee4..1a5ec975e 100644 > --- a/src/box/txn.h > +++ b/src/box/txn.h > @@ -258,6 +258,13 @@ struct txn_savepoint { > char name[1]; > }; > > +struct txn_ro_savepoint { 1. Please, add a comment to this structure why does it exist. So far there was no a single comment anywhere why do we need RO statements/savepoints. Worth changing it while we are here. > + /** Region used during this transaction. */ > + struct region *region; > + /** Savepoint for region. */ > + size_t region_used; > +}; > + > extern double too_long_threshold; > > /** > @@ -557,24 +564,27 @@ txn_begin_in_engine(struct engine *engine, struct txn *txn); > * select. > */ > static inline int > -txn_begin_ro_stmt(struct space *space, struct txn **txn) > +txn_begin_ro_stmt(struct space *space, struct txn **txn, > + struct txn_ro_savepoint *svp) > { > *txn = in_txn(); > if (*txn != NULL) { > struct engine *engine = space->engine; > return txn_begin_in_engine(engine, *txn); > } > + svp->region = &fiber()->gc; > + svp->region_used = region_used(svp->region); > return 0; > } > > static inline void > -txn_commit_ro_stmt(struct txn *txn) > +txn_commit_ro_stmt(struct txn *txn, struct txn_ro_savepoint *svp) > { > assert(txn == in_txn()); > if (txn) { > /* nothing to do */ > } else { > - fiber_gc(); > + region_truncate(svp->region, svp->region_used); 2. What exactly do the ro requests allocate on the fiber region? I couldn't find any examples in memtx, but I didn't look too deep. > } > } > > diff --git a/test/sql/gh-5427-lua-func-changes-result.result b/test/sql/gh-5427-lua-func-changes-result.result > new file mode 100644 > index 000000000..f2e777a0e > --- /dev/null > +++ b/test/sql/gh-5427-lua-func-changes-result.result > @@ -0,0 +1,153 @@ > +-- test-run result file version 2 > +test_run = require('test_run').new() > + | --- > + | ... > +engine = test_run:get_cfg('engine') > + | --- > + | ... > +_ = box.space._session_settings:update('sql_default_engine', {{'=', 2, engine}}) > + | --- > + | ... > + > +box.execute([[CREATE TABLE t (b STRING PRIMARY KEY);]]) > + | --- > + | - row_count: 1 > + | ... > +box.execute([[INSERT INTO t VALUES ('aaaaaaaaaaaa');]]) > + | --- > + | - row_count: 1 > + | ... > +test_run:cmd("setopt delimiter ';'"); > + | --- > + | - true > + | ... > +box.schema.func.create('CORRUPT_SELECT', { > + language = 'LUA', > + returns = 'integer', > + body = [[ > + function(x) > + box.space.T:select() > + return 1 > + end]], > + is_sandboxed = false, > + param_list = { "string" }, > + exports = { 'LUA', 'SQL' }, > + is_deterministic = false, > + if_not_exists = true > +}); > + | --- > + | ... > +box.schema.func.create('CORRUPT_GET', { > + language = 'LUA', > + returns = 'integer', > + body = [[ > + function(x) > + box.space.T:get('aaaaaaaaaaaa') > + return 1 > + end]], > + is_sandboxed = false, > + param_list = { "string" }, > + exports = { 'LUA', 'SQL' }, > + is_deterministic = false, > + if_not_exists = true > +}); > + | --- > + | ... > +box.schema.func.create('CORRUPT_COUNT', { > + language = 'LUA', > + returns = 'integer', > + body = [[ > + function(x) > + box.space.T:count() > + return 1 > + end]], > + is_sandboxed = false, > + param_list = { "string" }, > + exports = { 'LUA', 'SQL' }, > + is_deterministic = false, > + if_not_exists = true > +}); > + | --- > + | ... > +box.schema.func.create('CORRUPT_MAX', { > + language = 'LUA', > + returns = 'integer', > + body = [[ > + function(x) > + box.space.T.index[0]:max() > + return 1 > + end]], > + is_sandboxed = false, > + param_list = { "string" }, > + exports = { 'LUA', 'SQL' }, > + is_deterministic = false, > + if_not_exists = true > +}); > + | --- > + | ... > +box.schema.func.create('CORRUPT_MIN', { > + language = 'LUA', > + returns = 'integer', > + body = [[ > + function(x) > + box.space.T.index[0]:min() > + return 1 > + end]], > + is_sandboxed = false, > + param_list = { "string" }, > + exports = { 'LUA', 'SQL' }, > + is_deterministic = false, > + if_not_exists = true > +}); > + | --- > + | ... > +test_run:cmd("setopt delimiter ''"); > + | --- > + | - true > + | ... > + > +box.execute([[select CORRUPT_SELECT(t.b) from t where t.b = ? and t.b <= ? order by t.b;]], {"aaaaaaaaaaaa", "aaaaaaaaaaaa"}) > + | --- > + | - metadata: > + | - name: COLUMN_1 > + | type: integer > + | rows: > + | - [1] > + | ... > +box.execute([[select CORRUPT_GET(t.b) from t where t.b = ? and t.b <= ? order by t.b;]], {"aaaaaaaaaaaa", "aaaaaaaaaaaa"}) > + | --- > + | - metadata: > + | - name: COLUMN_1 > + | type: integer > + | rows: > + | - [1] > + | ... > +box.execute([[select CORRUPT_COUNT(t.b) from t where t.b = ? and t.b <= ? order by t.b;]], {"aaaaaaaaaaaa", "aaaaaaaaaaaa"}) > + | --- > + | - metadata: > + | - name: COLUMN_1 > + | type: integer > + | rows: > + | - [1] > + | ... > +box.execute([[select CORRUPT_MAX(t.b) from t where t.b = ? and t.b <= ? order by t.b;]], {"aaaaaaaaaaaa", "aaaaaaaaaaaa"}) > + | --- > + | - metadata: > + | - name: COLUMN_1 > + | type: integer > + | rows: > + | - [1] > + | ... > +box.execute([[select CORRUPT_MIN(t.b) from t where t.b = ? and t.b <= ? order by t.b;]], {"aaaaaaaaaaaa", "aaaaaaaaaaaa"}) 3. Please, keep 80 symbols line width limit. > + | --- > + | - metadata: > + | - name: COLUMN_1 > + | type: integer > + | rows: > + | - [1] > + | ... > + > +box.execute([[DROP TABLE t;]]) 4. You also need to drop the functions. > + | --- > + | - row_count: 1 > + | ...