From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 02F582457C for ; Tue, 30 Jul 2019 06:59:04 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id cpPFXjBQFW3W for ; Tue, 30 Jul 2019 06:59:03 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 8F2232456B for ; Tue, 30 Jul 2019 06:59:03 -0400 (EDT) Date: Tue, 30 Jul 2019 13:58:47 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH] net.box: ignore absence of _vcollation Message-ID: <20190730105847.5bfgdhqqcxoqz7bh@tkn_work_nb> References: <20190708121338.76309-1-roman.habibov@tarantool.org> <20190712095549.ac4hhaxhxzgaik5i@tarantool.org> <20190720190714.5pejxoy2ae43nssp@tkn_work_nb> <20190724212743.jwrti2iuclleogjq@tkn_work_nb> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Vladislav Shpilevoy Cc: Roman Khabibov , tarantool-patches@freelists.org, Konstantin Osipov , Kirill Yukhin 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 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()