Tarantool development patches archive
 help / color / mirror / Atom feed
From: Serge Petrenko via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Vladimir Davydov <vdavydov@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH] box: allow upgrading from version 1.6
Date: Fri, 13 Aug 2021 02:20:23 +0300	[thread overview]
Message-ID: <90eeefb5-3719-3198-dec7-4f00834733e7@tarantool.org> (raw)
In-Reply-To: <20210811110654.y3xyaojebckp6flq@esperanza>



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 <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.

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


  reply	other threads:[~2021-08-12 23:20 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-10 11:28 Serge Petrenko via Tarantool-patches
2021-08-10 14:43 ` Sergey Ostanevich via Tarantool-patches
2021-08-11 12:36   ` Serge Petrenko via Tarantool-patches
2021-08-11 11:06 ` Vladimir Davydov via Tarantool-patches
2021-08-12 23:20   ` Serge Petrenko via Tarantool-patches [this message]
2021-08-13  7:54     ` Vladimir Davydov via Tarantool-patches
2021-08-13 10:47       ` Serge Petrenko via Tarantool-patches

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=90eeefb5-3719-3198-dec7-4f00834733e7@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=sergepetrenko@tarantool.org \
    --cc=vdavydov@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] box: allow upgrading from version 1.6' \
    /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