[tarantool-patches] Re: [PATCH v2 3/9] box: fix Tarantool upgrade from 2.1.0 to 2.1.1

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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@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@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;