From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 99C806EC40; Fri, 13 Aug 2021 02:20:27 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 99C806EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1628810427; bh=zBxLAgbaylge1HvK8+QZiTOwpxqwly7Cqvdhxpk5OVo=; h=To:References:Date:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=GOeHW7K6/SH5FeeZnMq8WWlLBin9RbhhEwC4V7HinxNCwXijYv9XCvJAjr4gnwXAh hBofj19D07LlyLHouDjg9UgCTywnN5sR4brdnl3ePdMw7PEuzaqfGtIc03754PPKQA Dl2+Mr561T/0EqKWlblD/kBmGddYOFdUThBR9Dfs= Received: from smtp59.i.mail.ru (smtp59.i.mail.ru [217.69.128.39]) (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 75BCF6EC40 for ; Fri, 13 Aug 2021 02:20:26 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 75BCF6EC40 Received: by smtp59.i.mail.ru with esmtpa (envelope-from ) id 1mEJzt-0007XD-8F; Fri, 13 Aug 2021 02:20:25 +0300 To: Vladimir Davydov References: <20210810112836.48775-1-sergepetrenko@tarantool.org> <20210811110654.y3xyaojebckp6flq@esperanza> Message-ID: <90eeefb5-3719-3198-dec7-4f00834733e7@tarantool.org> Date: Fri, 13 Aug 2021 02:20:23 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: <20210811110654.y3xyaojebckp6flq@esperanza> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD910164DC12A5633065676A9727AC27C74182A05F5380850405D142DDAD78D0045C910219A4DDBBC973B96B4F9AF9733A56E40463AE465FEF2 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE78981306C6E927004EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637CDA089757FB31C668638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D835C160F4C1325394CDC626E472A2E665117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCF1175FABE1C0F9B6A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18CB629EEF1311BF91D2E47CDBA5A96583BA9C0B312567BB231DD303D21008E29813377AFFFEAFD269A417C69337E82CC2E827F84554CEF50127C277FBC8AE2E8BA83251EDC214901ED5E8D9A59859A8B6C94F115956DE4A7A089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A213B5FB47DCBC3458834459D11680B505AF2A145F032A48668F7B43F4C59C130D X-C1DE0DAB: 0D63561A33F958A53F8303FD2013365C88F1A58F2E4FBD829B204323BABF1CF6D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA750E14360347543F58410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34F6110710A6CC527F25C862C3A50B1FF196F9BB92912D33796B0735E98ACB4FDEC055275C716CF0831D7E09C32AA3244CF5796557B38E66B1939BEDBADA10ADBF35DA7DC5AF9B58C0FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj0dLV0c3jbkxjoXG2RTl5VQ== X-Mailru-Sender: 3B9A0136629DC9125D61937A2360A4468B0F3DFCCB335137CFF56B0EBD112312AD9BA18F9B960D2D424AE0EB1F3D1D21E2978F233C3FAE6EE63DB1732555E4A8EE80603BA4A5B0BC112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH] box: allow upgrading from version 1.6 X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Serge Petrenko via Tarantool-patches Reply-To: Serge Petrenko Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 11.08.2021 14:06, Vladimir Davydov пишет: > On Tue, Aug 10, 2021 at 02:28:36PM +0300, Serge Petrenko wrote: >> Direct upgrade support from pre-1.7.5 versions was removed in commit >> 7d3b80e78206c479cab75b4893629cfa1932252e >> (Forbid upgrade from Tarantool < 1.7.5 and refactor upgrade.lua) >> The reason for that was the mandatory space format checks introduced >> back then. Later on, more incompatible changes were made, and due to these >> changes, Tarantool failed to recover from pre-1.7.5 snapshots. >> >> Actually this is rather inconvenient. Now one has to perform an extra >> upgrade step when upgrading from, say, 1.6 to 2.x: instead of performing a >> direct upgrade one has to do 1.6 -> 1.10 -> 2.x upgrade which takes >> twice the time. >> >> Make it possible to boot from snapshots coming from Tarantool version 1.6.8 and >> above. >> >> In order to do so, introduce before_replace triggers on system spaces, >> which work during snapshot/xlog recovery. The triggers will set tuple >> formats to the ones supported by current Tarantool (2.x). This way the >> recovered data will have the correct format for a usual schema upgrade. >> >> Also add upgrade_to_1_7_5() handler, which finishes transformation of >> old schema to 1.7.5. The handler is fired together with other >> box.schema.upgrade() handlers, so there's no user-visible behaviour >> change. >> >> Side note: it would be great to use the same technique to allow booting >> from pre-1.6.8 snapshots. Unfortunately, this is not possible. >> >> Current triggers don't break the order of schema upgrades, so 1.7.1 >> upgrades come before 1.7.2 and 1.7.5. This is because all the upgrades >> in these versions are replacing existing tuples and not inserting new >> ones, so the upgrades may be handled by the before_replace triggers. >> >> Upgrade to 1.6.8 requires inserting new tuples: creating sysviews, like >> _vspace, _vuser and so on. This can't be done from the before_replace >> triggers, so we would have to run triggers for 1.7.x first which would >> allow Tarantool to recover the snapshot, and then run an upgrade handler for >> 1.6.8. This looks really messy. >> >> Closes #5894 >> --- >> https://github.com/tarantool/tarantool/issues/5894 >> https://github.com/tarantool/tarantool/tree/sp/gh-5894-1.6-upgrade >> src/box/lua/load_cfg.lua | 9 ++ >> src/box/lua/upgrade.lua | 251 ++++++++++++++++++++++++++++++++++++++- >> 2 files changed, 258 insertions(+), 2 deletions(-) Hi! Thanks for the review! Please, find my answers inline and the patch v2 in the mailing list. > Generally, the patch looks good, but it lacks a test. Please add one for > each version to test/xlog/upgrade. See a few minor comments inline. Ok, sure. >> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua >> index 4df70c210..ea73337b7 100644 >> --- a/src/box/lua/load_cfg.lua >> +++ b/src/box/lua/load_cfg.lua >> @@ -695,6 +695,12 @@ local function load_cfg(cfg) >> return box.error() -- re-throw exception from check_cfg() >> end >> >> + local snap_dir = box.cfg.memtx_dir >> + local snap_version = private.get_snapshot_version(snap_dir) >> + if snap_version and private.need_recovery_triggers(snap_version) then > Let's fold need_recovery_triggers in set_recovery_triggers. > Let's drop private.need_recovery_triggers(). AFAIU set_recovery_triggers > will be a no-op if need_recovery_triggers returns false anyway. Yep, you're right. Done. >> + private.set_recovery_triggers(snap_version) >> + end >> + >> -- NB: After this point the function should not raise an >> -- error. >> -- >> @@ -722,6 +728,9 @@ local function load_cfg(cfg) >> -- This call either succeeds or calls panic() / exit(). >> private.cfg_load() >> >> + if snap_version and private.need_recovery_triggers(snap_version) then >> + private.reset_recovery_triggers() >> + end >> -- This block does not raise an error: all necessary checks >> -- already performed in private.cfg_check(). See >> -- comment. >> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua >> index 97afc0b4d..c1f3a027a 100644 >> --- a/src/box/lua/upgrade.lua >> +++ b/src/box/lua/upgrade.lua >> @@ -1,6 +1,8 @@ >> local log = require('log') >> local bit = require('bit') >> local json = require('json') >> +local fio = require('fio') >> +local xlog = require('xlog') >> >> -- Guest user id - the default user >> local GUEST = 0 >> @@ -86,6 +88,41 @@ local function set_system_triggers(val) >> foreach_system_space(function(s) s:run_triggers(val) end) >> end >> >> +-- Get schema version, stored in _schema system space, by reading the latest >> +-- snapshot file from the snap_dir. Useful to define schema_version before >> +-- recovering the snapshot, because some schema versions are too old and cannot >> +-- be recovered normally. >> +local function get_snapshot_version(snap_dir) > This function could just as well live in load_cfg.lua, because it has > nothing to do with schema upgrade, really. Up to you. I'd rather leave it in upgrade.lua, because it uses version "class" defined in load_cfg.lua. I don't want to extract that. >> + local SCHEMA_ID = 272 >> + local snap_pattern = snap_dir..'/'..string.rep('[0-9]', 20)..'.snap' >> + local snap_list = fio.glob(snap_pattern) >> + table.sort(snap_list) >> + local snap = snap_list[#snap_list] >> + if snap == nil then >> + return nil >> + end >> + local version = nil >> + for _, row in xlog.pairs(snap) do >> + local sid = row.BODY and row.BODY.space_id >> + if sid == SCHEMA_ID then >> + local tuple = row.BODY.tuple >> + if tuple and tuple[1] == 'version' then >> + local major, minor, patch = tuple[2], tuple[3], tuple[4] or 0 >> + if major and minor and patch and type(major) == 'number' and >> + type(minor) == 'number' and type(patch) == 'number' then >> + version = mkversion(major, minor, patch) >> + break >> + end >> + end >> + elseif sid and sid > SCHEMA_ID then >> + -- Exit early if version wasn't found in _schema space. >> + -- Snapshot rows are ordered by space id. >> + break >> + end >> + end >> + return version >> +end >> + >> -------------------------------------------------------------------------------- >> -- Bootstrap >> -------------------------------------------------------------------------------- >> @@ -131,6 +168,135 @@ local function create_sysview(source_id, target_id) >> end >> end >> >> +-------------------------------------------------------------------------------- >> +-- Tarantool 1.7.1 >> +-------------------------------------------------------------------------------- >> +local function user_trig_1_7_1(_, tuple) > You should check if the new tuple is not nil in all before replace > triggers, because they are called for xlogs as well, and there may be > delete statements there. Missed that. Thanks! >> + if tuple[3] == 'guest' and not tuple[5] then >> + local auth_method_list = {} >> + auth_method_list["chap-sha1"] = box.schema.user.password("") >> + tuple = tuple:update{{'=', 5, auth_method_list}} >> + log.info("Set empty password to user 'guest'") >> + end >> + return tuple >> +end >> + >> +-------------------------------------------------------------------------------- >> +-- Tarantool 1.7.2 >> +-------------------------------------------------------------------------------- >> +local function index_trig_1_7_2(_, tuple) >> + local field_types_v16 = { >> + num = 'unsigned', >> + int = 'integer', >> + str = 'string', >> + } >> + local parts = tuple[6] >> + local changed = false >> + for _, part in pairs(parts) do >> + local field_type = part[2]:lower() >> + if field_types_v16[field_type] ~= nil then >> + part[2] = field_types_v16[field_type] >> + changed = true >> + end >> + end >> + if changed then >> + log.info("Updating index '%s' on space '%s': set parts to %s", tuple[3], >> + box.space[tuple[1]].name, json.encode(parts)) >> + tuple = tuple:update{{'=', 6, parts}} >> + end >> + return tuple >> +end >> + >> +-------------------------------------------------------------------------------- >> +-- Tarantool 1.7.5 >> +-------------------------------------------------------------------------------- >> +local function create_truncate_space() >> + local _truncate = box.space[box.schema.TRUNCATE_ID] >> + >> + log.info("create space _truncate") >> + box.space._space:insert{_truncate.id, ADMIN, '_truncate', 'memtx', 0, setmap({}), >> + {{name = 'id', type = 'unsigned'}, {name = 'count', type = 'unsigned'}}} > Line is too long. Please ensure there's no long lines (>80 characters) > introduced in this patch. Sorry, fixed. >> + >> + log.info("create index primary on _truncate") >> + box.space._index:insert{_truncate.id, 0, 'primary', 'tree', {unique = true}, {{0, 'unsigned'}}} >> + >> + local _priv = box.space[box.schema.PRIV_ID] >> + _priv:insert{ADMIN, PUBLIC, 'space', _truncate.id, box.priv.W} >> +end >> + >> +local function upgrade_to_1_7_5() >> + create_truncate_space() >> +end >> + >> +local function user_trig_1_7_5(_, tuple) >> + if not tuple[5] then >> + tuple = tuple:update{{'=', 5, setmap({})}} >> + log.info("Set empty password to %s '%s'", tuple[4], tuple[3]) >> + end >> + return tuple >> +end >> + >> +local space_formats_1_7_5 = { >> + _schema = { >> + {type='string', name='key'}, > According to our coding style, there should be spaces around '=': > > type = 'string', name = 'key' > > Please fix here and everywhere else in this patch. Ok >> + }, >> + _space = { >> + {name='id', type='unsigned'}, >> + {name='owner', type='unsigned'}, >> + {name='name', type='string'}, >> + {name='engine', type='string'}, >> + {name='field_count', type='unsigned'}, >> + {name='flags', type='map'}, >> + {name='format', type='array'}, >> + }, >> + _index = { >> + {name = 'id', type = 'unsigned'}, >> + {name = 'iid', type = 'unsigned'}, >> + {name = 'name', type = 'string'}, >> + {name = 'type', type = 'string'}, >> + {name = 'opts', type = 'map'}, >> + {name = 'parts', type = 'array'}, >> + }, >> + _func = { >> + {name='id', type='unsigned'}, >> + {name='owner', type='unsigned'}, >> + {name='name', type='string'}, >> + {name='setuid', type='unsigned'}, >> + }, >> + _user = { >> + {name='id', type='unsigned'}, >> + {name='owner', type='unsigned'}, >> + {name='name', type='string'}, >> + {name='type', type='string'}, >> + {name='auth', type='map'}, >> + }, >> + _priv = { >> + {name='grantor', type='unsigned'}, >> + {name='grantee', type='unsigned'}, >> + {name='object_type', type='string'}, >> + {name='object_id', type='unsigned'}, >> + {name='privilege', type='unsigned'}, >> + }, >> + _cluster = { >> + {name='id', type='unsigned'}, >> + {name='uuid', type='string'}, >> + }, >> +} >> +space_formats_1_7_5._vspace = space_formats_1_7_5._space >> +space_formats_1_7_5._vindex = space_formats_1_7_5._index >> +space_formats_1_7_5._vfunc = space_formats_1_7_5._func >> +space_formats_1_7_5._vuser = space_formats_1_7_5._user >> +space_formats_1_7_5._vpriv = space_formats_1_7_5._priv >> + >> +local function space_trig_1_7_5(_, tuple) >> + if space_formats_1_7_5[tuple[3]] then >> + tuple = tuple:update{{'=', 7, space_formats_1_7_5[tuple[3]]}} >> + log.info("Update space '%s' format: new format %s", tuple[3], >> + json.encode(tuple[7])) > AFAIU you shouldn't do it if it has already done, because the user may > make a new snapshot without upgrading schema version, in which case we > don't want to write this information message to the log again on the > next recovery. > > I see that some of the triggers already have this check. Please ensure > that every trigger does. Yes, indeed. Fixed. I've found and fixed another issue: imagine a user issues `box.schema.upgrade()` but doesn't make a new snapshot. Then after restart the triggers would run for the old snapshot and the xlogs generated by box.schema.upgrade(). This could revert box.schema.upgrade() to schema version ~1.7.5 in-memory. For example, space_trig_1_7_5 would then replace all the space formats with their 1.7.5 counterparts. Even if they were already upgraded to newer versions. Fixed that with a trigger on space _schema: each time new schema version is recovered, it removes all the triggers that are no longer needed. Please see patch v2 for more details. >> + end >> + return tuple >> +end >> + >> local function initial_1_7_5() >> -- stick to the following convention: >> -- prefer user id (owner id) in field #1 >> @@ -452,6 +618,13 @@ local function upgrade_to_1_7_7() >> _priv:replace({ADMIN, SUPER, 'universe', 0, 4294967295}) >> end >> >> +local function priv_trig_1_7_7(_, tuple) >> + if tuple[2] == ADMIN and tuple[3] == 'universe' then >> + tuple = tuple:update{{'=', 5, box.priv.ALL}} > Missing log.info? Yep, thanks for noticing. >> + end >> + return tuple >> +end >> + >> -------------------------------------------------------------------------------- >> --- Tarantool 1.10.0 >> -------------------------------------------------------------------------------- >> @@ -1021,6 +1194,7 @@ end >> -------------------------------------------------------------------------------- >> >> local handlers = { >> + {version = mkversion(1, 7, 5), func = upgrade_to_1_7_5, auto=true}, >> {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}, >> @@ -1061,13 +1235,82 @@ local function schema_needs_upgrade() >> return false >> end >> >> +local function need_recovery_triggers(snap_version) >> + return snap_version < handlers[1].version >> +end >> + >> +local snap_version = nil >> + >> +-- Some schema changes before version 1.7.7 make it impossible to recover from >> +-- older snapshot. The table below consists of before_replace triggers on system >> +-- spaces, which make old snapshot schema compatible with current Tarantool >> +-- (version 2.x). The triggers replace old format tuples with new ones >> +-- in-memory, thus making it possible to recover from a rather old snapshot >> +-- (up to schema version 1.6.8). Once the snapshot is recovered, a normal >> +-- upgrade procedure may set schema version to the latest one. >> +-- >> +-- The trigger mostly repeat the corresponding upgrade_to_1_7_x functions, which >> +-- were used when pre-1.7.x snapshot schema was still recoverable. >> +-- >> +-- When the triggers are used (i.e. when snapshot schema version is below 1.7.5, >> +-- the upgrade procedure works as follows: >> +-- * first the snapshot is recovered and 1.7.5-compatible schema is applied to >> +-- it in-memory with the help of triggers. >> +-- * then usual upgrade_to_X_X_X() handlers may be fired to turn schema into the >> +-- latest one. >> +local recovery_triggers = { >> + {version = mkversion(1, 7, 1), tbl = { >> + _user = user_trig_1_7_1, >> + }}, >> + {version = mkversion(1, 7, 2), tbl = { >> + _index = index_trig_1_7_2, >> + }}, >> + {version = mkversion(1, 7, 5), tbl = { >> + _space = space_trig_1_7_5, >> + _user = user_trig_1_7_5, >> + }}, >> + {version = mkversion(1, 7, 7), tbl = { >> + _priv = priv_trig_1_7_7, >> + }}, >> +} >> + >> +local function on_init_set_recovery_triggers() >> + log.info("Recovering snapshot with schema version %s", snap_version) >> + for _, trig_tbl in ipairs(recovery_triggers) do >> + if trig_tbl.version >= snap_version then >> + for space, trig in pairs(trig_tbl.tbl) do >> + box.space[space]:before_replace(trig) >> + log.info("Set recovery trigger on space '%s' to comply with ".. >> + "version %s format", space, trig_tbl.version) >> + end >> + end >> + end >> +end >> + >> +local function set_recovery_triggers(version) >> + snap_version = version > Please don't use a global variable for this - it's bad for encapsulation > and generally make code more difficult to follow. Pass version > explicitly to all functions that need it. I can't do that for an on_schema_init trigger, unfortunately. Am I missing something? Looks like there's no way to pass trigger.data for lua triggers. >> + box.ctl.on_schema_init(on_init_set_recovery_triggers) >> +end >> + >> +local function reset_recovery_triggers() > I'd call it clear_recovery_triggers. Up to you. Ok, sounds good. >> + for _, trig_tbl in ipairs(recovery_triggers) do >> + if trig_tbl.version >= snap_version then >> + for space, trig in pairs(trig_tbl.tbl) do >> + box.space[space]:before_replace(nil, trig) >> + log.info("Remove recovery trigger on space '%s' for version %s", >> + space, trig_tbl.version) >> + end >> + end >> + end >> +end >> + >> local function upgrade(options) >> options = options or {} >> setmetatable(options, {__index = {auto = false}}) >> >> local version = get_version() >> - if version < mkversion(1, 7, 5) then >> - log.warn('can upgrade from 1.7.5 only') >> + if version < mkversion(1, 6, 8) then >> + log.warn('can upgrade from 1.6.8 only') >> return >> end >> >> @@ -1110,3 +1353,7 @@ end >> box.schema.upgrade = upgrade; >> box.internal.bootstrap = bootstrap; >> box.internal.schema_needs_upgrade = schema_needs_upgrade; >> +box.internal.get_snapshot_version = get_snapshot_version; >> +box.internal.need_recovery_triggers = need_recovery_triggers; >> +box.internal.set_recovery_triggers = set_recovery_triggers; >> +box.internal.reset_recovery_triggers = reset_recovery_triggers; -- Serge Petrenko