[tarantool-patches] Re: [PATCH v2 3/9] box: fix Tarantool upgrade from 2.1.0 to 2.1.1
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Mar 22 12:28:58 MSK 2019
Hi! Thanks for the patch!
>
> I've rebased this separate patch on the branch
> kshch/migration-fixup
> you may cherry-pick it if it is really reasonable.
>
First of all, when you make something a separate
patch/patchset, it should be sent separately. It is really hard to
find which patchsets and branches exist in that thread containing
22 emails.
Secondly, talking of the patch itself, see 3 comments
below, review fixes at the bottom of the email, and on
the branch.
> commit 2b810b5c0e68ccc9e8796f41cf15ec693887be69
> Author: Kirill Shcherbatov <kshcherbatov at tarantool.org>
> Date: Fri Jan 25 13:21:01 2019 +0300
>
> box: fix Tarantool upgrade from 2.1.0 to 2.1.1
>
> Tarantool could not start from the snapshot created by version
> 2.1.0 because the new version 2.1.1 does not support the
> index.opts.sql index opt and stops the execution.
> Introduced a special state OPT_DEF_LEGACY macro to ignore legacy
> options and introduced migration code in upgrade.lua.
>
> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> index c72711ff7..944069913 100644
> --- a/src/box/lua/upgrade.lua
> +++ b/src/box/lua/upgrade.lua
> @@ -616,6 +616,27 @@ local function upgrade_to_2_1_0()
> upgrade_priv_to_2_1_0()
> end
>
> +--------------------------------------------------------------------------------
> +-- Tarantool 2.1.1
> +--------------------------------------------------------------------------------
> +
> +local function upgrade_priv_to_2_1_1()
1. Why 'priv'? upgrade_priv_to_2_1_0 contains 'priv' suffix,
because it upgrades _priv system space and privileges.
> + local _index = box.space[box.schema.INDEX_ID]
> + for _, index in _index:pairs() do
> + local opts = index.opts
> + if opts['sql'] ~= nil then
> + opts['sql'] = nil
> + _index:replace(box.tuple.new({index.id, index.iid, index.name,
> + index.type, opts, index.parts}))
> + end
> + end
> +end
> +
> +local function upgrade_to_2_1_1()
> + log.info("started upgrade_to_2_1_1")
2. Looks like a debug print, that you've forgotten to drop.
Please, do it.
> + upgrade_priv_to_2_1_1()
> +end
> +
> local function get_version()
> local version = box.space._schema:get{'version'}
> if version == nil then
> diff --git a/src/box/opt_def.h b/src/box/opt_def.h
> index 318204e91..27d088e74 100644
> --- a/src/box/opt_def.h
> +++ b/src/box/opt_def.h
> @@ -86,6 +86,7 @@ struct opt_def {
> int enum_size;
> const char **enum_strs;
> uint32_t enum_max;
> + bool is_legacy;
3. As I understand, legacy means that whatever is stored here, it
should be skipped. But it contradicts with other member of struct
opt_def - your legacy structure contains a type in enum opt_type type.
Moreover - invalid type.
===================================================================
commit bd90066e118ecddf3f04bdec1868b450cb4a2bab
Author: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
Date: Fri Mar 22 10:50:39 2019 +0300
Review fixes
diff --git a/src/box/opt_def.c b/src/box/opt_def.c
index 17c555a0e..c78021440 100644
--- a/src/box/opt_def.c
+++ b/src/box/opt_def.c
@@ -45,6 +45,7 @@ const char *opt_type_strs[] = {
/* [OPT_STRPTR] = */ "string",
/* [OPT_ENUM] = */ "enum",
/* [OPT_ARRAY] = */ "array",
+ /* [OPT_LEGACY] = */ "legacy",
};
static int
@@ -145,6 +146,9 @@ opt_set(void *opts, const struct opt_def *def, const char **val,
if (def->to_array(val, ival, opt, errcode, field_no) != 0)
return -1;
break;
+ case OPT_LEGACY:
+ mp_next(val);
+ break;
default:
unreachable();
}
@@ -167,8 +171,6 @@ opts_parse_key(void *opts, const struct opt_def *reg, const char *key,
if (key_len != strlen(def->name) ||
memcmp(key, def->name, key_len) != 0)
continue;
- if (def->is_legacy)
- goto skip;
return opt_set(opts, def, data, region, errcode, field_no);
}
@@ -179,7 +181,6 @@ opts_parse_key(void *opts, const struct opt_def *reg, const char *key,
diag_set(ClientError, errcode, field_no, errmsg);
return -1;
}
-skip:
mp_next(data);
return 0;
}
diff --git a/src/box/opt_def.h b/src/box/opt_def.h
index 27d088e74..21544412c 100644
--- a/src/box/opt_def.h
+++ b/src/box/opt_def.h
@@ -48,6 +48,7 @@ enum opt_type {
OPT_STRPTR, /* char* */
OPT_ENUM, /* enum */
OPT_ARRAY, /* array */
+ OPT_LEGACY, /* any type, skipped */
opt_type_MAX,
};
@@ -86,7 +87,6 @@ struct opt_def {
int enum_size;
const char **enum_strs;
uint32_t enum_max;
- bool is_legacy;
/** MsgPack data decode callbacks. */
union {
opt_def_to_enum_cb to_enum;
@@ -96,21 +96,21 @@ struct opt_def {
#define OPT_DEF(key, type, opts, field) \
{ key, type, offsetof(opts, field), sizeof(((opts *)0)->field), \
- NULL, 0, NULL, 0, false, {NULL} }
+ NULL, 0, NULL, 0, {NULL} }
#define OPT_DEF_ENUM(key, enum_name, opts, field, to_enum) \
{ key, OPT_ENUM, offsetof(opts, field), sizeof(int), #enum_name, \
sizeof(enum enum_name), enum_name##_strs, enum_name##_MAX, \
- false, {(void *)to_enum} }
+ {(void *)to_enum} }
#define OPT_DEF_ARRAY(key, opts, field, to_array) \
{ key, OPT_ARRAY, offsetof(opts, field), sizeof(((opts *)0)->field), \
- NULL, 0, NULL, 0, false, {(void *)to_array} }
+ NULL, 0, NULL, 0, {(void *)to_array} }
#define OPT_DEF_LEGACY(key) \
- { key, opt_type_MAX, 0, 0, NULL, 0, NULL, 0, true, {NULL} }
+ { key, OPT_LEGACY, 0, 0, NULL, 0, NULL, 0, {NULL} }
-#define OPT_END {NULL, opt_type_MAX, 0, 0, NULL, 0, NULL, 0, false, {NULL}}
+#define OPT_END {NULL, opt_type_MAX, 0, 0, NULL, 0, NULL, 0, {NULL}}
struct region;
More information about the Tarantool-patches
mailing list