[tarantool-patches] Re: [PATCH 2/2] netbox: reuse _request() to do SQL execute()
Konstantin Osipov
kostja at tarantool.org
Thu Apr 5 23:07:27 MSK 2018
* Vladislav Shpilevoy <v.shpilevoy at tarantool.org> [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...\"]:<line>: '")
>
> 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
More information about the Tarantool-patches
mailing list