[Tarantool-patches] [PATCH] box: allow upgrading from version 1.6
Vladimir Davydov
vdavydov at tarantool.org
Wed Aug 11 14:06:54 MSK 2021
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 <dynamic_cfg>
> -- 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;
More information about the Tarantool-patches
mailing list