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 283326EC40; Wed, 11 Aug 2021 14:07:02 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 283326EC40 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1628680022; bh=eBAmaafVQzq7ACjfP28CqiQ9/L2oHIiKQAouWy/tddc=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=Qjs7KTowkU4zyZ17F+a9fC1fcCG0K6Ur/6wUrtAwtT/GAIEimSRKY6C2d5/aP3M18 QoGhIq36UTGc+k6oQ+bPmF6tyQFg3EDSs2ZWejBlcUcOD3Z6LgwO1FdPProhkuzAZk 0JiQ7D+azZC7WuGzLYhv7hf9h4q/s83QBuz8ed18= Received: from smtpng2.i.mail.ru (smtpng2.i.mail.ru [94.100.179.3]) (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 DFE8F6EC40 for ; Wed, 11 Aug 2021 14:06:56 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org DFE8F6EC40 Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1mDm4V-0006ya-Rs; Wed, 11 Aug 2021 14:06:56 +0300 Date: Wed, 11 Aug 2021 14:06:54 +0300 To: Serge Petrenko Message-ID: <20210811110654.y3xyaojebckp6flq@esperanza> References: <20210810112836.48775-1-sergepetrenko@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210810112836.48775-1-sergepetrenko@tarantool.org> X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD92087353F0EC44DD9D5AC6413C25DCF08CC98B8FCC5CD86F3182A05F5380850407F1A8F85B78CC686F05C49361F2843E72CCF86CDE670A8B1AAE48565B95A4F60 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7A179494B5629353BEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637531CC3E3F637A59A8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D86EB34EF61DF9CBB9E927C0AEB6794ABF117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8B2EE5AD8F952D28FBA471835C12D1D977C4224003CC8364762BB6847A3DEAEFB0F43C7A68FF6260569E8FC8737B5C2249EC8D19AE6D49635B68655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7C565C1E6824D8037B43847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A5009B82033D0CCBF6A86C6017A76659B5B8D19C3556EB9EA7D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA753177526CD55AFC11410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D347798CB24D8990473DD67C8CD75FE3B368BA33CC4DFF02313FCDCBD844B38A099186738FC955B5B8D1D7E09C32AA3244C0D32B97412AA5BB9C8C44E6272FD1C13B018FE5BB746DCD1927AC6DF5659F194 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj6qlzQV0oSZPzzStsreV4dg== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5DF9FBC774C7DCBFABB9F991956CA88606274CEFED1673C562683ABF942079399BFB559BB5D741EB966A65DFF43FF7BE03240331F90058701C67EA787935ED9F1B 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: Vladimir Davydov via Tarantool-patches Reply-To: Vladimir Davydov Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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(-) 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. > > 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. > + 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. > + 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. > + 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. > + > + 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. > + }, > + _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. > + 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? > + 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. > + 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. > + 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;