From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id B37FD2D5F3 for ; Thu, 5 Apr 2018 16:07:30 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id NhOaas3-0Yzj for ; Thu, 5 Apr 2018 16:07:30 -0400 (EDT) Received: from smtp61.i.mail.ru (smtp61.i.mail.ru [217.69.128.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 505242D5D9 for ; Thu, 5 Apr 2018 16:07:30 -0400 (EDT) Date: Thu, 5 Apr 2018 23:07:27 +0300 From: Konstantin Osipov Subject: [tarantool-patches] Re: [PATCH 2/2] netbox: reuse _request() to do SQL execute() Message-ID: <20180405200727.GA3953@atlas> References: <9401c4f91662037dca337c6d320c5952c61863fb.1522956408.git.v.shpilevoy@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <9401c4f91662037dca337c6d320c5952c61863fb.1522956408.git.v.shpilevoy@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Vladislav Shpilevoy Cc: tarantool-patches@freelists.org * Vladislav Shpilevoy [18/04/05 22:31]: Please use select(1, f()) instead of first_value(). Otherwise the patch is OK to push. > _request() is a wrapper for perform_request, that detects schema > changes, and waits until it is reloaded. Lets use _request() > instead of direct perform_request() for execute(). The reason why > the _request() was not used earlier was my attempt to avoid > multiple return values in _request(), that leads to minor fixes > in non-execute methods like index.select or eval/call_16, which > return the _request() directly. > > But diplicating schema reloading logic for execute() is worse. > > Closes #3323 > Closes #3322 > --- > src/box/lua/net_box.lua | 57 +++++++++++++++++++++++++---------------------- > test/box/net.box.result | 5 +++-- > test/box/net.box.test.lua | 5 +++-- > test/sql/iproto.result | 45 ------------------------------------- > test/sql/iproto.test.lua | 15 ------------- > 5 files changed, 36 insertions(+), 91 deletions(-) > > diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua > index 8d1955315..ccf40df06 100644 > --- a/src/box/lua/net_box.lua > +++ b/src/box/lua/net_box.lua > @@ -848,33 +848,41 @@ function remote_methods:_request(method, opts, ...) > deadline = self._deadlines[this_fiber] > end > local buffer = opts and opts.buffer > - local err, res > + local err, res, metadata, info > repeat > local timeout = deadline and max(0, deadline - fiber_clock()) > if self.state ~= 'active' then > wait_state('active', timeout) > timeout = deadline and max(0, deadline - fiber_clock()) > end > - err, res = perform_request(timeout, buffer, method, > - self.schema_version, ...) > + err, res, metadata, info = > + perform_request(timeout, buffer, method, self.schema_version, ...) > if not err and buffer ~= nil then > return res -- the length of xrow.body > elseif not err then > - setmetatable(res, sequence_mt) > - local postproc = method ~= 'eval' and method ~= 'call_17' > - if postproc then > - local tnew = box.tuple.new > - for i, v in pairs(res) do > - res[i] = tnew(v) > + -- SQL execute can return info with no data. > + if res then > + setmetatable(res, sequence_mt) > + local postproc = method ~= 'eval' and method ~= 'call_17' > + if postproc then > + local tnew = box.tuple.new > + for i, v in pairs(res) do > + res[i] = tnew(v) > + end > end > end > - return res > + return res, metadata, info > elseif err == E_WRONG_SCHEMA_VERSION then > err = nil > end > until err > box.error({code = err, reason = res}) > end > +-- > +-- Get multiple values and return the first only. Use to ignore > +-- tail consisting of nils. > +-- > +local function first_value(arg) return arg end > > function remote_methods:ping(opts) > check_remote_arg(self, 'ping') > @@ -900,7 +908,8 @@ end > -- @deprecated since 1.7.4 > function remote_methods:call_16(func_name, ...) > check_remote_arg(self, 'call') > - return self:_request('call_16', nil, tostring(func_name), {...}) > + return first_value(self:_request('call_16', nil, tostring(func_name), > + {...})) > end > > function remote_methods:call(func_name, args, opts) > @@ -917,7 +926,7 @@ end > -- @deprecated since 1.7.4 > function remote_methods:eval_16(code, ...) > check_remote_arg(self, 'eval') > - return unpack(self:_request('eval', nil, code, {...})) > + return unpack(first_value(self:_request('eval', nil, code, {...}))) > end > > function remote_methods:eval(code, args, opts) > @@ -936,18 +945,10 @@ function remote_methods:execute(query, parameters, sql_opts, netbox_opts) > if sql_opts ~= nil then > box.error(box.error.UNSUPPORTED, "execute", "options") > end > - local timeout = netbox_opts and netbox_opts.timeout > - local buffer = netbox_opts and netbox_opts.buffer > - parameters = parameters or {} > - sql_opts = sql_opts or {} > - local err, res, metadata, info = > - self._transport.perform_request(timeout, buffer, 'execute', > - self.schema_version, query, parameters, > - sql_opts) > - if err then > - box.error({code = err, reason = res}) > - end > - if buffer ~= nil then > + local res, metadata, info = > + self:_request('execute', netbox_opts, query, parameters or {}, > + sql_opts or {}) > + if netbox_opts and netbox_opts.buffer then > return res -- body length. Body is written to the buffer. > end > assert((info == nil and metadata ~= nil and res ~= nil) or > @@ -1180,8 +1181,9 @@ index_metatable = function(remote) > local iterator = check_iterator_type(opts, key_is_nil) > local offset = tonumber(opts and opts.offset) or 0 > local limit = tonumber(opts and opts.limit) or 0xFFFFFFFF > - return remote:_request('select', opts, self.space.id, self.id, > - iterator, offset, limit, key) > + return first_value(remote:_request('select', opts, self.space.id, > + self.id, iterator, offset, limit, > + key)) > end > > function methods:get(key, opts) > @@ -1222,7 +1224,8 @@ index_metatable = function(remote) > end > local code = string.format('box.space.%s.index.%s:count', > self.space.name, self.name) > - return remote:_request('call_16', opts, code, { key })[1][1] > + return first_value(remote:_request('call_16', opts, code, > + { key }))[1][1] > end > > function methods:delete(key, opts) > diff --git a/test/box/net.box.result b/test/box/net.box.result > index cf7b27f0b..1c95fe0aa 100644 > --- a/test/box/net.box.result > +++ b/test/box/net.box.result > @@ -25,8 +25,9 @@ test_run:cmd("setopt delimiter ';'") > - true > ... > function x_select(cn, space_id, index_id, iterator, offset, limit, key, opts) > - return cn:_request('select', opts, space_id, index_id, iterator, > - offset, limit, key) > + local ret = cn:_request('select', opts, space_id, index_id, iterator, > + offset, limit, key) > + return ret > end > function x_fatal(cn) cn._transport.perform_request(nil, nil, 'inject', nil, '\x80') end > test_run:cmd("setopt delimiter ''"); > diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua > index 576b5cfea..03ca5cc0f 100644 > --- a/test/box/net.box.test.lua > +++ b/test/box/net.box.test.lua > @@ -8,8 +8,9 @@ test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:: '") > > test_run:cmd("setopt delimiter ';'") > function x_select(cn, space_id, index_id, iterator, offset, limit, key, opts) > - return cn:_request('select', opts, space_id, index_id, iterator, > - offset, limit, key) > + local ret = cn:_request('select', opts, space_id, index_id, iterator, > + offset, limit, key) > + return ret > end > function x_fatal(cn) cn._transport.perform_request(nil, nil, 'inject', nil, '\x80') end > test_run:cmd("setopt delimiter ''"); > diff --git a/test/sql/iproto.result b/test/sql/iproto.result > index f7749cb05..39e51d84c 100644 > --- a/test/sql/iproto.result > +++ b/test/sql/iproto.result > @@ -286,9 +286,6 @@ cn:execute('create table test2(id primary key, a, b, c)') > --- > - rowcount: 1 > ... > -cn:reload_schema() > ---- > -... > box.space.TEST2.name > --- > - TEST2 > @@ -307,9 +304,6 @@ cn:execute('create index test2_a_b_index on test2(a, b)') > --- > - rowcount: 1 > ... > -cn:reload_schema() > ---- > -... > #box.space.TEST2.index > --- > - 1 > @@ -318,9 +312,6 @@ cn:execute('drop table test2') > --- > - rowcount: 1 > ... > -cn:reload_schema() > ---- > -... > box.space.TEST2 > --- > - null > @@ -333,9 +324,6 @@ cn:execute('create table test3(id primary key, a, b)') > ... > -- Rowcount = 1, although two tuples were created: > -- for _space and for _index. > -cn:reload_schema() > ---- > -... > cn:execute('insert into test3 values (1, 1, 1), (2, 2, 2), (3, 3, 3)') > --- > - rowcount: 3 > @@ -350,9 +338,6 @@ cn:execute('create view test3_view(id) as select id from test3') > --- > - rowcount: 1 > ... > -cn:reload_schema() > ---- > -... > cn:execute('create view if not exists test3_view(id) as select id from test3') > --- > - rowcount: 0 > @@ -361,9 +346,6 @@ cn:execute('drop view test3_view') > --- > - rowcount: 1 > ... > -cn:reload_schema() > ---- > -... > cn:execute('drop view if exists test3_view') > --- > - rowcount: 0 > @@ -374,9 +356,6 @@ cn:execute('create index test3_sec on test3(a, b)') > --- > - rowcount: 1 > ... > -cn:reload_schema() > ---- > -... > cn:execute('create index if not exists test3_sec on test3(a, b)') > --- > - rowcount: 0 > @@ -385,9 +364,6 @@ cn:execute('drop index test3_sec on test3') > --- > - rowcount: 1 > ... > -cn:reload_schema() > ---- > -... > cn:execute('drop index if exists test3_sec on test3') > --- > - rowcount: 0 > @@ -398,9 +374,6 @@ cn:execute('create trigger trig INSERT ON test3 BEGIN SELECT * FROM test3; END;' > --- > - rowcount: 1 > ... > -cn:reload_schema() > ---- > -... > cn:execute('create trigger if not exists trig INSERT ON test3 BEGIN SELECT * FROM test3; END;') > --- > - rowcount: 0 > @@ -409,9 +382,6 @@ cn:execute('drop trigger trig') > --- > - rowcount: 1 > ... > -cn:reload_schema() > ---- > -... > cn:execute('drop trigger if exists trig') > --- > - rowcount: 0 > @@ -422,29 +392,17 @@ cn:execute('create index idx1 on test3(a)') > --- > - rowcount: 1 > ... > -cn:reload_schema() > ---- > -... > cn:execute('create index idx2 on test3(b)') > --- > - rowcount: 1 > ... > -cn:reload_schema() > ---- > -... > box.space.TEST3:truncate() > --- > ... > -cn:reload_schema() > ---- > -... > cn:execute('create trigger trig INSERT ON test3 BEGIN SELECT * FROM test3; END;') > --- > - rowcount: 1 > ... > -cn:reload_schema() > ---- > -... > cn:execute('insert into test3 values (1, 1, 1), (2, 2, 2), (3, 3, 3)') > --- > - rowcount: 3 > @@ -453,9 +411,6 @@ cn:execute('drop table test3') > --- > - rowcount: 1 > ... > -cn:reload_schema() > ---- > -... > cn:execute('drop table if exists test3') > --- > - rowcount: 0 > diff --git a/test/sql/iproto.test.lua b/test/sql/iproto.test.lua > index 64c0a56fe..faf844226 100644 > --- a/test/sql/iproto.test.lua > +++ b/test/sql/iproto.test.lua > @@ -111,15 +111,12 @@ cn:execute('select :value', parameters) > > -- gh-2608 SQL iproto DDL > cn:execute('create table test2(id primary key, a, b, c)') > -cn:reload_schema() > box.space.TEST2.name > cn:execute('insert into test2 values (1, 1, 1, 1)') > cn:execute('select * from test2') > cn:execute('create index test2_a_b_index on test2(a, b)') > -cn:reload_schema() > #box.space.TEST2.index > cn:execute('drop table test2') > -cn:reload_schema() > box.space.TEST2 > > -- gh-2617 DDL row_count either 0 or 1. > @@ -128,50 +125,38 @@ box.space.TEST2 > cn:execute('create table test3(id primary key, a, b)') > -- Rowcount = 1, although two tuples were created: > -- for _space and for _index. > -cn:reload_schema() > cn:execute('insert into test3 values (1, 1, 1), (2, 2, 2), (3, 3, 3)') > cn:execute('create table if not exists test3(id primary key)') > > -- Test CREATE VIEW [IF NOT EXISTS] and > -- DROP VIEW [IF EXISTS]. > cn:execute('create view test3_view(id) as select id from test3') > -cn:reload_schema() > cn:execute('create view if not exists test3_view(id) as select id from test3') > cn:execute('drop view test3_view') > -cn:reload_schema() > cn:execute('drop view if exists test3_view') > > -- Test CREATE INDEX [IF NOT EXISTS] and > -- DROP INDEX [IF EXISTS]. > cn:execute('create index test3_sec on test3(a, b)') > -cn:reload_schema() > cn:execute('create index if not exists test3_sec on test3(a, b)') > cn:execute('drop index test3_sec on test3') > -cn:reload_schema() > cn:execute('drop index if exists test3_sec on test3') > > -- Test CREATE TRIGGER [IF NOT EXISTS] and > -- DROP TRIGGER [IF EXISTS]. > cn:execute('create trigger trig INSERT ON test3 BEGIN SELECT * FROM test3; END;') > -cn:reload_schema() > cn:execute('create trigger if not exists trig INSERT ON test3 BEGIN SELECT * FROM test3; END;') > cn:execute('drop trigger trig') > -cn:reload_schema() > cn:execute('drop trigger if exists trig') > > -- Test DROP TABLE [IF EXISTS]. > -- Create more indexes, triggers and _truncate tuple. > cn:execute('create index idx1 on test3(a)') > -cn:reload_schema() > cn:execute('create index idx2 on test3(b)') > -cn:reload_schema() > box.space.TEST3:truncate() > -cn:reload_schema() > cn:execute('create trigger trig INSERT ON test3 BEGIN SELECT * FROM test3; END;') > -cn:reload_schema() > cn:execute('insert into test3 values (1, 1, 1), (2, 2, 2), (3, 3, 3)') > cn:execute('drop table test3') > -cn:reload_schema() > cn:execute('drop table if exists test3') > > -- gh-2602 obuf_alloc breaks the tuple in different slabs > -- > 2.14.3 (Apple Git-98) > -- Konstantin Osipov, Moscow, Russia, +7 903 626 22 32 http://tarantool.io - www.twitter.com/kostja_osipov