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

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

[Roman Khabibov]

Ignore error about absence of _vcollation, when a client connects
to a server of old Tarantool versions, where this space does not
exist.

Closes #4307
---
Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-4307-vcoll
Issue: https://github.com/tarantool/tarantool/issues/4307

 src/box/lua/net_box.lua | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 2892bb3d3..723e63045 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -761,6 +761,11 @@ local function create_transport(host, port, user, password, callback,
                 if status ~= 0 then
                     local body
                     body, body_end = decode(body_rpos)
+                    local err_msg = 'Space \'277\' does not exist'
+                    if id == select3_id and string.find(body[IPROTO_ERROR_KEY],
+                                                        err_msg) ~= nil then
+                        break
+                    end
                     return error_sm(E_NO_CONNECTION, body[IPROTO_ERROR_KEY])
                 end
                 if schema_version == nil then
@@ -776,7 +781,7 @@ local function create_transport(host, port, user, password, callback,
         until response[select1_id] and response[select2_id] and
               response[select3_id]
         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
@@ -1270,16 +1275,28 @@ function remote_methods:_install_schema(schema_version, spaces, indices,
                 local pktype = index[PARTS][k][2] or index[PARTS][k].type
                 local pkfield = index[PARTS][k][1] or index[PARTS][k].field
                 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
-                }
+                local pk
+                if collations ~= nil then
+                    pk = {
+                        type = pktype,
+                        fieldno = pkfield + 1,
+                        collation = pkcollation,
+                        is_nullable = pknullable
+                    }
+                else
+                    pk = {
+                        type = pktype,
+                        fieldno = pkfield + 1,
+                        collation_id = pkcollationid,
+                        is_nullable = pknullable
+                    }
+                end
+
+
                 idx.parts[k] = pk
             end
             idx.unique = not not index[OPTS].unique
-- 
2.20.1 (Apple Git-117)

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

[Kirill Yukhin]

Hello,

On 08 Jul 15:13, Roman Khabibov wrote:
> Ignore error about absence of _vcollation, when a client connects
> to a server of old Tarantool versions, where this space does not
> exist.
> 
> Closes #4307
> ---
> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-4307-vcoll
> Issue: https://github.com/tarantool/tarantool/issues/4307

Add a regression test please.

> 
>  src/box/lua/net_box.lua | 33 +++++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index 2892bb3d3..723e63045 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -761,6 +761,11 @@ local function create_transport(host, port, user, password, callback,
>                  if status ~= 0 then
>                      local body
>                      body, body_end = decode(body_rpos)
> +                    local err_msg = 'Space \'277\' does not exist'

Space 277? Seriously? This code handles not only collations request.
Should be more generic, IMHO.

> @@ -1270,16 +1275,28 @@ function remote_methods:_install_schema(schema_version, spaces, indices,
>                  local pktype = index[PARTS][k][2] or index[PARTS][k].type
>                  local pkfield = index[PARTS][k][1] or index[PARTS][k].field
>                  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
> -                }
> +                local pk
> +                if collations ~= nil then
> +                    pk = {
> +                        type = pktype,
> +                        fieldno = pkfield + 1,
> +                        collation = pkcollation,
> +                        is_nullable = pknullable
> +                    }
> +                else
> +                    pk = {
> +                        type = pktype,
> +                        fieldno = pkfield + 1,
> +                        collation_id = pkcollationid,
> +                        is_nullable = pknullable
> +                    }
> +                end

IMHO, it'd be better to not duplicate code and put actual difference
under if-then-else hammock.

--
Regards, Kirill Yukhin

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

[Roman Khabibov]

> On Jul 12, 2019, at 12:55, Kirill Yukhin <kyukhin@tarantool.org> wrote:
> 
> Hello,
> 
> On 08 Jul 15:13, Roman Khabibov wrote:
>> Ignore error about absence of _vcollation, when a client connects
>> to a server of old Tarantool versions, where this space does not
>> exist.
>> 
>> Closes #4307
>> ---
>> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-4307-vcoll
>> Issue: https://github.com/tarantool/tarantool/issues/4307

>> 
>> src/box/lua/net_box.lua | 33 +++++++++++++++++++++++++--------
>> 1 file changed, 25 insertions(+), 8 deletions(-)
>> 
>> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
>> index 2892bb3d3..723e63045 100644
>> --- a/src/box/lua/net_box.lua
>> +++ b/src/box/lua/net_box.lua
>> @@ -761,6 +761,11 @@ local function create_transport(host, port, user, password, callback,
>>                 if status ~= 0 then
>>                     local body
>>                     body, body_end = decode(body_rpos)
>> +                    local err_msg = 'Space \'277\' does not exist'
> 
> Space 277? Seriously? This code handles not only collations request.
> Should be more generic, IMHO.
@@ -685,7 +685,7 @@ local function create_transport(host, port, user, password, callback,
             set_state('active')
             return console_sm(rid)
         elseif greeting.protocol == 'Binary' then
-            return iproto_auth_sm(greeting.salt)
+            return iproto_auth_sm(greeting.salt, greeting.version_id)
         else
             return error_sm(E_NO_CONNECTION,
                             'Unknown protocol: '..greeting.protocol)
@@ -710,11 +710,11 @@ local function create_transport(host, port, user, password, callback,
         end
     end
 
-    iproto_auth_sm = function(salt)
+    iproto_auth_sm = function(salt, version_id)
         set_state('auth')
         if not user or not password then
             set_state('fetch_schema')
-            return iproto_schema_sm()
+            return iproto_schema_sm(nil, version_id)
         end
         encode_auth(send_buf, new_request_id(), user, password, salt)
         local err, hdr, body_rpos, body_end = send_and_recv_iproto()
@@ -727,34 +727,52 @@ local function create_transport(host, port, user, password, callback,
             return error_sm(E_NO_CONNECTION, body[IPROTO_ERROR_KEY])
         end
         set_state('fetch_schema')
-        return iproto_schema_sm(hdr[IPROTO_SCHEMA_VERSION_KEY])
+        return iproto_schema_sm(hdr[IPROTO_SCHEMA_VERSION_KEY], version_id)
     end
 
-    iproto_schema_sm = function(schema_version)
+    iproto_schema_sm = function(schema_version, version_id)
         if not callback('will_fetch_schema') then
             set_state('active')
-            return iproto_sm(schema_version)
+            return iproto_sm(schema_version, version_id)
         end
+        local need_vcoll
+        -- 131584 is tarantool_version_id() for 2.2.0.
+        if version_id ~= nil and version_id >= 131584 then
+            need_vcoll = true
+        else
+            need_vcoll = false
+        end
+
         local select1_id = new_request_id()
         local select2_id = new_request_id()
-        local select3_id = new_request_id()
+        local select3_id = nil
         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 need_vcoll == true then
+            select3_id = new_request_id()
+            -- fetch everything from space _vcollation, 2 = ITER_ALL
+            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
+
+        local function vcoll_response(need_vcoll)
+            if need_vcoll == true then
+                return response[select3_id]
+            end
+            return true
+        end
         repeat
             local err, hdr, body_rpos, body_end = send_and_recv_iproto()
             if err then return error_sm(err, hdr) end
             dispatch_response_iproto(hdr, body_rpos, body_end)
             local id = hdr[IPROTO_SYNC_KEY]
-            if id == select1_id or id == select2_id or id == select3_id then
+            if id == select1_id or id == select2_id or id == select3_id and need_vcoll then
                 -- response to a schema query we've submitted
                 local status = hdr[IPROTO_STATUS_KEY]
                 local response_schema_version = hdr[IPROTO_SCHEMA_VERSION_KEY]
@@ -767,21 +785,21 @@ local function create_transport(host, port, user, password, callback,
                     schema_version = response_schema_version
                 elseif schema_version ~= response_schema_version then
                     -- schema changed while fetching schema; restart loader
-                    return iproto_schema_sm()
+                    return iproto_schema_sm(nil, version_id)
                 end
                 local body
                 body, body_end = decode(body_rpos)
                 response[id] = body[IPROTO_DATA_KEY]
             end
         until response[select1_id] and response[select2_id] and
-              response[select3_id]
+              vcoll_response(need_vcoll)
         callback('did_fetch_schema', schema_version, response[select1_id],
-                 response[select2_id],response[select3_id])
+                 response[select2_id], need_vcoll and response[select3_id] or nil)
         set_state('active')
-        return iproto_sm(schema_version)
+        return iproto_sm(schema_version, version_id)
     end
 
-    iproto_sm = function(schema_version)
+    iproto_sm = function(schema_version, version_id)
         local err, hdr, body_rpos, body_end = send_and_recv_iproto()
         if err then return error_sm(err, hdr) end
         dispatch_response_iproto(hdr, body_rpos, body_end)
@@ -794,9 +812,9 @@ local function create_transport(host, port, user, password, callback,
             local body
             body, body_end = decode(body_rpos)
             set_state('fetch_schema')
-            return iproto_schema_sm(schema_version)
+            return iproto_schema_sm(schema_version, version_id)
         end
-        return iproto_sm(schema_version)
+        return iproto_sm(schema_version, version_id)
     end
 
     error_sm = function(err, msg)

>> @@ -1270,16 +1275,28 @@ function remote_methods:_install_schema(schema_version, spaces, indices,
>>                 local pktype = index[PARTS][k][2] or index[PARTS][k].type
>>                 local pkfield = index[PARTS][k][1] or index[PARTS][k].field
>>                 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
>> -                }
>> +                local pk
>> +                if collations ~= nil then
>> +                    pk = {
>> +                        type = pktype,
>> +                        fieldno = pkfield + 1,
>> +                        collation = pkcollation,
>> +                        is_nullable = pknullable
>> +                    }
>> +                else
>> +                    pk = {
>> +                        type = pktype,
>> +                        fieldno = pkfield + 1,
>> +                        collation_id = pkcollationid,
>> +                        is_nullable = pknullable
>> +                    }
>> +                end
> 
> IMHO, it'd be better to not duplicate code and put actual difference
> under if-then-else hammock.
@@ -1270,16 +1288,21 @@ function remote_methods:_install_schema(schema_version, spaces, indices,
                 local pktype = index[PARTS][k][2] or index[PARTS][k].type
                 local pkfield = index[PARTS][k][1] or index[PARTS][k].field
                 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 = pkcollation
+                else
+                    pk.collation_id = pkcollationid
+                end
+

commit abc88ef8d38e21a6c7faf925a19105ce65d45467
Author: Roman Khabibov <roman.habibov@tarantool.org>
Date:   Mon Jul 8 14:52:48 2019 +0300

    net.box: take into account _vcollation presence
    
    The error occured, when a client connects to a server of old
    Tarantool versions, where _vcollation space does not exist. After
    this patch, _vcollation is fetched depending on a server version.
    
    Closes #4307

diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 2892bb3d3..9fe792195 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -685,7 +685,7 @@ local function create_transport(host, port, user, password, callback,
             set_state('active')
             return console_sm(rid)
         elseif greeting.protocol == 'Binary' then
-            return iproto_auth_sm(greeting.salt)
+            return iproto_auth_sm(greeting.salt, greeting.version_id)
         else
             return error_sm(E_NO_CONNECTION,
                             'Unknown protocol: '..greeting.protocol)
@@ -710,11 +710,11 @@ local function create_transport(host, port, user, password, callback,
         end
     end
 
-    iproto_auth_sm = function(salt)
+    iproto_auth_sm = function(salt, version_id)
         set_state('auth')
         if not user or not password then
             set_state('fetch_schema')
-            return iproto_schema_sm()
+            return iproto_schema_sm(nil, version_id)
         end
         encode_auth(send_buf, new_request_id(), user, password, salt)
         local err, hdr, body_rpos, body_end = send_and_recv_iproto()
@@ -727,34 +727,52 @@ local function create_transport(host, port, user, password, callback,
             return error_sm(E_NO_CONNECTION, body[IPROTO_ERROR_KEY])
         end
         set_state('fetch_schema')
-        return iproto_schema_sm(hdr[IPROTO_SCHEMA_VERSION_KEY])
+        return iproto_schema_sm(hdr[IPROTO_SCHEMA_VERSION_KEY], version_id)
     end
 
-    iproto_schema_sm = function(schema_version)
+    iproto_schema_sm = function(schema_version, version_id)
         if not callback('will_fetch_schema') then
             set_state('active')
-            return iproto_sm(schema_version)
+            return iproto_sm(schema_version, version_id)
         end
+        local need_vcoll
+        -- 131584 is tarantool_version_id() for 2.2.0.
+        if version_id ~= nil and version_id >= 131584 then
+            need_vcoll = true
+        else
+            need_vcoll = false
+        end
+
         local select1_id = new_request_id()
         local select2_id = new_request_id()
-        local select3_id = new_request_id()
+        local select3_id = nil
         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 need_vcoll == true then
+            select3_id = new_request_id()
+            -- fetch everything from space _vcollation, 2 = ITER_ALL
+            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
+
+        local function vcoll_response(need_vcoll)
+            if need_vcoll == true then
+                return response[select3_id]
+            end
+            return true
+        end
         repeat
             local err, hdr, body_rpos, body_end = send_and_recv_iproto()
             if err then return error_sm(err, hdr) end
             dispatch_response_iproto(hdr, body_rpos, body_end)
             local id = hdr[IPROTO_SYNC_KEY]
-            if id == select1_id or id == select2_id or id == select3_id then
+            if id == select1_id or id == select2_id or id == select3_id and need_vcoll then
                 -- response to a schema query we've submitted
                 local status = hdr[IPROTO_STATUS_KEY]
                 local response_schema_version = hdr[IPROTO_SCHEMA_VERSION_KEY]
@@ -767,21 +785,21 @@ local function create_transport(host, port, user, password, callback,
                     schema_version = response_schema_version
                 elseif schema_version ~= response_schema_version then
                     -- schema changed while fetching schema; restart loader
-                    return iproto_schema_sm()
+                    return iproto_schema_sm(nil, version_id)
                 end
                 local body
                 body, body_end = decode(body_rpos)
                 response[id] = body[IPROTO_DATA_KEY]
             end
         until response[select1_id] and response[select2_id] and
-              response[select3_id]
+              vcoll_response(need_vcoll)
         callback('did_fetch_schema', schema_version, response[select1_id],
-                 response[select2_id],response[select3_id])
+                 response[select2_id], need_vcoll and response[select3_id] or nil)
         set_state('active')
-        return iproto_sm(schema_version)
+        return iproto_sm(schema_version, version_id)
     end
 
-    iproto_sm = function(schema_version)
+    iproto_sm = function(schema_version, version_id)
         local err, hdr, body_rpos, body_end = send_and_recv_iproto()
         if err then return error_sm(err, hdr) end
         dispatch_response_iproto(hdr, body_rpos, body_end)
@@ -794,9 +812,9 @@ local function create_transport(host, port, user, password, callback,
             local body
             body, body_end = decode(body_rpos)
             set_state('fetch_schema')
-            return iproto_schema_sm(schema_version)
+            return iproto_schema_sm(schema_version, version_id)
         end
-        return iproto_sm(schema_version)
+        return iproto_sm(schema_version, version_id)
     end
 
     error_sm = function(err, msg)
@@ -1270,16 +1288,21 @@ function remote_methods:_install_schema(schema_version, spaces, indices,
                 local pktype = index[PARTS][k][2] or index[PARTS][k].type
                 local pkfield = index[PARTS][k][1] or index[PARTS][k].field
                 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 = pkcollation
+                else
+                    pk.collation_id = pkcollationid
+                end
+
                 idx.parts[k] = pk
             end
             idx.unique = not not index[OPTS].unique

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

[Alexander Turenko]

> > diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> > index 2892bb3d3..723e63045 100644
> > --- a/src/box/lua/net_box.lua
> > +++ b/src/box/lua/net_box.lua
> > @@ -761,6 +761,11 @@ local function create_transport(host, port, user, password, callback,
> >                  if status ~= 0 then
> >                      local body
> >                      body, body_end = decode(body_rpos)
> > +                    local err_msg = 'Space \'277\' does not exist'
> 
> Space 277? Seriously? This code handles not only collations request.
> Should be more generic, IMHO.

Of course, it is better to use VCOLLATION_ID constant that already
defined above.

Kirill, I didn't got your comment. This error message is only about one
specific case: there is no such space. I think this approach is better
then version check (that is proposed in the next version of the patch),
because of readability: the version check approach leads to many small
if's, while here we add only one check.

BTW, Kostja also has a comment:
https://github.com/tarantool/tarantool/commit/47227cfa4c9bac76a882464a2d0440e5137f158e#commitcomment-34342515

WBR, Alexander Turenko.

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

[Alexander Turenko]

On Sat, Jul 20, 2019 at 10:07:14PM +0300, Alexander Turenko wrote:
> > > diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> > > index 2892bb3d3..723e63045 100644
> > > --- a/src/box/lua/net_box.lua
> > > +++ b/src/box/lua/net_box.lua
> > > @@ -761,6 +761,11 @@ local function create_transport(host, port, user, password, callback,
> > >                  if status ~= 0 then
> > >                      local body
> > >                      body, body_end = decode(body_rpos)
> > > +                    local err_msg = 'Space \'277\' does not exist'
> > 
> > Space 277? Seriously? This code handles not only collations request.
> > Should be more generic, IMHO.
> 
> Of course, it is better to use VCOLLATION_ID constant that already
> defined above.
> 
> Kirill, I didn't got your comment. This error message is only about one
> specific case: there is no such space. I think this approach is better
> then version check (that is proposed in the next version of the patch),
> because of readability: the version check approach leads to many small
> if's, while here we add only one check.
> 
> BTW, Kostja also has a comment:
> https://github.com/tarantool/tarantool/commit/47227cfa4c9bac76a882464a2d0440e5137f158e#commitcomment-34342515
> 
> WBR, Alexander Turenko.

We discussed two approaches with Kostja:

* Ignore _vcollation absense on a peer during schema (re)load:
  https://github.com/tarantool/tarantool/commit/47227cfa4c9bac76a882464a2d0440e5137f158e
* Don't fire a request according to a peer tarantool version:
  https://github.com/tarantool/tarantool/commit/abc88ef8d38e21a6c7faf925a19105ce65d45467

Summary of the discussion:

* The former lean on the fact that the only reason of the error re
  absense of _vcollation is an old version of a server. There is no
  guarantee that this assumption will retain in the future.
* The former approach checks an error message against a pattern.
  Changing of the error message will break backward compatibility.
* The latter approach looks bigger then the former one, because of
  passthrough of remote.peer_version_id value and more code with
  branching. We should try to find better way to implement this
  approach.
* Kostja also mentioned possibility to return 'collation' field always,
  even for old peers: just use a table with default collation_id -
  collation (string) mapping. It will not work with non-standard
  collations, but maybe it worth to consider this approach anyway.

To be honest I don't know net.box code enough to propose better way to
implement 2nd approach. Vlad, it would be great if you will look into
this and give some pointers to Roman, if time allows.

CCed Kostja and Vlad.

WBR, Alexander Turenko.

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

[Vladislav Shpilevoy]

Hi! Yeah, I remember that I should explain my vision of how that
task should be solved. See below a long-read.

Here are my explanations on how netbox works, and on how should this
issue be fixed (perhaps).

Netbox for each connection creates a worker fiber and a metadata object
keeping connect timeout, reconnect timeout, status, etc. The metadata
object is known as a netbox connection, and is the object with which a
user works. It is a result of 'netbox.connect()'. Worker fiber does real
work: establishes a tcp connection, sends/receives data.

But there appears a problem of cyclic references. The thing is that the
worker fiber needs some metadata from the netbox object. At least,
'reconnect_timeout'. On the other hand, the netbox object needs some
results from the worker fiber - responses on the requests, connection
status. In other words, both the worker fiber and the netbox object need
something from each other.

If the worker fiber and the netbox object would keep references to each
other, they would never be deleted - Lua GC is not that smart. So there is
a channel of communication between them, which references them both weakly.
https://github.com/tarantool/tarantool/blob/0529e2f0ef7dd4c19283b9e8e6b0e7e43bf400a3/src/box/lua/net_box.lua#L834-L852

This function is called when a netbox object is created. It takes some
mandatory meta like host, port, and a so called 'callback'. It is a closure
defined here:
https://github.com/tarantool/tarantool/blob/0529e2f0ef7dd4c19283b9e8e6b0e7e43bf400a3/src/box/lua/net_box.lua#L910-L954

The closure has access to the netbox object
https://github.com/tarantool/tarantool/blob/0529e2f0ef7dd4c19283b9e8e6b0e7e43bf400a3/src/box/lua/net_box.lua#L909
and returns certain parameters of it. The worker fiber uses a weak reference
to this closure as a proxy to read metadata of the netbox object:
https://github.com/tarantool/tarantool/blob/0529e2f0ef7dd4c19283b9e8e6b0e7e43bf400a3/src/box/lua/net_box.lua#L837-L840.

For example, when the worker fiber needs to obtain reconnect_timeout, it
calls the callback
https://github.com/tarantool/tarantool/blob/0529e2f0ef7dd4c19283b9e8e6b0e7e43bf400a3/src/box/lua/net_box.lua#L443.
The callback here is a weak reference to the closure
https://github.com/tarantool/tarantool/blob/0529e2f0ef7dd4c19283b9e8e6b0e7e43bf400a3/src/box/lua/net_box.lua#L948-L953.

And vice versa - when the worker fiber wants to return something back to
the netbox object, it calls the same callback, but with a different argument:
https://github.com/tarantool/tarantool/blob/0529e2f0ef7dd4c19283b9e8e6b0e7e43bf400a3/src/box/lua/net_box.lua#L399.

How does that callback help to GC the connection? Lets assume, that there are
no references to a netbox object in user code. It is not referenced in the
worker fiber either, because the only reference is callback, and it is weak. So
it is deleted. Together with
https://github.com/tarantool/tarantool/blob/0529e2f0ef7dd4c19283b9e8e6b0e7e43bf400a3/src/box/lua/net_box.lua#L844-L846.
It is a standard way of setting your own callbacks on GC of a table - you create a
dummy cdata object with a custom GC callback. On deletion of that gc-hook a
function
https://github.com/tarantool/tarantool/blob/0529e2f0ef7dd4c19283b9e8e6b0e7e43bf400a3/src/box/lua/net_box.lua#L473-L481
is called. It cancels the worker fiber. Now both the netbox object and the worker
fiber are deleted.

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.

Your case is different from reconnect_timeout and other callback-stuff, because
these parameters are set by a user out of the worker fiber, and can be changed by
him. Callback allows the worker fiber to read actual values always. Version id
originates from the worker fiber, and so can be maintained and kept here.

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.

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.

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.

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

[Alexander Turenko]

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()

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

[Konstantin Osipov]

* Alexander Turenko <alexander.turenko@tarantool.org> [19/07/30 15:04]:

LGTM, Vlad please take another look.


-- 
Konstantin Osipov, Moscow, Russia

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

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

> 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
> @@ -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

Nit: usually we start comments with a capital letter and finish
with a dot. Here and in other hunks. Other than that the patch
LGTM.

>              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]
> 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)
> +#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 \

I like this new test-run feature with '\', looks way better
than setopt delimited, and looks like C.

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

[Alexander Turenko]

Kirill, I have acquired LGTMs from Kostja and Vlad (with the tiny comment
below, which I had fixed). Can you proceed with the change?

https://github.com/tarantool/tarantool/issues/4307
https://github.com/tarantool/tarantool/tree/Totktonada/gh-4307-net-box-fix-schema-fetch

WBR, Alexander Turenko.

> > 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
> > @@ -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
> 
> Nit: usually we start comments with a capital letter and finish
> with a dot. Here and in other hunks. Other than that the patch
> LGTM.

I'm aware of that. I gave glance to comments around and they are
formatted from a small letter and w/o a period, so I formatted the new
code in the same way to make reading more comfortable. Now hovewer I see
that there is no consistency in this point across net_box.lua file. So I
agree with you: it worth to write new comments in our standard way.

Applied an incremental diff at end of the email.

I have applied the diff, rebased the branch upward current master and
force-pushed the branch.

> > +if _TARANTOOL >= '2.2.1' then                    \
> > +    return parts[1].collation == 'test'          \
> > +else                                             \
> > +    return parts[1].collation_id == collation_id \
> 
> I like this new test-run feature with '\', looks way better
> than setopt delimited, and looks like C.

Thanks!

----

diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 31a8c16b7..ea169637d 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -743,7 +743,7 @@ 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
+        -- _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()
@@ -768,8 +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
+            -- 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]
@@ -791,7 +791,7 @@ local function create_transport(host, port, user, password, callback,
             end
         until response[select1_id] and response[select2_id] and
               (not peer_has_vcollation or response[select3_id])
-        -- trick: response[select3_id] is nil when the key is nil
+        -- 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])
         set_state('active')
@@ -1286,8 +1286,8 @@ 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
+                -- Resolve a collation name if a peer has
+                -- _vcollation view.
                 local pkcollation = nil
                 if pkcollationid ~= nil and collations ~= nil then
                     pkcollation = collations[pkcollationid + 1][2]