[tarantool-patches] Re: [PATCH] net.box: ignore absence of _vcollation

[Alexander Turenko <alexander.turenko@tarantool.org>]

Thank you for the detailed overview and the ways to solve the problem!

> Talking of this issue: your situation is simpler, as I see now, when I dived a
> bit into the problem. You have nothing to do about the netbox object. Your target
> is the worker fiber only. And in such a case I don't see any magic solution that
> would shrink this patch https://github.com/tarantool/tarantool/commit/abc88ef8d38e21a6c7faf925a19105ce65d45467
> to a couple of lines. The only obvious thing you can do here is to avoid passing
> version id to iproto_auth_sm and iproto_schema_sm functions, and Alexander proposed.
> Just make it a global value here
> https://github.com/tarantool/tarantool/blob/0529e2f0ef7dd4c19283b9e8e6b0e7e43bf400a3/src/box/lua/net_box.lua#L232
> together with the others, if you still will need.

I found it is even simpler: we already have greeting in this context and can
use it directly.

> Talking of how to remove branching - it has nothing to do with netbox specific code,
> or GC, or anything application dependent. It is a pure technical matter. You have a
> classic problem already solved in box/lua/upgrade.lua. The problem is that you have a
> pipeline of versions, each adding, or removing, or changing something. And it is logical
> to have a pipeline of functions building that schema step by step. IMO, you should have
> several functions to fetch and to install schema.
> 
> Foe example:
> 
> function fetch_schema_2_2_1()
> ...
> end
> 
> function fetch_schema_2_2_2()
>    fetch_schema_2_2_1()
>    -- + some v_collation things.
> end
> 
> function fetch_schema_2_2_3()
>    fetch_schema_2_2_3()
>    --- + ...
> end
> 
> And you have a global table where key is version id, value is a schema fetch
> function. Here
> https://github.com/tarantool/tarantool/commit/abc88ef8d38e21a6c7faf925a19105ce65d45467#diff-a9dd9be71f8af749f288abb6304a5185R733
> you take a fetcher from the table like 'fetcher = fetchers[version_id]', and just
> call it.

We'll either need to add a fetcher for each release or choose a last
that is not newer then a peer's version_id.

> All these encode selects
> https://github.com/tarantool/tarantool/commit/abc88ef8d38e21a6c7faf925a19105ce65d45467#diff-a9dd9be71f8af749f288abb6304a5185R754
> should move to these schema fetchers. Of course, it wouldn't be that simple. At least,
> because schema fetch requests are sent in parallel, as you can see here:
> https://github.com/tarantool/tarantool/blob/0529e2f0ef7dd4c19283b9e8e6b0e7e43bf400a3/src/box/lua/net_box.lua#L743-L748
> And still the idea is the same. Perhaps you will have not only fetcher for each
> version, but two functions: start_fetch/end_fetch, or begin_fetch/end_fetch. A
> first function sends a request (encode_select), a second one waits for a response.
> Or let these fetchers return an asynchronous id, by which you will wait for
> responses in one place. Don't know what to chose. If I were you, and I had time, I
> would try to implement these step-wise fetchers. They would be quite easy to
> support and to extend for other future schema changes.

The idea is interesting. This however would take some effort (several
days for me I guess) to experimenting and find a good variant. Sadly, it
seems I can not pay enough time to that.

> Perhaps, it will remove the branching. Up to you what to do, to Alexander, and to
> other managers. My only wish is to look at the final patch when it will be ready.

Please, look at the patch below or on
Totktonada/gh-4307-net-box-fix-schema-fetch branch if the time allows. It is
more or less the same as Roman's one, but I was charged by your great email and
tried to minimize the patch as possible.

----

commit fff809e6e3d253245a1c596f3c2d51ab983f3efe
Author: Alexander Turenko <alexander.turenko@tarantool.org>
Date:   Tue Jul 30 03:38:22 2019 +0300

    net.box: fix schema fetching from 1.10/2.1 servers
    
    After 2.2.0-390-ga7c855e5b ("net.box: fetch '_vcollation' sysview into
    the module") net.box fetches _vcollation view unconditionally, while the
    view was added in 2.2.0-389-g3e3ef182f and, say, tarantool-1.10 and
    tarantool-2.1 do not have it. This leads to a runtime error "Space '277'
    does not exist" on a newer client that connects to an older server.
    
    Now the view is fetched conditionally depending of a version of a
    server: if it is above 2.2.1, then net.box will fetch it. Note: at the
    time there are no release with a number above 2.2.1.
    
    When _vcollation view is available, a collation in an index part will be
    shown by its name (with 'collation' field), otherwise it will be shown
    by its ID (in 'collation_id' field). For example:
    
    Connect to tarantool 1.10:
    
     | tarantool> connection = require('net.box').connect('localhost:3301')
     | ---
     | ...
     |
     | tarantool> connection.space.s.index.sk.parts
     | ---
     | - - type: string
     |     is_nullable: false
     |     collation_id: 2
     |     fieldno: 2
     | ...
    
    Connect to tarantool 2.2.1 (when it will be released):
    
     | tarantool> connection = require('net.box').connect('localhost:3301')
     | ---
     | ...
     |
     | tarantool> connection.space.s.index.sk.parts
     | ---
     | - - type: string
     |     is_nullable: false
     |     collation: unicode_ci
     |     fieldno: 2
     | ...
    
    Fixes #4307.

diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 2892bb3d3..31a8c16b7 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -84,6 +84,14 @@ local function decode_push(raw_data)
     return response[IPROTO_DATA_KEY][1], raw_end
 end
 
+local function version_id(major, minor, patch)
+    return bit.bor(bit.lshift(major, 16), bit.lshift(minor, 8), patch)
+end
+
+local function version_at_least(peer_version_id, major, minor, patch)
+    return peer_version_id >= version_id(major, minor, patch)
+end
+
 local method_encoder = {
     ping    = internal.encode_ping,
     call_16 = internal.encode_call_16,
@@ -735,17 +743,23 @@ local function create_transport(host, port, user, password, callback,
             set_state('active')
             return iproto_sm(schema_version)
         end
+        -- _vcollation view was added in 2.2.0-389-g3e3ef182f
+        local peer_has_vcollation = version_at_least(greeting.version_id,
+                                                     2, 2, 1)
         local select1_id = new_request_id()
         local select2_id = new_request_id()
-        local select3_id = new_request_id()
+        local select3_id
         local response = {}
         -- fetch everything from space _vspace, 2 = ITER_ALL
         encode_select(send_buf, select1_id, VSPACE_ID, 0, 2, 0, 0xFFFFFFFF, nil)
         -- fetch everything from space _vindex, 2 = ITER_ALL
         encode_select(send_buf, select2_id, VINDEX_ID, 0, 2, 0, 0xFFFFFFFF, nil)
         -- fetch everything from space _vcollation, 2 = ITER_ALL
-        encode_select(send_buf, select3_id, VCOLLATION_ID, 0, 2, 0, 0xFFFFFFFF,
-                      nil)
+        if peer_has_vcollation then
+            select3_id = new_request_id()
+            encode_select(send_buf, select3_id, VCOLLATION_ID, 0, 2, 0,
+                          0xFFFFFFFF, nil)
+        end
 
         schema_version = nil -- any schema_version will do provided that
                              -- it is consistent across responses
@@ -754,6 +768,8 @@ local function create_transport(host, port, user, password, callback,
             if err then return error_sm(err, hdr) end
             dispatch_response_iproto(hdr, body_rpos, body_end)
             local id = hdr[IPROTO_SYNC_KEY]
+            -- trick: omit check for peer_has_vcollation: id is
+            -- not nil
             if id == select1_id or id == select2_id or id == select3_id then
                 -- response to a schema query we've submitted
                 local status = hdr[IPROTO_STATUS_KEY]
@@ -774,9 +790,10 @@ local function create_transport(host, port, user, password, callback,
                 response[id] = body[IPROTO_DATA_KEY]
             end
         until response[select1_id] and response[select2_id] and
-              response[select3_id]
+              (not peer_has_vcollation or response[select3_id])
+        -- trick: response[select3_id] is nil when the key is nil
         callback('did_fetch_schema', schema_version, response[select1_id],
-                 response[select2_id],response[select3_id])
+                 response[select2_id], response[select3_id])
         set_state('active')
         return iproto_sm(schema_version)
     end
@@ -1269,17 +1286,23 @@ function remote_methods:_install_schema(schema_version, spaces, indices,
                 local pkcollationid = index[PARTS][k].collation
                 local pktype = index[PARTS][k][2] or index[PARTS][k].type
                 local pkfield = index[PARTS][k][1] or index[PARTS][k].field
+                -- resolve a collation name if a peer has
+                -- _vcollation view
                 local pkcollation = nil
-                if pkcollationid ~= nil then
+                if pkcollationid ~= nil and collations ~= nil then
                     pkcollation = collations[pkcollationid + 1][2]
                 end
 
                 local pk = {
                     type = pktype,
                     fieldno = pkfield + 1,
-                    collation = pkcollation,
                     is_nullable = pknullable
                 }
+                if collations == nil then
+                    pk.collation_id = pkcollationid
+                else
+                    pk.collation = pkcollation
+                end
                 idx.parts[k] = pk
             end
             idx.unique = not not index[OPTS].unique
diff --git a/test/box/net.box.result b/test/box/net.box.result
index 1d9f43400..e3dabf7d9 100644
--- a/test/box/net.box.result
+++ b/test/box/net.box.result
@@ -2905,18 +2905,41 @@ c = net:connect(box.cfg.listen)
 box.internal.collation.create('test', 'ICU', 'ru-RU')
 ---
 ...
+collation_id = box.internal.collation.id_by_name('test')
+---
+...
 _ = space:create_index('sk', { type = 'tree', parts = {{1, 'str', collation = 'test'}}, unique = true })
 ---
 ...
 c:reload_schema()
 ---
 ...
-c.space.test.index.sk.parts
+parts = c.space.test.index.sk.parts
 ---
-- - type: string
-    is_nullable: false
-    collation: test
-    fieldno: 1
+...
+#parts == 1
+---
+- true
+...
+parts[1].fieldno == 1
+---
+- true
+...
+parts[1].type == 'string'
+---
+- true
+...
+parts[1].is_nullable == false
+---
+- true
+...
+if _TARANTOOL >= '2.2.1' then                    \
+    return parts[1].collation == 'test'          \
+else                                             \
+    return parts[1].collation_id == collation_id \
+end
+---
+- true
 ...
 c:close()
 ---
diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
index de629ab59..8e65ff470 100644
--- a/test/box/net.box.test.lua
+++ b/test/box/net.box.test.lua
@@ -1195,9 +1195,19 @@ c:close()
 box.schema.user.grant('guest', 'read', 'space', 'test')
 c = net:connect(box.cfg.listen)
 box.internal.collation.create('test', 'ICU', 'ru-RU')
+collation_id = box.internal.collation.id_by_name('test')
 _ = space:create_index('sk', { type = 'tree', parts = {{1, 'str', collation = 'test'}}, unique = true })
 c:reload_schema()
-c.space.test.index.sk.parts
+parts = c.space.test.index.sk.parts
+#parts == 1
+parts[1].fieldno == 1
+parts[1].type == 'string'
+parts[1].is_nullable == false
+if _TARANTOOL >= '2.2.1' then                    \
+    return parts[1].collation == 'test'          \
+else                                             \
+    return parts[1].collation_id == collation_id \
+end
 c:close()
 box.internal.collation.drop('test')
 space:drop()
diff --git a/test/box/stat_net.result b/test/box/stat_net.result
index 9eeada7ec..2cf567d57 100644
--- a/test/box/stat_net.result
+++ b/test/box/stat_net.result
@@ -83,9 +83,9 @@ box.stat.net.CONNECTIONS.total
 ---
 - 4
 ...
-box.stat.net.REQUESTS.total
+box.stat.net.REQUESTS.total > 0
 ---
-- 17
+- true
 ...
 box.stat.net.CONNECTIONS.current
 ---
@@ -115,6 +115,9 @@ test_run:wait_cond(function() return box.stat.net.CONNECTIONS.current == 1 end,
 ---
 - true
 ...
+requests_total_saved = box.stat.net.REQUESTS.total
+---
+...
 future1 = cn:call('tweedledee', {}, {is_async = true})
 ---
 ...
@@ -149,9 +152,9 @@ test_run:wait_cond(function() return box.stat.net.REQUESTS.current == 0 end, WAI
 ---
 - true
 ...
-box.stat.net.REQUESTS.total
+box.stat.net.REQUESTS.total - requests_total_saved == 2
 ---
-- 19
+- true
 ...
 -- reset
 box.stat.reset()
diff --git a/test/box/stat_net.test.lua b/test/box/stat_net.test.lua
index 830deb36b..a1dd324bd 100644
--- a/test/box/stat_net.test.lua
+++ b/test/box/stat_net.test.lua
@@ -29,7 +29,7 @@ cn.space.tweedledum:select() --small request
 box.stat.net.SENT.total > 0
 box.stat.net.RECEIVED.total > 0
 box.stat.net.CONNECTIONS.total
-box.stat.net.REQUESTS.total
+box.stat.net.REQUESTS.total > 0
 box.stat.net.CONNECTIONS.current
 box.stat.net.REQUESTS.current
 
@@ -41,6 +41,7 @@ test_run:wait_cond(function() return box.stat.net.CONNECTIONS.current == 2 end,
 cn3:close()
 test_run:wait_cond(function() return box.stat.net.CONNECTIONS.current == 1 end, WAIT_COND_TIMEOUT)
 
+requests_total_saved = box.stat.net.REQUESTS.total
 future1 = cn:call('tweedledee', {}, {is_async = true})
 test_run:wait_cond(function() return box.stat.net.REQUESTS.current == 1 end, WAIT_COND_TIMEOUT)
 future2 = cn:call('tweedledee', {}, {is_async = true})
@@ -50,7 +51,7 @@ ch:put(true)
 future1:wait_result()
 future2:wait_result()
 test_run:wait_cond(function() return box.stat.net.REQUESTS.current == 0 end, WAIT_COND_TIMEOUT)
-box.stat.net.REQUESTS.total
+box.stat.net.REQUESTS.total - requests_total_saved == 2
 
 -- reset
 box.stat.reset()