[Tarantool-patches] [PATCH] net.box: fix fetching of schema of an old version

[Tarantool-patches] [PATCH] net.box: fix fetching of schema of an old version

[Alexander Turenko]

After 2.2.0-633-gaa0964ae1 ('net.box: fix schema fetching from 1.10/2.1
servers') net.box expects that _vcollation system view exists on a
tarantool server of 2.2.1+ version. This is however not always so: a
server may be run on a new version of tarantool, but work on a schema of
an old version.

The situation with non last schema is usual for replication cluster in
process of upgrading: all instances run on the new version of tarantool
first (no auto-upgrade is performed by tarantools in a cluster). Then
box.schema.upgrade() should be called, but the instances should be
operable even before the call.

Before the commit net.box was unable to connect a server if it is run on
a schema without _vcollation system view (say, 2.1.3), but the server
executable is of 2.2.1 version or newer.

Follows up #4307
Fixes #4691
---

https://github.com/tarantool/tarantool/issues/4691
https://github.com/tarantool/tarantool/tree/Totktonada/gh-4691-net-box-connect-schema-2-1-3

 src/box/lua/net_box.lua                       |   9 ++
 ...4691-net-box-connect-schema-2-1-3.test.lua |  93 ++++++++++++++++++
 test/box-tap/no_auto_schema_upgrade.lua       |  30 ++++++
 .../snap/2.1.3/00000000000000000000.snap      | Bin 0 -> 4466 bytes
 4 files changed, 132 insertions(+)
 create mode 100755 test/box-tap/gh-4691-net-box-connect-schema-2-1-3.test.lua
 create mode 100644 test/box-tap/no_auto_schema_upgrade.lua
 create mode 100644 test/box-tap/snap/2.1.3/00000000000000000000.snap

diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 3f611c027..d3ccc0fc6 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -53,6 +53,7 @@ local E_UNKNOWN              = box.error.UNKNOWN
 local E_NO_CONNECTION        = box.error.NO_CONNECTION
 local E_TIMEOUT              = box.error.TIMEOUT
 local E_PROC_LUA             = box.error.PROC_LUA
+local E_NO_SUCH_SPACE        = box.error.NO_SUCH_SPACE
 
 -- utility tables
 local is_final_state         = {closed = 1, error = 1}
@@ -779,6 +780,13 @@ local function create_transport(host, port, user, password, callback,
                 local status = hdr[IPROTO_STATUS_KEY]
                 local response_schema_version = hdr[IPROTO_SCHEMA_VERSION_KEY]
                 if status ~= 0 then
+                    -- No _vcollation space (server has an old
+                    -- schema version).
+                    local errno = band(status, IPROTO_ERRNO_MASK)
+                    if id == select3_id and errno == E_NO_SUCH_SPACE then
+                        peer_has_vcollation = false
+                        goto continue
+                    end
                     local body
                     body, body_end = decode(body_rpos)
                     return error_sm(E_NO_CONNECTION, body[IPROTO_ERROR_KEY])
@@ -793,6 +801,7 @@ local function create_transport(host, port, user, password, callback,
                 body, body_end = decode(body_rpos)
                 response[id] = body[IPROTO_DATA_KEY]
             end
+            ::continue::
         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
diff --git a/test/box-tap/gh-4691-net-box-connect-schema-2-1-3.test.lua b/test/box-tap/gh-4691-net-box-connect-schema-2-1-3.test.lua
new file mode 100755
index 000000000..565f7da7b
--- /dev/null
+++ b/test/box-tap/gh-4691-net-box-connect-schema-2-1-3.test.lua
@@ -0,0 +1,93 @@
+#!/usr/bin/env tarantool
+
+--
+-- gh-4691: net.box fails to connect to tarantool-2.2+ server with
+-- a schema of version 2.1.3 or below (w/o _vcollation system
+-- space).
+--
+-- Tarantool does not update a schema automatically when an
+-- instance is in a replication cluster, because it may break
+-- instances that are run under an old tarantool version. It is
+-- quite usual to have non-upgraded instances for some time,
+-- because upgrade is usually performed step-by-step.
+--
+-- net.box leans on a server version from greeting to determine
+-- whether _vcollation system view should exist and reports an
+-- error if fetching of the space fails. This causes the problem:
+-- a version may be 2.2+, but _vcollation does not exists, because
+-- schema upgrade is not performed yet. The fix of gh-4691 allows
+-- the server to respond ER_NO_SUCH_SPACE for the query.
+--
+
+local test_run = require('test_run').new()
+local net_box = require('net.box')
+local tap = require('tap')
+
+local function before_all()
+    local opts = {
+        'script = "box-tap/no_auto_schema_upgrade.lua"',
+        'workdir = "box-tap/snap/2.1.3"',
+        'return_listen_uri = True',
+    }
+    local opts_str = table.concat(opts, ', ')
+    local cmd = 'create server schema_2_1_3 with %s'
+    local uri = test_run:cmd(cmd:format(opts_str))
+    test_run:cmd('start server schema_2_1_3')
+
+    -- Create 'test' space with 'unicode_ci' index part.
+    --
+    -- We need it to verify that net.box will expose collation_id
+    -- for an index key part when collation names information is
+    -- not available.
+    --
+    -- Note: read_only = false on reconfiguration does not lead to
+    -- a schema upgrading.
+    test_run:eval('schema_2_1_3', ([[
+        box.cfg{read_only = false}
+        box.schema.create_space('test')
+        box.space.test:create_index('pk', {parts =
+            {{field = 1, type = 'string', collation = 'unicode_ci'}}})
+        box.cfg{read_only = true}
+    ]]):gsub('\n', ' '))
+    return uri
+end
+
+local function test_connect_schema_2_1_3(test, uri)
+    test:plan(3)
+
+    local connection = net_box.connect(uri)
+
+    -- Connection is alive.
+    test:ok(connection:is_connected(), 'connection is alive')
+
+    -- Space metainfo is correct: collation_id is exposed when
+    -- collation name information is not available.
+    local key_part = connection.space.test.index[0].parts[1]
+    test:is(key_part.collation, nil,
+        'collation names are not available')
+    test:is(type(key_part.collation_id), 'number',
+        'collation numeric ids are exposed')
+
+    connection:close()
+end
+
+local function after_all()
+    -- Drop 'test' space.
+    test_run:eval('schema_2_1_3', ([[
+        box.cfg{read_only = false}
+        box.space.test:drop()
+        box.cfg{read_only = true}
+    ]]):gsub('\n', ' '))
+
+    test_run:cmd('stop server schema_2_1_3')
+    test_run:cmd('cleanup server schema_2_1_3')
+    test_run:cmd('delete server schema_2_1_3')
+end
+
+local uri = before_all()
+local test = tap.test('gh-4691-net-box-connect-schema-2-1-3')
+test:plan(1)
+test:test('connect_schema_2_1_3', test_connect_schema_2_1_3, uri)
+after_all()
+
+os.exit(test:check() and 0 or 1)
diff --git a/test/box-tap/no_auto_schema_upgrade.lua b/test/box-tap/no_auto_schema_upgrade.lua
new file mode 100644
index 000000000..b693637ac
--- /dev/null
+++ b/test/box-tap/no_auto_schema_upgrade.lua
@@ -0,0 +1,30 @@
+#!/usr/bin/env tarantool
+
+--
+-- This instance is intended to be used with existing snapshot
+-- from a previous tarantool version. It does not perform
+-- automatic schema upgrade.
+--
+-- Having tarantool in this state allows us to create test cases
+-- for net.box, relay, applier connected to an instance in such
+-- state or test behaviour of the instance itself.
+--
+-- There are two ways to disable automatic schema upgrade: set
+-- 'replication' box.cfg(<...>) option or set 'read_only' option.
+-- Use the latter one, because it is simpler.
+--
+
+box.cfg({
+    listen = os.getenv('LISTEN'),
+    read_only = true,
+})
+
+-- Give 'guest' user read/write accesses to all spaces.
+--
+-- Note: reconfiguration with read_only = false does not lead to
+-- schema upgrading.
+box.cfg{read_only = false}
+box.schema.user.grant('guest', 'read, write', 'universe')
+box.cfg{read_only = true}
+
+require('console').listen(os.getenv('ADMIN'))
diff --git a/test/box-tap/snap/2.1.3/00000000000000000000.snap b/test/box-tap/snap/2.1.3/00000000000000000000.snap
new file mode 100644
index 0000000000000000000000000000000000000000..c8eecc0b4cc6b7bc0ee8634b3d51081662331da7
GIT binary patch
literal 4466
zcmV-&5smIsPC-x#FfK7O3RY!ub7^mGIv_GGF)lMLFfC^@IAk|uHe)qp3Q2BrbYX5|
zWjY{XVm320F*!LcWn(ruEi^D=Gc7qXF*YqSH8o{6F=jb9Vq`N4RzqxWV{1AfdwmKD
z)w&D1%@Fzk&U17FP^AC>0000ewJ-euP>qKG%9u1FMi7Wt+W-In0001RFet(YFVtK2
zf0sfAlW(K}05f&SC}m1UnJL<qD>qlp#0`(NKMrfvA;5%fHx*vF9_)X%KG)6BV<!?E
z0vZAX0%X>5EyrZ=%@KovY>ovN;GC*}otzV93`v3!Ly})CAlnMUpgS*MWIGDJpqjkx
zx>WFOaq*(bEfi={j^5JJMUz>$XmWxrn$%>AMGHw@$wHD=CLqbS3YPz=T>0P0%KvRs
zl@E^uR3dO1asNKY!JF%^N8CRM`iy5v69f|E{6GXbJ1~Xk83a5tJ-{>1QXI&6X_m7R
zQ_gLYl+f?KrO@wWN}(Tbq2I-lK0HXD3{L|xF-|ctPI#ikzO5r9*%kv0oLmG~@QM{M
zk6I_^Vcy(SK`?JYiGs%kj__#203PvJ1cj!+0yJ6zMl>1%MgEBb-XD;F_XZ$<#TNDi
zi>;Fbuvmq~wuu3s77<LQ#l(pY?X)UdLrr`EMBWp?#CxHk9aO;H0}!yc075woASj1G
z1LQD=O9ZIZlmt+#B>}3{ewKKTM2YuDl6Y_Ar}rXAdM9$EcL0o}>5w96DuhUy;x-5X
zACMvA=3@3(vL8Z7Tov;miNV%K5L(AuhdCc0sqU&yiy)lW51*Gyiq^6IJ^w57mpk|G
zuh6_1Q$@?|=8=nfue$5Q=jD<*^M*_qCofv4<Zj-4Te+xUKt4V%mz2+aZLWRKtd5P(
z%O$nv_C^hw!D;ULyj)UYR{D}=VbW3(&PO;&2_NAZPCAl;M9z+qKJk;ql#-rU(ew8!
zD(Y;opX+kz`AH)}6<<C$S)jsc2_G+4ns%zdLS~1GD_B2g%xULj?jXo|J5yNUy%be=
z9|aY@g<^_N>j^1jT24fv`?w3(y;c)YbkD>S-Rp6p`|f*9KzSY$P<{zLP#(%F(epkD
zp7%&B;r$UxcyB}!-WP#{Z;3eKyOYNV*_|PZ5cTNebWx8WsQn!wh|oTWJlX>>g!bD1
zAVl^)1d)9YKxCgi4?kd!#|P~1^ng9~TRiY?2M_Oc?(oh#oh?+|!49hGPzO~}Rf`T*
zbJzi^b9BHeR^@1QzRRKWc24J=$B~ortX<0Cv?)(HR-CIW?zaw(-#5R@VQt*i-d4K3
zZ@F++;lhoBRk*|b=Js!>`rif|{;%Opr{RX)XRx7n8EQb?(14;b%z&b8WH?hQH&Zd3
z>6x-=!?J6rSlblKs$ug#3~SmKY-m;Mf|`xC0K-OEcwwU(UBLwvT9EBhV1aj{w-eo=
zu)=qO3cRy5AqC#qmVm-X!U?>clnFe3-p(|<5<WCM5;#A@lj5Btq~U!cpy8co8^Rgh
zKKBX22<Z8N2<TZ00X@-^2Euuo`QSX;sM=X|wk!=lt!D7kkHSy*$+KSf)oZBlwb!aI
zHSeX~P^B()s?to9)68?zOq%(WuBJv;(>zymHOnW{B(-T~(=^+$G)<8-%`lrS9&`3U
z%oz-G?f`!~4m^<UHUOird5gm4PJ<3ivkV8ONoE7nY%|bcEiovwmKOwWSP;%7YVz?2
z?k*FejWf!5h>Emqide+`j=#s5!CR-dp3D5V)~We>D>pl@P}zf6cZf+;)~u&_9SiEN
z(9p+e=VY$LD2e-f<svwuS(8~?mpkX$fVmq~fQr+b|0=h_B1dy7?f1N+IIfCnbNw-j
zauI}cfe6Tx(Ga8Px<RkA@`_>>N89)&Ma$={+`50COJh6ZGV{}%;&7Tx(Sy4Cb9=aV
zjLWFHe%$uxZOpsWey>+qrG2YWgIUC?zsf1zR%zDUQG@ErdwGS%l~Hb<B1S{3{4<(I
zaLm1DGLIli+nNRI+<SX%U_n7Z11u3>HET)&NCNb;B%i#0k3`8w=SUJ69V0)QwzW#9
z2$FA$$Pw8V07iDQ*e46itszCeIzouZg52KinKe1sR4R1{)&Vlato;yT)_eppYi7*{
z$oIOvAF_LUK4>)8?fHCrcvLW;+mr3Vqk;k5UQ|@8hwJvfT^@U6J3RKlQ8yg5yF(Ao
z+K~rm>%fDvoTcLq&CX$mX62|uvzm>Aj=sV%M_=ENqpy8+BaU@#1CDiN<Bhdl!;Mc%
zjW%RjX|TattIAaijWzl|Lyi8;NTdJ$%Ru9QGS2va3^V?3#VF(dFv$471!Ih=_6sr0
z??o8q?*a_-o1cp>{@=oj|F!7i|27LQ@W)~c{IAdgfBdb;;tSsxGR?@?QdUxu9WBLV
zv!hQ$N;;Y?iTP+YizR(aA%0@TwZWR8ny8mI`Rv!0bcV)GqbHaqGOtki0=hlii@bX!
zuU>Zw#&mmo_UzGf#~@esJ+peRSE7%X>rTO#!k*RD<8BiyYE;cV>-u<*QQoaYjfSh5
zyRRu2((P@Xju>}u2UAMYRB0Y{;(|ql0;WukqIvm9Dwn9tnrsg-Z2BV=n<5sPp8aa|
zeO^o4bxpyLIB*)%<#su_b(Eg{8YPMu9`11O6s%~2J8EDX%UoSDVlvo*!Oe0(mNB<U
zOp^t(Ff%Z-FSgu^sR9?17a!XiCJJI*Vq9WdkhCCWL7Kn_i_sM|MplZd6j4!A<OFN7
z!6a}Jfz8PjA_#~cAacO;*km#rVj7Y%v?Qb?lqB@MUO}>JQ!~U3iUlFv3)BEm6qij+
zp1eZoz2?y5+`pCU=k};zPL8rrF2cd1x`LZyZH{G%AE#bt?cA}7HKN)S-TbX_|9*23
zJV|hOM+V0HanC!Kpx1G-*2uYHpx|uzV{xzjgIIUV0&B`r_YYp(EJmvhyt++AtnTVe
zfik0-)m_EHC>DJV3;be!3VBw}{BPV#9jm(v;x1DHQ$71NhJg83Jk_&bLEJ@p!baRH
z$%46Sbin$vXCv;F1aY^>04f&LT^Oum6{l}hX7ac-n7dA#1cpqg^V6pmqLBdr004p@
z0Am|S5SUwLNd^-DfI-56!Ll(t77U~qhK%)?WoiHhK!Ab(MF1s_M3TO>36Mxr)#z#z
zHHsQVjoM+;&N272)=$D7`8Oy9LjUOtHM$tleexDdQwqg3#v*KHwWkF5g<DeCqDzRW
zXC8S8+0qX0M(6M}jSmYBj8c8M46tuLzc0UHNsZ4F$)#>K^c+&pqkzb3Qu3QD4H%)+
zCu({*vMS33C?C)=r?o+fqi!Nq+}UNa$RtQwztE)+EQ)rCbN1FS+bDjZmfs3{eq4X>
zW}`s1F9Vkqd}FrFqVrTaTU;@U>GJfr;uP0f9<Pggq`+BS%k$n}j2NCQID>tZDQ<Rk
zk$=G~7bNiLaTz;K8Z6IEH)1ft%#+cHt!K2R%h}_KQ%skq#|5YSuAClkoZpkM_=^im
z@9@uNvK4D!Ns8GygXsv3&qg48uqi}^;0gnrXI=AE#o-OD)>@!1sf3s*s9JPxLiA1n
zBezWzcljz1FGX){8APQS2i`daP6&=zX@|^Z=LPUylQRJ4L$L-dN&^<kZEYhs!aHEw
zC4n)udvy7P6pXYGzT+wcGuXDbI7u@g3_fEKeraY~V`whM`X^-!!|tZ|e@v)JPY4RA
z(Ly(wqZj2@CwyLv@X-pBds}E5|DxDvNAfJSu(Lu<{=}7*b&_aH;$il11A=XraB6ml
zp5sVb$?@nC83sbFV}fmA<JRxM(|7V{(_ZsHTd|>cgnINT&y^f@^L8kzY%A#aBFh9B
ziap_#V!}u0lZpmBxIJY1wFV3SqEATyRV|sjI9?Z;Zbyv=n?)-@6Yt!Cvtb)2aN=8`
z?D$`_^W&T3b$w7x{EOPVBHd_`bAs6}QZY97!rDfjfQ6_Wb!~Bxj%};X6!T%J=Wf+@
z2vz2Ox8EqT0J@?)Uhrsm69-^(bX|gK=|)$JP47QMB|s9fd2yp>f$W8f59NgZyveCZ
z!-v7TnzmZjn;b{P2=2J|$jDnY0PfDdMEFj=_v6zCg`necKkhTb*MDgjX}<n7z!q<~
zAnRmCiQ@Yz2%Tf}b4`Zm&nOvs_CoDhcNQ{J1}M=z^JoddLhCQZ_+6Y8RSr|9xpgzk
ztDh9VEytq~<}iUmbr4jmkMhbDYwCJZA6Zt|sVPXXC`cQH1{2A`ccQ(iK<ey4)45FW
z7DeC7(=xHVS{0=v`VgODZXyioauvL|Vgd4#<~xE-l$bGcC&W?mH+ZB*#2Bq1B4wzp
z=6R3<qT|B`6X*d%NYYEuM!$mDflhDy^kO_V9$%x`Jfy|T{4!X&Fhl%fw&AhmEgqqW
zn4&R!<P56xbSXg-><A=VYd7Iq3Q88<7EF~ro@8Z~99OCbu_HPjG!TdmDF#wqlvcbO
zbp0*9#JxPCfik=?dZf&%G|b1nmIjPWrB{D1tp{4AA9wyRDvM{iy!>Ht!b^FOt5TSu
z!<)p1jFH`zwJHU02#zW2AkDb!8j2!MaQ=QGVglml$Sosq#qgU6UZUx>Be;vwvqs>e
zfG+di0!cb&N2ZL45es(Cj-D^H%AOq|^>l9lo=Nu&vX4Un5IJcJ<R>ZKG(S2EaKYF#
zjqsJ}L+pr;0UJ!92M}Q)7?Lpd_|zbZeT1F}%8-Ydz^TSjMj+Hi$%X_jO2!(15kjE(
z!#PAi%D~nr05J-#lm3ta(t|-G0q785C;&qe#vYLxM6r+1Ga(uBFcWyyILZu!+9=zO
z94VQkGD8eK0@MJMh*!}no1TQeBi%~rKJo1*<VQq!O8M6)j#7rtlD9XYfqc(W$?0~2
zZ>iQ3-h6@%G~^5zRSBaedyd>}2ecpcK9V`ky^Re(bx{VW-2`$s6ZCwgG3Obk#OyO|
zP^r>>kUi3)K_h|akYXTXXx5}KrtVN#FysIPMr8X=q*zJ0h>Ahyd_8+>FOepRLCg?}
z5eU^`*X&tMRz78SkO>}1A*IFW=q-%N4JBX1F!R&n3=j=OZDro3zeOofKX#6d#FvY7
zspFQE$NP?SNQ9vw9u`Ul@u(3jAp$KJpGy)M4Kv2Sgc?o(6lLutT{$JPp_$p3O=VjM
z+^K>IukB?+d)g#R6o?AmPx1~7gH6*vaRF2j_>>?r8fJ`mi6PJm(6c0w(J*8DOALWl
zfSzPrSW&tXGj&C}NX~)^#fkXpugg#s+1lI;D0t_~l`@-%3Eh5@Ez*!UH$7(J9b^|}
z(m@$fQV=i1tj*C!QpE;q#`;6p5m!hyy*xBdgmKK)o%~J}F`YP5@)DJz0G;h!tukxA
z)uolXv2EJ=^X>}@wh@=QOgLvSQMb*aVON<{j5RM+1e)7<VFbPTFwTW*a8K+NLd(KT
zZI_JZapQTy0xjV0@deAd(1JK29ATdpkcYli?-Trx+E*@g2>5$z;81z*@=WQQEVaPF
zdiv2$L6vPTD_Ny#6>RLI7$R(czt$HF?|Jh)(Eu$NpUJqeqI4yCjY?f~+0$bLyxy3b
zqh_OllDws_(($lU6uo5=MDb=vDbv{=yfYh4$z*bPZrx(8QXvgjl?xN4^`H0Wewwd_
zA#j3*q63iXH|ZQztp<Up7&K89l^LH)qC}aHj9PBn#1$3j3QWH$^DV993WlRQ$yb9@
zr3p607KtkCZj53ym}KY3D#$~mnFe=s42-^eoMD^gr%*z<zNGb<JGV&ns}SGJLIqd>
zYhT4~+g>z;=Z*7316trcCT;bq#s&zwSF)Z^=$SI<*EZiw=$SI<*EZiw=$X><Bt1OY
zi)d2D73&(CPK{95-J^G9-olOyRBWR>4CObB0t?7~dtaWO1*<;d!(RvbN#bKDAJq`8
E?c6t5<^TWy

literal 0
HcmV?d00001

-- 
2.25.0

Re: [Tarantool-patches] [PATCH] net.box: fix fetching of schema of an old version

[Alexander Turenko]

On Wed, Apr 01, 2020 at 11:45:21PM +0300, Alexander Turenko wrote:
> After 2.2.0-633-gaa0964ae1 ('net.box: fix schema fetching from 1.10/2.1
> servers') net.box expects that _vcollation system view exists on a
> tarantool server of 2.2.1+ version. This is however not always so: a
> server may be run on a new version of tarantool, but work on a schema of
> an old version.
> 
> The situation with non last schema is usual for replication cluster in
> process of upgrading: all instances run on the new version of tarantool
> first (no auto-upgrade is performed by tarantools in a cluster). Then
> box.schema.upgrade() should be called, but the instances should be
> operable even before the call.
> 
> Before the commit net.box was unable to connect a server if it is run on
> a schema without _vcollation system view (say, 2.1.3), but the server
> executable is of 2.2.1 version or newer.
> 
> Follows up #4307
> Fixes #4691
> ---
> 
> https://github.com/tarantool/tarantool/issues/4691
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-4691-net-box-connect-schema-2-1-3

Forgot to add the changelog entry.

```
## Bugs fixed

### Lua

* net.box: fixed inability to connect 2.2.1+ server before
  box.schema.upgrade() is called on the server (gh-4691)
```

Re: [Tarantool-patches] [PATCH] net.box: fix fetching of schema of an old version

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

On 01/04/2020 22:45, Alexander Turenko wrote:
> After 2.2.0-633-gaa0964ae1 ('net.box: fix schema fetching from 1.10/2.1
> servers') net.box expects that _vcollation system view exists on a
> tarantool server of 2.2.1+ version. This is however not always so: a
> server may be run on a new version of tarantool, but work on a schema of
> an old version.
> 
> The situation with non last schema is usual for replication cluster in
> process of upgrading: all instances run on the new version of tarantool
> first (no auto-upgrade is performed by tarantools in a cluster). Then
> box.schema.upgrade() should be called, but the instances should be
> operable even before the call.
> 
> Before the commit net.box was unable to connect a server if it is run on
> a schema without _vcollation system view (say, 2.1.3), but the server
> executable is of 2.2.1 version or newer.
> 
> Follows up #4307
> Fixes #4691
> ---
> 
> https://github.com/tarantool/tarantool/issues/4691
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-4691-net-box-connect-schema-2-1-3
> 
>  src/box/lua/net_box.lua                       |   9 ++
>  ...4691-net-box-connect-schema-2-1-3.test.lua |  93 ++++++++++++++++++
>  test/box-tap/no_auto_schema_upgrade.lua       |  30 ++++++
>  .../snap/2.1.3/00000000000000000000.snap      | Bin 0 -> 4466 bytes
>  4 files changed, 132 insertions(+)
>  create mode 100755 test/box-tap/gh-4691-net-box-connect-schema-2-1-3.test.lua

We have xlog/upgrade suite specifically for these tests. With
already designed processes, where and how to store old snaps, etc.
See xlog/upgrade/how_to_add_new_test.md.

Re: [Tarantool-patches] [PATCH] net.box: fix fetching of schema of an old version

[Alexander Turenko]

On Sat, Apr 04, 2020 at 12:53:53AM +0200, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> On 01/04/2020 22:45, Alexander Turenko wrote:
> > After 2.2.0-633-gaa0964ae1 ('net.box: fix schema fetching from 1.10/2.1
> > servers') net.box expects that _vcollation system view exists on a
> > tarantool server of 2.2.1+ version. This is however not always so: a
> > server may be run on a new version of tarantool, but work on a schema of
> > an old version.
> > 
> > The situation with non last schema is usual for replication cluster in
> > process of upgrading: all instances run on the new version of tarantool
> > first (no auto-upgrade is performed by tarantools in a cluster). Then
> > box.schema.upgrade() should be called, but the instances should be
> > operable even before the call.
> > 
> > Before the commit net.box was unable to connect a server if it is run on
> > a schema without _vcollation system view (say, 2.1.3), but the server
> > executable is of 2.2.1 version or newer.
> > 
> > Follows up #4307
> > Fixes #4691
> > ---
> > 
> > https://github.com/tarantool/tarantool/issues/4691
> > https://github.com/tarantool/tarantool/tree/Totktonada/gh-4691-net-box-connect-schema-2-1-3
> > 
> >  src/box/lua/net_box.lua                       |   9 ++
> >  ...4691-net-box-connect-schema-2-1-3.test.lua |  93 ++++++++++++++++++
> >  test/box-tap/no_auto_schema_upgrade.lua       |  30 ++++++
> >  .../snap/2.1.3/00000000000000000000.snap      | Bin 0 -> 4466 bytes
> >  4 files changed, 132 insertions(+)
> >  create mode 100755 test/box-tap/gh-4691-net-box-connect-schema-2-1-3.test.lua
> 
> We have xlog/upgrade suite specifically for these tests. With
> already designed processes, where and how to store old snaps, etc.
> See xlog/upgrade/how_to_add_new_test.md.

Not exactly.

I want to test the case when an instance is NOT upgraded. So 'upgrade'
directory name for snaps would be misleading.

I also think that we should move from using compare-against-output
tests. I'll not insist on this during review, but personally I will
create new tests using pytest-like (phpunit-like, maven-failsafe-like,
luatest-like) way (it is our TAP13 tests or at least near to them). I
can summarize cons and pros if you want (don't sure whether I did it
already somewhere).

We can move 'snap' directory upward (right into test/) and use it for
snapshots for all test suites. Or keep them per-suite. However see the
next paragraph re fill.lua.

I prefer to feed initial schema and data right from a test to keep them
near to actual usage in test cases (within one file). Sometimes it eases
reading of test cases, especially when you're not much familiar with a
testing system. I don't much like the approach with fill.lua script (at
least for data corresponds to test cases).

WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH] net.box: fix fetching of schema of an old version

[Vladislav Shpilevoy]

>>> https://github.com/tarantool/tarantool/issues/4691
>>> https://github.com/tarantool/tarantool/tree/Totktonada/gh-4691-net-box-connect-schema-2-1-3
>>>
>>>  src/box/lua/net_box.lua                       |   9 ++
>>>  ...4691-net-box-connect-schema-2-1-3.test.lua |  93 ++++++++++++++++++
>>>  test/box-tap/no_auto_schema_upgrade.lua       |  30 ++++++
>>>  .../snap/2.1.3/00000000000000000000.snap      | Bin 0 -> 4466 bytes
>>>  4 files changed, 132 insertions(+)
>>>  create mode 100755 test/box-tap/gh-4691-net-box-connect-schema-2-1-3.test.lua
>>
>> We have xlog/upgrade suite specifically for these tests. With
>> already designed processes, where and how to store old snaps, etc.
>> See xlog/upgrade/how_to_add_new_test.md.
> 
> 
> I want to test the case when an instance is NOT upgraded. So 'upgrade'
> directory name for snaps would be misleading.

The test and the bug are still upgrade related. 'xlog/upgrade' is not
called 'xlog/upgrade_called'. It is called just 'upgrade'. For all cases
when an old snapshot is used to start a new tarantool.

> I also think that we should move from using compare-against-output
> tests.

This is not about tap vs diff. It is about having all upgrade related
tests in one place, and organized in one way, with how to fill them, with
how to store xlogs and snapshots in the repository.

> I'll not insist on this during review, but personally I will
> create new tests using pytest-like (phpunit-like, maven-failsafe-like,
> luatest-like) way (it is our TAP13 tests or at least near to them). I
> can summarize cons and pros if you want (don't sure whether I did it
> already somewhere).

Don't really want to discuss this again, everything was said already
1000 times or more.

> We can move 'snap' directory upward (right into test/) and use it for
> snapshots for all test suites. Or keep them per-suite. However see the
> next paragraph re fill.lua.

This is a bad idea, because different upgrade-related tests usually
need some specific data stored in the old snapshot. If you merge data,
needed by many tests, into one snapshot, you will violate the policy of
one-bug-one-test. Because many tests will start to depend on each other.

> I prefer to feed initial schema and data right from a test to keep them
> near to actual usage in test cases (within one file).

This makes impossible to test bugs, which appear only when you bootstrap
from the old snapshot.

> Sometimes it eases
> reading of test cases, especially when you're not much familiar with a
> testing system.

You are now creating a second way of testing upgrade. This is definitely
not a simplification of the testing system.

> I don't much like the approach with fill.lua script (at
> least for data corresponds to test cases).

fill.lua is inevitable, when you need to test bootstrap from an old
snapshot having some data in it before you start tarantool, and you want
to know what data is stored here exactly.

Re: [Tarantool-patches] [PATCH] net.box: fix fetching of schema of an old version

[Alexander Turenko]

On Sat, Apr 04, 2020 at 07:40:44PM +0200, Vladislav Shpilevoy wrote:
> >>> https://github.com/tarantool/tarantool/issues/4691
> >>> https://github.com/tarantool/tarantool/tree/Totktonada/gh-4691-net-box-connect-schema-2-1-3
> >>>
> >>>  src/box/lua/net_box.lua                       |   9 ++
> >>>  ...4691-net-box-connect-schema-2-1-3.test.lua |  93 ++++++++++++++++++
> >>>  test/box-tap/no_auto_schema_upgrade.lua       |  30 ++++++
> >>>  .../snap/2.1.3/00000000000000000000.snap      | Bin 0 -> 4466 bytes
> >>>  4 files changed, 132 insertions(+)
> >>>  create mode 100755 test/box-tap/gh-4691-net-box-connect-schema-2-1-3.test.lua
> >>
> >> We have xlog/upgrade suite specifically for these tests. With
> >> already designed processes, where and how to store old snaps, etc.
> >> See xlog/upgrade/how_to_add_new_test.md.
> > 
> > 
> > I want to test the case when an instance is NOT upgraded. So 'upgrade'
> > directory name for snaps would be misleading.
> 
> The test and the bug are still upgrade related. 'xlog/upgrade' is not
> called 'xlog/upgrade_called'. It is called just 'upgrade'. For all cases
> when an old snapshot is used to start a new tarantool.

I disagree here. 'upgrade' is not something about working upward an old
snapshot.

> 
> > I also think that we should move from using compare-against-output
> > tests.
> 
> This is not about tap vs diff. It is about having all upgrade related
> tests in one place, and organized in one way, with how to fill them, with
> how to store xlogs and snapshots in the repository.

But I still want to test it using TAP13 test. test/xlog is the test
suite that is fed to the console.

> 
> > I'll not insist on this during review, but personally I will
> > create new tests using pytest-like (phpunit-like, maven-failsafe-like,
> > luatest-like) way (it is our TAP13 tests or at least near to them). I
> > can summarize cons and pros if you want (don't sure whether I did it
> > already somewhere).
> 
> Don't really want to discuss this again, everything was said already
> 1000 times or more.

I'm too.

> 
> > We can move 'snap' directory upward (right into test/) and use it for
> > snapshots for all test suites. Or keep them per-suite. However see the
> > next paragraph re fill.lua.
> 
> This is a bad idea, because different upgrade-related tests usually
> need some specific data stored in the old snapshot. If you merge data,
> needed by many tests, into one snapshot, you will violate the policy of
> one-bug-one-test. Because many tests will start to depend on each other.

Ouch, okay, fill.lua is called on the old tarantool version (on which
you generate a snapshot). It is okay for my case to disable autoupgrade
and insert the data using the new tarantool version. I guess it is okay
too for most of cases. Your case with upgrade of a sequence can be
tested this way too.

Inserting necessary data right from a test seems to be more sane
approach: you don't need to jump over several files to look at the test
case at whole: its data and code.

This way also splits snapshot from test data. The snapshot contains just
what appears after box.cfg() + box.snapshot() on an old tarantool
version. No surprises.

> 
> > I prefer to feed initial schema and data right from a test to keep them
> > near to actual usage in test cases (within one file).
> 
> This makes impossible to test bugs, which appear only when you bootstrap
> from the old snapshot.

Any example? Your case with a sequence upgrade can be tested with
inserting data using the new tarantool version.

We just need to start tarantool of the new version on the old snapshot
in the read-only mode to disable autoupgrading, enable read-write,
insert necessary data. Then we can either test the instance on the old
schema or call upgrade and test it after upgrade.

> 
> > Sometimes it eases
> > reading of test cases, especially when you're not much familiar with a
> > testing system.
> 
> You are now creating a second way of testing upgrade. This is definitely
> not a simplification of the testing system.

Nope. Again, I don't test upgrade here.

But, yep, I want to enforce several properties and so I'm unable to use
the way you proposed to run tarantool on an old schema:

* A test is the script with TAP13 output, so it should be inside
  a corresponding test suite.
* A test inserts all needed data right from the test file. The instance
  file just calls box.cfg() and give guest user accesses. It is highly
  reusable so.

> 
> > I don't much like the approach with fill.lua script (at
> > least for data corresponds to test cases).
> 
> fill.lua is inevitable, when you need to test bootstrap from an old
> snapshot having some data in it before you start tarantool, and you want
> to know what data is stored here exactly.

I would call for examples.

But anyway, we can keep both ways and use fill.lua when it is not
possible to feed initial data for a test using the new tarantool
version.

Re: [Tarantool-patches] [PATCH] net.box: fix fetching of schema of an old version

[Vladislav Shpilevoy]

On 05/04/2020 14:05, Alexander Turenko wrote:
> On Sat, Apr 04, 2020 at 07:40:44PM +0200, Vladislav Shpilevoy wrote:
>>>>> https://github.com/tarantool/tarantool/issues/4691
>>>>> https://github.com/tarantool/tarantool/tree/Totktonada/gh-4691-net-box-connect-schema-2-1-3
>>>>>
>>>>>  src/box/lua/net_box.lua                       |   9 ++
>>>>>  ...4691-net-box-connect-schema-2-1-3.test.lua |  93 ++++++++++++++++++
>>>>>  test/box-tap/no_auto_schema_upgrade.lua       |  30 ++++++
>>>>>  .../snap/2.1.3/00000000000000000000.snap      | Bin 0 -> 4466 bytes
>>>>>  4 files changed, 132 insertions(+)
>>>>>  create mode 100755 test/box-tap/gh-4691-net-box-connect-schema-2-1-3.test.lua
>>>>
>>>> We have xlog/upgrade suite specifically for these tests. With
>>>> already designed processes, where and how to store old snaps, etc.
>>>> See xlog/upgrade/how_to_add_new_test.md.
>>>
>>>
>>> I want to test the case when an instance is NOT upgraded. So 'upgrade'
>>> directory name for snaps would be misleading.
>>
>> The test and the bug are still upgrade related. 'xlog/upgrade' is not
>> called 'xlog/upgrade_called'. It is called just 'upgrade'. For all cases
>> when an old snapshot is used to start a new tarantool.
> 
> I disagree here. 'upgrade' is not something about working upward an old
> snapshot.

It is. Because you won't work on the old snapshot forever. You are
going to upgrade anyway. Your bug is for when upgrade is started but
not finished. Because Tarantools are new, but upgrade() is not called
yet.

>>> I also think that we should move from using compare-against-output
>>> tests.
>>
>> This is not about tap vs diff. It is about having all upgrade related
>> tests in one place, and organized in one way, with how to fill them, with
>> how to store xlogs and snapshots in the repository.
> 
> But I still want to test it using TAP13 test. test/xlog is the test
> suite that is fed to the console.

Well, I also want many things different, but also I don't want to break
what works, and I want to be consistent. Currently this patch basically
proposes to throw away the just started to work xlog/upgrade suite, which
was disabled for a long time. And it makes upgrade tests inconsistent.

>>> We can move 'snap' directory upward (right into test/) and use it for
>>> snapshots for all test suites. Or keep them per-suite. However see the
>>> next paragraph re fill.lua.
>>
>> This is a bad idea, because different upgrade-related tests usually
>> need some specific data stored in the old snapshot. If you merge data,
>> needed by many tests, into one snapshot, you will violate the policy of
>> one-bug-one-test. Because many tests will start to depend on each other.
> 
> Ouch, okay, fill.lua is called on the old tarantool version (on which
> you generate a snapshot). It is okay for my case to disable autoupgrade
> and insert the data using the new tarantool version. I guess it is okay
> too for most of cases. Your case with upgrade of a sequence can be
> tested this way too.

No, it can't. Unless you tried and actually managed to reproduce this.
Because I just tried your way. I made checkout to 2.1, took an empty
snapshot (00000000000000000000.snap). Took 2.2 before the sequence fix
(that commit: 13de917a0e3b8affb693d041663c174d9ec7fcc9). Then I started
Tarantool in read-only mode:

    box.cfg{read_only = true}

Then I check the upgrade is not called:

    tarantool> box.space._schema:select{}
    ---
    - - ['cluster', '9990ec84-1f65-4452-aa39-3c06c1aac709']
      - ['max_id', 511]
      - ['version', 2, 1, 3]
    ...

    tarantool> _TARANTOOL
    ---
    - 2.2.2-40-g13de917a0
    ...

Then I made it read_only false to be able to create a sequence:

    tarantool> box.cfg{read_only = false}
    2020-04-05 18:14:27.256 [23941] main/102/interactive I> set 'read_only' configuration option to false
    ---
    ...

Then I created a space with sequence, like described in the ticket:
https://github.com/tarantool/tarantool/issues/4771

    tarantool> box.schema.create_space('s')
    ---
    - engine: memtx
      ... -- snipped
    - created
    ...

    tarantool> box.space.s:create_index('pk', {parts = {{1, 'integer'}}, sequence = true})
    ---
    - parts:
      ... -- snipped
      name: pk
    ...

Then I called upgrade:

    tarantool> box.schema.upgrade()
    2020-04-05 18:15:54.580 [23941] main/102/interactive I> add sequence field to space _space_sequence
    2020-04-05 18:15:54.581 [23941] main/102/interactive I> create space _ck_constraint
    2020-04-05 18:15:54.581 [23941] main/102/interactive I> create index primary on _ck_constraint
    2020-04-05 18:15:54.582 [23941] main/102/interactive I> create view _vcollation...
    2020-04-05 18:15:54.583 [23941] main/102/interactive I> create index primary on _vcollation
    2020-04-05 18:15:54.583 [23941] main/102/interactive I> create index name on _vcollation
    2020-04-05 18:15:54.583 [23941] main/102/interactive I> grant read access to 'public' role for _vcollation view
    2020-04-05 18:15:54.584 [23941] main/102/interactive I> Update _func format
    2020-04-05 18:15:54.602 [23941] main/102/interactive I> Create _func_index space
    2020-04-05 18:15:54.603 [23941] main/102/interactive I> set schema version to 2.2.1
    ---
    ...

So even though in your particular case it is ok, it is very
artificial, and can't be applied to all tests.

> Inserting necessary data right from a test seems to be more sane
> approach: you don't need to jump over several files to look at the test
> case at whole: its data and code.

It may look simpler (even though for me, honestly, the test in the
patch does not look simple). But it does not mean anything it is right.
As I said, I consider that test too artificial.

> This way also splits snapshot from test data. The snapshot contains just
> what appears after box.cfg() + box.snapshot() on an old tarantool
> version. No surprises.

For having test data we use fill.lua. I don't see any surprises here.
What you put into fill.lua, you will see after bootstrap.

>>> I prefer to feed initial schema and data right from a test to keep them
>>> near to actual usage in test cases (within one file).
>>
>> This makes impossible to test bugs, which appear only when you bootstrap
>> from the old snapshot.
> 
> Any example? Your case with a sequence upgrade can be tested with
> inserting data using the new tarantool version.

As I already showed above, it can't. At least without manual change
of _sequence/_space_sequence/_sequence_data (but this I didn't check),
which would make the test artificial, not real.

So if you want examples, first is this:
https://github.com/tarantool/tarantool/issues/4771

Second is this:
https://github.com/tarantool/tarantool/issues/2950

The problem was that space formats started being validated, and you
couldn't insert bad data even if you didn't call upgrade yet. Because
alter.cc already parsed existing formats, and turned on format validation.
Idea is to insert bad data on the old version, and bootstrap a new
version on that. From what I remember, fix was to add a check in
alter.cc that wouldn't turn on validation in case you have an old record
in _schema.

> We just need to start tarantool of the new version on the old snapshot
> in the read-only mode to disable autoupgrading, enable read-write,
> insert necessary data. Then we can either test the instance on the old
> schema or call upgrade and test it after upgrade.

This does not work, in case your 'necessary data' will be inserted
or validated differently due to alter.cc or any other internal changes.

>>> Sometimes it eases
>>> reading of test cases, especially when you're not much familiar with a
>>> testing system.
>>
>> You are now creating a second way of testing upgrade. This is definitely
>> not a simplification of the testing system.
> 
> Nope. Again, I don't test upgrade here.
> 
> But, yep, I want to enforce several properties and so I'm unable to use
> the way you proposed to run tarantool on an old schema:
> 
> * A test is the script with TAP13 output, so it should be inside
>   a corresponding test suite.

Tap or diff has nothing to do with that. It is not a blocker.

> * A test inserts all needed data right from the test file. The instance
>   file just calls box.cfg() and give guest user accesses. It is highly
>   reusable so.

This is also not a blocker to write a test on this issue.

On the summary, all 'blockers' are subjective, and don't really
block anything. It is the same as I would refuse to patch luajit code,
because I personally prefer Tarantool's code style.

>>> I don't much like the approach with fill.lua script (at
>>> least for data corresponds to test cases).
>>
>> fill.lua is inevitable, when you need to test bootstrap from an old
>> snapshot having some data in it before you start tarantool, and you want
>> to know what data is stored here exactly.
> 
> I would call for examples.

See two examples above.

> But anyway, we can keep both ways and use fill.lua when it is not
> possible to feed initial data for a test using the new tarantool
> version.

Okey, you can omit fill.lua, but I want upgrade-related tests in
one place. Or organized in one way in maximum two places, if you are
so desperate to use tap.

Re: [Tarantool-patches] [PATCH] net.box: fix fetching of schema of an old version

[Alexander Turenko]

> >> The test and the bug are still upgrade related. 'xlog/upgrade' is not
> >> called 'xlog/upgrade_called'. It is called just 'upgrade'. For all cases
> >> when an old snapshot is used to start a new tarantool.
> > 
> > I disagree here. 'upgrade' is not something about working upward an old
> > snapshot.
> 
> It is. Because you won't work on the old snapshot forever. You are
> going to upgrade anyway. Your bug is for when upgrade is started but
> not finished. Because Tarantools are new, but upgrade() is not called
> yet.

It is like "sooner or later a user will use feature X after Y, so let's
call Y as X". I understood your point, but still think that the name
'upgrade' is misleading. More general 'snap' or 'snapshots' looks better
for me.

In fact upgrade may break old connectors (say, due to unicode_ci
collation of _func on 2.2) and I guess the old schema may be kept for
quite long time so.

> > Ouch, okay, fill.lua is called on the old tarantool version (on which
> > you generate a snapshot). It is okay for my case to disable autoupgrade
> > and insert the data using the new tarantool version. I guess it is okay
> > too for most of cases. Your case with upgrade of a sequence can be
> > tested this way too.
> 
> No, it can't. Unless you tried and actually managed to reproduce this.
> Because I just tried your way. I made checkout to 2.1, took an empty
> snapshot (00000000000000000000.snap). Took 2.2 before the sequence fix
> (that commit: 13de917a0e3b8affb693d041663c174d9ec7fcc9). Then I started
> Tarantool in read-only mode:
>
> <...snipped steps to reproduce...>
> 
> So even though in your particular case it is ok, it is very
> artificial, and can't be applied to all tests.

You're right. I missed that we'll unable to reproduce the problem if
we'll call fixed upgrade script (from a new tarantool version).

> Okey, you can omit fill.lua, but I want upgrade-related tests in
> one place. Or organized in one way in maximum two places, if you are
> so desperate to use tap.

I'll rework the patch to keep snapshots together. Don't sure about
feeding initial data in my case (from fill.lua or from a test): I'll
took a time to think around.

It seems logical to move snapshots out from test suites, since they will
be used across the test suites. What do you think?

Re: [Tarantool-patches] [PATCH] net.box: fix fetching of schema of an old version

[Vladislav Shpilevoy]

>>> Ouch, okay, fill.lua is called on the old tarantool version (on which
>>> you generate a snapshot). It is okay for my case to disable autoupgrade
>>> and insert the data using the new tarantool version. I guess it is okay
>>> too for most of cases. Your case with upgrade of a sequence can be
>>> tested this way too.
>>
>> No, it can't. Unless you tried and actually managed to reproduce this.
>> Because I just tried your way. I made checkout to 2.1, took an empty
>> snapshot (00000000000000000000.snap). Took 2.2 before the sequence fix
>> (that commit: 13de917a0e3b8affb693d041663c174d9ec7fcc9). Then I started
>> Tarantool in read-only mode:
>>
>> <...snipped steps to reproduce...>
>>
>> So even though in your particular case it is ok, it is very
>> artificial, and can't be applied to all tests.
> 
> You're right. I missed that we'll unable to reproduce the problem if
> we'll call fixed upgrade script (from a new tarantool version).
> 
>> Okey, you can omit fill.lua, but I want upgrade-related tests in
>> one place. Or organized in one way in maximum two places, if you are
>> so desperate to use tap.
> 
> I'll rework the patch to keep snapshots together. Don't sure about
> feeding initial data in my case (from fill.lua or from a test): I'll
> took a time to think around.

I am ok with feeding data however you want, suitable for a particular
ticket. But what I consider important is having such tests organized in
one way. With the same folder structure, file naming, isolation.

Although your snapshot is 'pure'. Zero snapshot with no data. Probably
such snapshot could be shared by multiple tests via a symbolic link, if
needed.

> It seems logical to move snapshots out from test suites, since they will
> be used across the test suites. What do you think?

It is not always the case. A snapshot is ok to use across tests only if
it is not related to any particular test more than to others. Only empty
snapshots satisfy that criteria. The reason is that we are obligated to
keep tests isolated from each other. And if you use one non-empty snapshot
for multiple tests, the policy will be violated.

I don't like doing file-per-bug tests, but for snapshots it is fair. Because
otherwise such a snapshot quickly becomes a pile of unmaintainable garbage,
like xlog/upgrade snapshots were beforehand.

Re: [Tarantool-patches] [PATCH] net.box: fix fetching of schema of an old version

[Alexander Turenko]

On Mon, Apr 06, 2020 at 02:39:37PM +0300, Alexander Turenko wrote:
> > >> The test and the bug are still upgrade related. 'xlog/upgrade' is not
> > >> called 'xlog/upgrade_called'. It is called just 'upgrade'. For all cases
> > >> when an old snapshot is used to start a new tarantool.
> > > 
> > > I disagree here. 'upgrade' is not something about working upward an old
> > > snapshot.
> > 
> > It is. Because you won't work on the old snapshot forever. You are
> > going to upgrade anyway. Your bug is for when upgrade is started but
> > not finished. Because Tarantools are new, but upgrade() is not called
> > yet.
> 
> It is like "sooner or later a user will use feature X after Y, so let's
> call Y as X". I understood your point, but still think that the name
> 'upgrade' is misleading. More general 'snap' or 'snapshots' looks better
> for me.
> 
> In fact upgrade may break old connectors (say, due to unicode_ci
> collation of _func on 2.2) and I guess the old schema may be kept for
> quite long time so.

We should agree on some name for directory(-ies) with snapshots for
testing purposes.

I think that stored snapshots may be used to test various scenarious:

* Start from a snapshot with old schema and data, then call upgrade and
  test a scenario.
* Start from a snapshot with old schema and some data, then test a
  scenario.
* Start from a snapshot that is broken by a past upgrade (see #4804),
  then test a scenario.
* Start from a broken snapshot and test a scenario:
  - A snapshot that violates some constraint (and so is hard to be
    generated on demand), see #4797.
  - A snapshot that has broken metainformation: instance uuid, vclock.

So I would prefer a name that is more general then 'upgrade': 'snap',
'snapshots'.

Aren't this convince you?

WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH] net.box: fix fetching of schema of an old version

[Alexander Turenko]

> > It seems logical to move snapshots out from test suites, since they will
> > be used across the test suites. What do you think?
> 
> It is not always the case. A snapshot is ok to use across tests only if
> it is not related to any particular test more than to others. Only empty
> snapshots satisfy that criteria. The reason is that we are obligated to
> keep tests isolated from each other. And if you use one non-empty snapshot
> for multiple tests, the policy will be violated.

(Continued snap/upgrade discussion in the sibling email thread.)

How about this directory structure?

 test/
 |
 +- snap/
    |
    +- how_to_add_new_test.md
    |
    +- xlog/
    |  |
    |  +- gh-4771-upgrade-sequence/
    |     |
    |     +- fill.lua
    |     +- 2.1.3/
    |        |
    |        +- *.snap
    |
    +- box-tap/
       |
       +- gh-4691-net-box-connect-schema-2-1-3/
          |
          +- fill.lua
          +- 2.1.3/
             |
             +- *.snap

Key points:

* All snapshots are in one place: it is easier to keep them right
  structured.
* Snapshots are organized on per-test basis.

Alternative:

 test/
 |
 +- xlog/
 |  |
 |  +- snap/
 |     |
 |     +- how_to_add_new_test.md
 |     |
 |     +- gh-4771-upgrade-sequence/
 |        |
 |        +- fill.lua
 |        +- 2.1.3/
 |           |
 |           +- *.snap
 |
 +- box-tap/
    |
    +- snap/
       |
       +- how_to_add_new_test.md -> ../../xlog/snap/how_to_add_new_test.md
       |
       +- gh-4691-net-box-connect-schema-2-1-3/
          |
          +- fill.lua
          +- 2.1.3/
             |
             +- *.snap

Key points:

* All snapshots organized as same structured subtrees.
* Same as above, snapshots are organized on per-test basis.

WBR, Alexander Turenko.

Re: [Tarantool-patches] [PATCH] net.box: fix fetching of schema of an old version

[Vladislav Shpilevoy]

On 19/04/2020 18:22, Alexander Turenko wrote:
> On Mon, Apr 06, 2020 at 02:39:37PM +0300, Alexander Turenko wrote:
>>>>> The test and the bug are still upgrade related. 'xlog/upgrade' is not
>>>>> called 'xlog/upgrade_called'. It is called just 'upgrade'. For all cases
>>>>> when an old snapshot is used to start a new tarantool.
>>>>
>>>> I disagree here. 'upgrade' is not something about working upward an old
>>>> snapshot.
>>>
>>> It is. Because you won't work on the old snapshot forever. You are
>>> going to upgrade anyway. Your bug is for when upgrade is started but
>>> not finished. Because Tarantools are new, but upgrade() is not called
>>> yet.
>>
>> It is like "sooner or later a user will use feature X after Y, so let's
>> call Y as X". I understood your point, but still think that the name
>> 'upgrade' is misleading. More general 'snap' or 'snapshots' looks better
>> for me.
>>
>> In fact upgrade may break old connectors (say, due to unicode_ci
>> collation of _func on 2.2) and I guess the old schema may be kept for
>> quite long time so.
> 
> We should agree on some name for directory(-ies) with snapshots for
> testing purposes.
> 
> I think that stored snapshots may be used to test various scenarious:
> 
> * Start from a snapshot with old schema and data, then call upgrade and
>   test a scenario.
> * Start from a snapshot with old schema and some data, then test a
>   scenario.
> * Start from a snapshot that is broken by a past upgrade (see #4804),
>   then test a scenario.
> * Start from a broken snapshot and test a scenario:
>   - A snapshot that violates some constraint (and so is hard to be
>     generated on demand), see #4797.
>   - A snapshot that has broken metainformation: instance uuid, vclock.
> 
> So I would prefer a name that is more general then 'upgrade': 'snap',
> 'snapshots'.
> 
> Aren't this convince you?
> 
> WBR, Alexander Turenko.

Snap looks ok. If it will be used consistently and everywhere.

Re: [Tarantool-patches] [PATCH] net.box: fix fetching of schema of an old version

[Vladislav Shpilevoy]

On 19/04/2020 18:23, Alexander Turenko wrote:
>>> It seems logical to move snapshots out from test suites, since they will
>>> be used across the test suites. What do you think?
>>
>> It is not always the case. A snapshot is ok to use across tests only if
>> it is not related to any particular test more than to others. Only empty
>> snapshots satisfy that criteria. The reason is that we are obligated to
>> keep tests isolated from each other. And if you use one non-empty snapshot
>> for multiple tests, the policy will be violated.
> 
> (Continued snap/upgrade discussion in the sibling email thread.)
> 
> How about this directory structure?
> 
>  test/
>  |
>  +- snap/
>     |
>     +- how_to_add_new_test.md
>     |
>     +- xlog/
>     |  |
>     |  +- gh-4771-upgrade-sequence/
>     |     |
>     |     +- fill.lua
>     |     +- 2.1.3/
>     |        |
>     |        +- *.snap
>     |
>     +- box-tap/
>        |
>        +- gh-4691-net-box-connect-schema-2-1-3/
>           |
>           +- fill.lua
>           +- 2.1.3/
>              |
>              +- *.snap
> 
> Key points:
> 
> * All snapshots are in one place: it is easier to keep them right
>   structured.
> * Snapshots are organized on per-test basis.
> 
> Alternative:
> 
>  test/
>  |
>  +- xlog/
>  |  |
>  |  +- snap/
>  |     |
>  |     +- how_to_add_new_test.md
>  |     |
>  |     +- gh-4771-upgrade-sequence/
>  |        |
>  |        +- fill.lua
>  |        +- 2.1.3/
>  |           |
>  |           +- *.snap
>  |
>  +- box-tap/
>     |
>     +- snap/
>        |
>        +- how_to_add_new_test.md -> ../../xlog/snap/how_to_add_new_test.md
>        |
>        +- gh-4691-net-box-connect-schema-2-1-3/
>           |
>           +- fill.lua
>           +- 2.1.3/
>              |
>              +- *.snap
> 
> Key points:
> 
> * All snapshots organized as same structured subtrees.
> * Same as above, snapshots are organized on per-test basis.
> 
> WBR, Alexander Turenko.

Both options look good to me.

Re: [Tarantool-patches] [PATCH] net.box: fix fetching of schema of an old version

[Alexander Turenko]

On Wed, Apr 01, 2020 at 11:45:21PM +0300, Alexander Turenko wrote:
> After 2.2.0-633-gaa0964ae1 ('net.box: fix schema fetching from 1.10/2.1
> servers') net.box expects that _vcollation system view exists on a
> tarantool server of 2.2.1+ version. This is however not always so: a
> server may be run on a new version of tarantool, but work on a schema of
> an old version.
> 
> The situation with non last schema is usual for replication cluster in
> process of upgrading: all instances run on the new version of tarantool
> first (no auto-upgrade is performed by tarantools in a cluster). Then
> box.schema.upgrade() should be called, but the instances should be
> operable even before the call.
> 
> Before the commit net.box was unable to connect a server if it is run on
> a schema without _vcollation system view (say, 2.1.3), but the server
> executable is of 2.2.1 version or newer.
> 
> Follows up #4307
> Fixes #4691
> ---
> 
> https://github.com/tarantool/tarantool/issues/4691
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-4691-net-box-connect-schema-2-1-3

Pushed to Totktonada/gh-4691-net-box-connect-schema-2-1-3-no-test
without tests to include into 2.4.1.

CCed Kirill.