From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 03765469710 for ; Tue, 24 Nov 2020 15:11:38 +0300 (MSK) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\)) From: Sergey Ostanevich In-Reply-To: <20201115235416.72858-1-roman.habibov@tarantool.org> Date: Tue, 24 Nov 2020 15:11:36 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <39A73765-A387-4EA2-83DA-4B80534AAAF0@tarantool.org> References: <20201115235416.72858-1-roman.habibov@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] box: check schema version after tarantool update List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Roman Khabibov Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the patch! See my 2 comments inline. Sergos > On 16 Nov 2020, at 02:54, Roman Khabibov = wrote: >=20 > From: Sergey Voinov >=20 > Check schema version (stored in box.space._schema) on start and > print a warning if it doesn't match last avaliable schema version. > It is needed bcause some users forget to call box.schema.upgrade() > after Tarantool update and get stuck with an old schema version > until they encounter some hard to debug problems. >=20 > Closes #4574 > --- >=20 > @ChangeLog: Print log warning if schema version is older than last = available schema version (gh-4574). >=20 > Branch: = https://github.com/tarantool/tarantool/tree/servoin/gh-4574-schema-version= > Issue: https://github.com/tarantool/tarantool/issues/4574 >=20 > src/box/lua/load_cfg.lua | 19 +++++++++++++++ > src/box/lua/upgrade.lua | 36 ++++++++++++++++----------- > test/box/cfg.result | 50 ++++++++++++++++++++++++++++++++++++++ > test/box/cfg.test.lua | 18 ++++++++++++++ > test/box/lua/cfg_test8.lua | 9 +++++++ > test/box/lua/cfg_test9.lua | 9 +++++++ > test/box/stat.result | 16 ++++++------ > 7 files changed, 135 insertions(+), 22 deletions(-) > create mode 100644 test/box/lua/cfg_test8.lua > create mode 100644 test/box/lua/cfg_test9.lua >=20 > diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua > index 76e2e92c2..13088fe73 100644 > --- a/src/box/lua/load_cfg.lua > +++ b/src/box/lua/load_cfg.lua > @@ -702,6 +702,25 @@ local function load_cfg(cfg) > box_configured =3D nil >=20 > box_is_configured =3D true > + > + -- Check if schema version matches Tarantool version > + -- and print warning if it's not (in case user forgot to call = box.schema.upgrade()) 1. The code starting from here=20 > + local version =3D box.space._schema:get{'version'} > + if version ~=3D nil then > + local major =3D version[2] > + local minor =3D version[3] > + local patch =3D version[4] or 0 > + local schema_version =3D string.format("%s.%s.%s", major, = minor, patch) > + local tarantool_version =3D box.info.version to here doesn=E2=80=99t needed, as you do exactly the same inside the = schema_needs_upgrade() > + if private.schema_needs_upgrade() then > + -- Print the warning > + local msg =3D string.format( 1.1 The returning value of schema_needs_upgrade() may contain the = expected upgrade version, so you can reuse it for the message itself. > + 'Your schema version is %s while Tarantool %s = requires a more'.. > + ' recent schema version. Please, consider using '.. > + 'box.schema.upgrade().', schema_version, = tarantool_version) > + log.warn(msg) > + end > + end > end > box.cfg =3D locked(load_cfg) >=20 > diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua > index add791cd7..5ea2fb23d 100644 > --- a/src/box/lua/upgrade.lua > +++ b/src/box/lua/upgrade.lua > @@ -973,6 +973,21 @@ end >=20 > = --------------------------------------------------------------------------= ------ >=20 > +local handlers =3D { > + {version =3D mkversion(1, 7, 6), func =3D upgrade_to_1_7_6, auto = =3D true}, > + {version =3D mkversion(1, 7, 7), func =3D upgrade_to_1_7_7, auto = =3D true}, > + {version =3D mkversion(1, 10, 0), func =3D upgrade_to_1_10_0, = auto =3D true}, > + {version =3D mkversion(1, 10, 2), func =3D upgrade_to_1_10_2, = auto =3D true}, > + {version =3D mkversion(2, 1, 0), func =3D upgrade_to_2_1_0, auto = =3D true}, > + {version =3D mkversion(2, 1, 1), func =3D upgrade_to_2_1_1, auto = =3D true}, > + {version =3D mkversion(2, 1, 2), func =3D upgrade_to_2_1_2, auto = =3D true}, > + {version =3D mkversion(2, 1, 3), func =3D upgrade_to_2_1_3, auto = =3D true}, > + {version =3D mkversion(2, 2, 1), func =3D upgrade_to_2_2_1, auto = =3D true}, > + {version =3D mkversion(2, 3, 0), func =3D upgrade_to_2_3_0, auto = =3D true}, > + {version =3D mkversion(2, 3, 1), func =3D upgrade_to_2_3_1, auto = =3D true}, > +} > + > +-- Schema version of the snapshot. > local function get_version() > local version =3D box.space._schema:get{'version'} > if version =3D=3D nil then > @@ -985,6 +1000,12 @@ local function get_version() > return mkversion(major, minor, patch) > end >=20 > +local function schema_needs_upgrade() > + -- Schema needs upgrade if current schema version is greater > + -- than schema version of the snapshot. > + return handlers[#handlers].version > get_version() 1.2 Update the return value here. > +end > + > local function upgrade(options) > options =3D options or {} > setmetatable(options, {__index =3D {auto =3D false}}) > @@ -995,20 +1016,6 @@ local function upgrade(options) > return > end >=20 > - local handlers =3D { > - {version =3D mkversion(1, 7, 6), func =3D upgrade_to_1_7_6, = auto =3D true}, > - {version =3D mkversion(1, 7, 7), func =3D upgrade_to_1_7_7, = auto =3D true}, > - {version =3D mkversion(1, 10, 0), func =3D upgrade_to_1_10_0, = auto =3D true}, > - {version =3D mkversion(1, 10, 2), func =3D upgrade_to_1_10_2, = auto =3D true}, > - {version =3D mkversion(2, 1, 0), func =3D upgrade_to_2_1_0, = auto =3D true}, > - {version =3D mkversion(2, 1, 1), func =3D upgrade_to_2_1_1, = auto =3D true}, > - {version =3D mkversion(2, 1, 2), func =3D upgrade_to_2_1_2, = auto =3D true}, > - {version =3D mkversion(2, 1, 3), func =3D upgrade_to_2_1_3, = auto =3D true}, > - {version =3D mkversion(2, 2, 1), func =3D upgrade_to_2_2_1, = auto =3D true}, > - {version =3D mkversion(2, 3, 0), func =3D upgrade_to_2_3_0, = auto =3D true}, > - {version =3D mkversion(2, 3, 1), func =3D upgrade_to_2_3_1, = auto =3D true}, > - } > - > for _, handler in ipairs(handlers) do > if version >=3D handler.version then > goto continue > @@ -1047,3 +1054,4 @@ end >=20 > box.schema.upgrade =3D upgrade; > box.internal.bootstrap =3D bootstrap; > +box.internal.schema_needs_upgrade =3D schema_needs_upgrade; > diff --git a/test/box/cfg.result b/test/box/cfg.result > index 4ad3c6493..676324662 100644 > --- a/test/box/cfg.result > +++ b/test/box/cfg.result > @@ -656,3 +656,53 @@ test_run:cmd("cleanup server cfg_tester7") > | --- > | - true > | ... > + > +-- > +-- gh-4574: Check schema version after Tarantool update. > +-- > +test_run:cmd('create server cfg_tester8 with script =3D = "box/lua/cfg_test8.lua", workdir=3D"sql/upgrade/2.1.0/"') > + | --- > + | - true > + | ... > +test_run:cmd("start server cfg_tester8") > + | --- > + | - true > + | ... > +--- Check that the warning is printed. > +version_warning =3D "Please, consider using box.schema.upgrade()." > + | --- > + | ... > +test_run:grep_log('cfg_tester8', version_warning, 1000) ~=3D nil > + | --- > + | - true > + | ... > +test_run:cmd("stop server cfg_tester8") > + | --- > + | - true > + | ... > +test_run:cmd("cleanup server cfg_tester8") > + | --- > + | - true > + | ... > + > +test_run:cmd('create server cfg_tester9 with script =3D = "box/lua/cfg_test9.lua"') > + | --- > + | - true > + | ... > +test_run:cmd("start server cfg_tester9") > + | --- > + | - true > + | ... > +--- Check that the warning isn't printed. > +test_run:grep_log('cfg_tester9', version_warning, 1000) =3D=3D nil > + | --- > + | - true > + | ... > +test_run:cmd("stop server cfg_tester9") > + | --- > + | - true > + | ... > +test_run:cmd("cleanup server cfg_tester9") > + | --- > + | - true > + | ... > diff --git a/test/box/cfg.test.lua b/test/box/cfg.test.lua > index 56018b1a0..015af1a91 100644 > --- a/test/box/cfg.test.lua > +++ b/test/box/cfg.test.lua > @@ -159,3 +159,21 @@ test_run:grep_log('cfg_tester7', 'set = \'replication\' configuration option to', > test_run:grep_log('cfg_tester7', 'test%-cluster%-cookie', 1000) > test_run:cmd("stop server cfg_tester7") > test_run:cmd("cleanup server cfg_tester7") > + > +-- > +-- gh-4574: Check schema version after Tarantool update. > +-- > +test_run:cmd('create server cfg_tester8 with script =3D = "box/lua/cfg_test8.lua", workdir=3D"sql/upgrade/2.1.0/"') > +test_run:cmd("start server cfg_tester8") > +--- Check that the warning is printed. > +version_warning =3D "Please, consider using box.schema.upgrade()." > +test_run:grep_log('cfg_tester8', version_warning, 1000) ~=3D nil > +test_run:cmd("stop server cfg_tester8") > +test_run:cmd("cleanup server cfg_tester8") > + > +test_run:cmd('create server cfg_tester9 with script =3D = "box/lua/cfg_test9.lua=E2=80=9D=E2=80=99) 2. You can reuse the cfg_test1.lua for the writable node, no need in = extra file. > +test_run:cmd("start server cfg_tester9") > +--- Check that the warning isn't printed. > +test_run:grep_log('cfg_tester9', version_warning, 1000) =3D=3D nil > +test_run:cmd("stop server cfg_tester9") > +test_run:cmd("cleanup server cfg_tester9") > diff --git a/test/box/lua/cfg_test8.lua b/test/box/lua/cfg_test8.lua > new file mode 100644 > index 000000000..c61b86ae3 > --- /dev/null > +++ b/test/box/lua/cfg_test8.lua > @@ -0,0 +1,9 @@ > +#!/usr/bin/env tarantool > +os =3D require('os') > + > +box.cfg{ > + listen =3D os.getenv("LISTEN"), > + read_only =3D true > +} > + > +require('console').listen(os.getenv('ADMIN')) > diff --git a/test/box/lua/cfg_test9.lua b/test/box/lua/cfg_test9.lua > new file mode 100644 > index 000000000..fa8b303f1 > --- /dev/null > +++ b/test/box/lua/cfg_test9.lua > @@ -0,0 +1,9 @@ > +#!/usr/bin/env tarantool > +os =3D require('os') > + > +box.cfg{ > + listen =3D os.getenv("LISTEN"), > + read_only =3D false > +} > + > +require('console').listen(os.getenv('ADMIN')) > diff --git a/test/box/stat.result b/test/box/stat.result > index 55f29fe59..e808678eb 100644 > --- a/test/box/stat.result > +++ b/test/box/stat.result > @@ -24,7 +24,7 @@ box.stat.REPLACE.total > ... > box.stat.SELECT.total > --- > -- 1 > +- 3 > ... > box.stat.ERROR.total > --- > @@ -59,7 +59,7 @@ box.stat.REPLACE.total > ... > box.stat.SELECT.total > --- > -- 5 > +- 7 > ... > -- check exceptions > space:get('Impossible value') > @@ -77,14 +77,14 @@ space:get(1) > ... > box.stat.SELECT.total > --- > -- 6 > +- 8 > ... > space:get(11) > --- > ... > box.stat.SELECT.total > --- > -- 7 > +- 9 > ... > space:select(5) > --- > @@ -92,7 +92,7 @@ space:select(5) > ... > box.stat.SELECT.total > --- > -- 8 > +- 10 > ... > space:select(15) > --- > @@ -100,14 +100,14 @@ space:select(15) > ... > box.stat.SELECT.total > --- > -- 9 > +- 11 > ... > for _ in space:pairs() do end > --- > ... > box.stat.SELECT.total > --- > -- 10 > +- 12 > ... > -- reset > box.stat.reset() > @@ -157,7 +157,7 @@ box.stat.REPLACE.total > ... > box.stat.SELECT.total > --- > -- 1 > +- 3 > ... > box.stat.ERROR.total > --- > --=20 > 2.24.3 (Apple Git-128) >=20