From: Sergey Ostanevich <sergos@tarantool.org> To: Roman Khabibov <roman.habibov@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH] box: check schema version after tarantool update Date: Tue, 24 Nov 2020 15:11:36 +0300 [thread overview] Message-ID: <39A73765-A387-4EA2-83DA-4B80534AAAF0@tarantool.org> (raw) In-Reply-To: <20201115235416.72858-1-roman.habibov@tarantool.org> Hi! Thanks for the patch! See my 2 comments inline. Sergos > On 16 Nov 2020, at 02:54, Roman Khabibov <roman.habibov@tarantool.org> wrote: > > From: Sergey Voinov <sergeiv@tarantool.org> > > 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. > > Closes #4574 > --- > > @ChangeLog: Print log warning if schema version is older than last available schema version (gh-4574). > > Branch: https://github.com/tarantool/tarantool/tree/servoin/gh-4574-schema-version > Issue: https://github.com/tarantool/tarantool/issues/4574 > > 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 > > 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 = nil > > box_is_configured = 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 > + local version = box.space._schema:get{'version'} > + if version ~= nil then > + local major = version[2] > + local minor = version[3] > + local patch = version[4] or 0 > + local schema_version = string.format("%s.%s.%s", major, minor, patch) > + local tarantool_version = box.info.version to here doesn’t needed, as you do exactly the same inside the schema_needs_upgrade() > + if private.schema_needs_upgrade() then > + -- Print the warning > + local msg = 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 = locked(load_cfg) > > 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 > > -------------------------------------------------------------------------------- > > +local handlers = { > + {version = mkversion(1, 7, 6), func = upgrade_to_1_7_6, auto = true}, > + {version = mkversion(1, 7, 7), func = upgrade_to_1_7_7, auto = true}, > + {version = mkversion(1, 10, 0), func = upgrade_to_1_10_0, auto = true}, > + {version = mkversion(1, 10, 2), func = upgrade_to_1_10_2, auto = true}, > + {version = mkversion(2, 1, 0), func = upgrade_to_2_1_0, auto = true}, > + {version = mkversion(2, 1, 1), func = upgrade_to_2_1_1, auto = true}, > + {version = mkversion(2, 1, 2), func = upgrade_to_2_1_2, auto = true}, > + {version = mkversion(2, 1, 3), func = upgrade_to_2_1_3, auto = true}, > + {version = mkversion(2, 2, 1), func = upgrade_to_2_2_1, auto = true}, > + {version = mkversion(2, 3, 0), func = upgrade_to_2_3_0, auto = true}, > + {version = mkversion(2, 3, 1), func = upgrade_to_2_3_1, auto = true}, > +} > + > +-- Schema version of the snapshot. > local function get_version() > local version = box.space._schema:get{'version'} > if version == nil then > @@ -985,6 +1000,12 @@ local function get_version() > return mkversion(major, minor, patch) > end > > +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 = options or {} > setmetatable(options, {__index = {auto = false}}) > @@ -995,20 +1016,6 @@ local function upgrade(options) > return > end > > - local handlers = { > - {version = mkversion(1, 7, 6), func = upgrade_to_1_7_6, auto = true}, > - {version = mkversion(1, 7, 7), func = upgrade_to_1_7_7, auto = true}, > - {version = mkversion(1, 10, 0), func = upgrade_to_1_10_0, auto = true}, > - {version = mkversion(1, 10, 2), func = upgrade_to_1_10_2, auto = true}, > - {version = mkversion(2, 1, 0), func = upgrade_to_2_1_0, auto = true}, > - {version = mkversion(2, 1, 1), func = upgrade_to_2_1_1, auto = true}, > - {version = mkversion(2, 1, 2), func = upgrade_to_2_1_2, auto = true}, > - {version = mkversion(2, 1, 3), func = upgrade_to_2_1_3, auto = true}, > - {version = mkversion(2, 2, 1), func = upgrade_to_2_2_1, auto = true}, > - {version = mkversion(2, 3, 0), func = upgrade_to_2_3_0, auto = true}, > - {version = mkversion(2, 3, 1), func = upgrade_to_2_3_1, auto = true}, > - } > - > for _, handler in ipairs(handlers) do > if version >= handler.version then > goto continue > @@ -1047,3 +1054,4 @@ end > > box.schema.upgrade = upgrade; > box.internal.bootstrap = bootstrap; > +box.internal.schema_needs_upgrade = 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 = "box/lua/cfg_test8.lua", workdir="sql/upgrade/2.1.0/"') > + | --- > + | - true > + | ... > +test_run:cmd("start server cfg_tester8") > + | --- > + | - true > + | ... > +--- Check that the warning is printed. > +version_warning = "Please, consider using box.schema.upgrade()." > + | --- > + | ... > +test_run:grep_log('cfg_tester8', version_warning, 1000) ~= 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 = "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) == 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 = "box/lua/cfg_test8.lua", workdir="sql/upgrade/2.1.0/"') > +test_run:cmd("start server cfg_tester8") > +--- Check that the warning is printed. > +version_warning = "Please, consider using box.schema.upgrade()." > +test_run:grep_log('cfg_tester8', version_warning, 1000) ~= nil > +test_run:cmd("stop server cfg_tester8") > +test_run:cmd("cleanup server cfg_tester8") > + > +test_run:cmd('create server cfg_tester9 with script = "box/lua/cfg_test9.lua”’) 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) == 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 = require('os') > + > +box.cfg{ > + listen = os.getenv("LISTEN"), > + read_only = 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 = require('os') > + > +box.cfg{ > + listen = os.getenv("LISTEN"), > + read_only = 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 > --- > -- > 2.24.3 (Apple Git-128) >
next prev parent reply other threads:[~2020-11-24 12:11 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-11-15 23:54 Roman Khabibov 2020-11-24 12:11 ` Sergey Ostanevich [this message] 2020-11-24 12:21 ` Sergey Ostanevich 2020-11-24 18:53 ` Roman Khabibov 2020-11-30 11:00 ` Sergey Ostanevich 2020-11-30 13:43 ` Roman Khabibov 2020-12-01 9:58 ` Serge Petrenko 2020-12-02 0:16 ` Roman Khabibov 2020-12-02 9:17 ` Serge Petrenko 2020-12-02 15:25 ` roman 2020-12-03 12:19 ` Kirill Yukhin 2020-12-03 12:21 ` Roman Khabibov 2020-12-03 12:40 ` Alexander V. Tikhonov 2020-12-03 12:56 ` Kirill Yukhin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=39A73765-A387-4EA2-83DA-4B80534AAAF0@tarantool.org \ --to=sergos@tarantool.org \ --cc=roman.habibov@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH] box: check schema version after tarantool update' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox