Tarantool development patches archive
 help / color / mirror / Atom feed
From: Konstantin Osipov <kostja@tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH 2/2] netbox: reuse _request() to do SQL execute()
Date: Thu, 5 Apr 2018 23:07:27 +0300	[thread overview]
Message-ID: <20180405200727.GA3953@atlas> (raw)
In-Reply-To: <9401c4f91662037dca337c6d320c5952c61863fb.1522956408.git.v.shpilevoy@tarantool.org>

* Vladislav Shpilevoy <v.shpilevoy@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

  reply	other threads:[~2018-04-05 20:07 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-05 19:28 [tarantool-patches] [PATCH 0/2] " Vladislav Shpilevoy
2018-04-05 19:28 ` [tarantool-patches] [PATCH 1/2] netbox: forbid conn:timeout():execute(...) Vladislav Shpilevoy
2018-04-05 20:08   ` [tarantool-patches] " Konstantin Osipov
2018-04-05 19:28 ` [tarantool-patches] [PATCH 2/2] netbox: reuse _request() to do SQL execute() Vladislav Shpilevoy
2018-04-05 20:07   ` Konstantin Osipov [this message]
2018-04-05 23:30     ` [tarantool-patches] " Vladislav Shpilevoy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180405200727.GA3953@atlas \
    --to=kostja@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='[tarantool-patches] Re: [PATCH 2/2] netbox: reuse _request() to do SQL execute()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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