box: move index_opts::hint check from Lua to space_check_index_def

The checks in box.schema.index.create() and box.schema.index.alter()
were case sensitive, also it was possible to insert incorrect index
options directly into `box.space._index`. Fixed by adding checks
to memtx_space_check_index_def() and vinyl_space_check_index_def().

Closes tarantool#8937

NO_DOC=bugfix

(cherry picked from commit 4e25384)

changelogs/unreleased/gh-8937-memtx-tree-index-doesnt-accept-true-option-for-hint.md

@@ -0,0 +1,4 @@
+## bugfix/core
+
+* Fixed a bug because of which it was impossible to set the `hint` option
+  to `true` for TREE indexes (gh-8937).

src/box/alter.cc

@@ -350,6 +350,17 @@ index_def_new_from_tuple(struct tuple *tuple, struct space *space)
 		return NULL;
 	if (space_check_index_def(space, index_def) != 0)
 		return NULL;
+	/*
+	 * Set opts.hint to the unambiguous ON or OFF value. This
+	 * allows to compare opts.hint like in index_opts_cmp()
+	 * or memtx_index_def_change_requires_rebuild().
+	 */
+	if (index_def->opts.hint == INDEX_HINT_DEFAULT) {
+		if (space_is_memtx(space) && type == TREE)
+			index_def->opts.hint = INDEX_HINT_ON;
+		else
+			index_def->opts.hint = INDEX_HINT_OFF;
+	}
 	/*
 	 * In case of functional index definition, resolve a
 	 * function pointer to perform a complete index build

src/box/index_def.c

@@ -51,9 +51,30 @@ const struct index_opts index_opts_default = {
 	/* .lsn                 = */ 0,
 	/* .stat                = */ NULL,
 	/* .func                = */ 0,
-	/* .hint                = */ true,
+	/* .hint                = */ INDEX_HINT_DEFAULT,
 };
 
+/**
+ * Parse index hint option from msgpack.
+ * Used as callback to parse a boolean value with 'hint' key in index options.
+ * Move @a data msgpack pointer to the end of msgpack value.
+ * By convention @a opts must point to corresponding struct index_opts.
+ * Return 0 on success or -1 on error (diag is set to IllegalParams).
+ */
+static int
+index_opts_parse_hint(const char **data, void *opts, struct region *region)
+{
+	(void)region;
+	struct index_opts *index_opts = (struct index_opts *)opts;
+	if (mp_typeof(**data) != MP_BOOL) {
+		diag_set(IllegalParams, "'hint' must be boolean");
+		return -1;
+	}
+	bool hint = mp_decode_bool(data);
+	index_opts->hint = hint ? INDEX_HINT_ON : INDEX_HINT_OFF;
+	return 0;
+}
+
 const struct opt_def index_opts_reg[] = {
 	OPT_DEF("unique", OPT_BOOL, struct index_opts, is_unique),
 	OPT_DEF("dimension", OPT_INT64, struct index_opts, dimension),
@@ -67,7 +88,7 @@ const struct opt_def index_opts_reg[] = {
 	OPT_DEF("lsn", OPT_INT64, struct index_opts, lsn),
 	OPT_DEF("func", OPT_UINT32, struct index_opts, func_id),
 	OPT_DEF_LEGACY("sql"),
-	OPT_DEF("hint", OPT_BOOL, struct index_opts, hint),
+	OPT_DEF_CUSTOM("hint", index_opts_parse_hint),
 	OPT_END,
 };
 

src/box/index_def.h

@@ -49,6 +49,13 @@ enum index_type {
 
 extern const char *index_type_strs[];
 
+/** Settings for the hint config option. */
+enum index_hint_cfg {
+	INDEX_HINT_DEFAULT = 0,
+	INDEX_HINT_ON,
+	INDEX_HINT_OFF
+};
+
 enum rtree_index_distance_type {
 	 /* Euclid distance, sqrt(dx*dx + dy*dy) */
 	RTREE_INDEX_DISTANCE_TYPE_EUCLID,
@@ -168,7 +175,7 @@ struct index_opts {
 	/**
 	 * Use hint optimization for tree index.
 	 */
-	bool hint;
+	enum index_hint_cfg hint;
 };
 
 extern const struct index_opts index_opts_default;

src/box/lua/schema.lua

@@ -1559,11 +1559,6 @@ box.schema.index.create = function(space_id, name, options)
         options_defaults = {}
     end
     options = update_param_table(options, options_defaults)
-    if options.hint and
-            (options.type ~= 'tree' or box.space[space_id].engine ~= 'memtx') then
-        box.error(box.error.MODIFY_INDEX, name, space.name,
-                "hint is only reasonable with memtx tree index")
-    end
     if options.hint and options.func then
         box.error(box.error.MODIFY_INDEX, name, space.name,
                 "functional index can't use hints")
@@ -1751,12 +1746,6 @@ box.schema.index.alter = function(space_id, index_id, options)
             index_opts[k] = options[k]
         end
     end
-    if options.hint and
-       (options.type ~= 'tree' or box.space[space_id].engine ~= 'memtx') then
-        box.error(box.error.MODIFY_INDEX, space.index[index_id].name,
-                                          space.name,
-            "hint is only reasonable with memtx tree index")
-    end
     if options.hint and options.func then
         box.error(box.error.MODIFY_INDEX, space.index[index_id].name,
                                           space.name,

src/box/lua/space.cc

@@ -416,7 +416,7 @@ lbox_fillspace(struct lua_State *L, struct space *space, int i)
 			lua_setfield(L, -2, "dimension");
 		}
 		if (space_is_memtx(space) && index_def->type == TREE) {
-			lua_pushboolean(L, index_opts->hint);
+			lua_pushboolean(L, index_opts->hint == INDEX_HINT_ON);
 			lua_setfield(L, -2, "hint");
 		} else {
 			lua_pushnil(L);

src/box/memtx_space.c

@@ -818,6 +818,19 @@ memtx_space_check_index_def(struct space *space, struct index_def *index_def)
 			 index_def->name, space_name(space));
 		return -1;
 	}
+
+	if (index_def->type != TREE && index_def->opts.hint == INDEX_HINT_ON &&
+	    recovery_state == FINISHED_RECOVERY) {
+		/*
+		 * The error is silenced during recovery to be able to recover
+		 * the indexes with incorrect hint options.
+		 */
+		diag_set(ClientError, ER_MODIFY_INDEX, index_def->name,
+			 space_name(space),
+			 "hint is only reasonable with memtx tree index");
+		return -1;
+	}
+
 	/* Only HASH and TREE indexes checks parts there */
 	/* Check that there are no ANY, ARRAY, MAP parts */
 	for (uint32_t i = 0; i < key_def->part_count; i++) {

src/box/memtx_tree.cc

@@ -2156,7 +2156,7 @@ memtx_tree_index_new(struct memtx_engine *memtx, struct index_def *def)
 	} else if (def->key_def->is_multikey) {
 		vtab = get_memtx_tree_index_vtab<MEMTX_TREE_VTAB_MULTIKEY>();
 		use_hint = true;
-	} else if (def->opts.hint) {
+	} else if (def->opts.hint == INDEX_HINT_ON) {
 		vtab = get_memtx_tree_index_vtab
 			<MEMTX_TREE_VTAB_GENERAL, true>();
 		use_hint = true;

src/box/vinyl.c

@@ -670,6 +670,17 @@ vinyl_space_check_index_def(struct space *space, struct index_def *index_def)
 			 index_def->name, space_name(space));
 		return -1;
 	}
+	if (index_def->opts.hint == INDEX_HINT_ON &&
+	    recovery_state == FINISHED_RECOVERY) {
+		/*
+		 * The error is silenced during recovery to be able to recover
+		 * the indexes with incorrect hint options.
+		 */
+		diag_set(ClientError, ER_MODIFY_INDEX, index_def->name,
+			 space_name(space),
+			 "hint is only reasonable with memtx tree index");
+		return -1;
+	}
 
 	struct key_def *key_def = index_def->key_def;
 

test/box-luatest/gh_8937_data/gen.lua

@@ -0,0 +1,14 @@
+-- This script can be used only with Tarantool without the fix for gh-8937 (e.g.
+-- version 3.0.0-alpha1-62-g983a7ec21).
+
+box.cfg{}
+
+local s = box.schema.create_space('gh_8937_memtx')
+box.space._index:insert{s.id, 0, "pk", "hash", {hint = true}, {{0, "unsigned"}}}
+
+s = box.schema.create_space('gh_8937_vinyl', {engine = 'vinyl'})
+box.space._index:insert{s.id, 0, "pk", "tree", {hint = true}, {{0, "unsigned"}}}
+
+box.snapshot()
+
+os.exit(0)

test/box-luatest/gh_8937_recover_wrong_hint_options_test.lua

@@ -0,0 +1,22 @@
+local t = require('luatest')
+local g = t.group('gh-8937')
+
+g.before_all(function(cg)
+    local server = require('luatest.server')
+    cg.server = server:new{datadir = 'test/box-luatest/gh_8937_data'}
+    cg.server:start()
+end)
+
+g.after_all(function(cg)
+    cg.server:drop()
+end)
+
+-- Check that indexes are recovered without errors like:
+-- "Can't create or modify index 'pk' in space 'gh_8937_memtx':
+-- hint is only reasonable with memtx tree index".
+g.test_recovery = function(cg)
+    cg.server:exec(function()
+        t.assert_equals(box.space.gh_8937_memtx.index[0].name, "pk")
+        t.assert_equals(box.space.gh_8937_vinyl.index[0].name, "pk")
+    end)
+end

test/box/tree_pk.result

@@ -868,24 +868,77 @@ box.internal.collation.drop('test')
 box.internal.collation.drop('test-ci')
 ---
 ...
--- hints
+-- memtx hints
 s = box.schema.space.create('test')
 ---
 ...
-s:create_index('test', {type = 'tree', hint = 'true'} )
+s:create_index('t1', {type = 'TRee'}).hint
+---
+- true
+...
+s:create_index('h1', {type = 'hAsh'}).hint
+---
+- null
+...
+s:create_index('t2', {type = 'trEE', hint = 'true'})
 ---
 - error: Illegal parameters, options parameter 'hint' should be of type boolean
 ...
-s:create_index('test', {type = 'hash', hint = true} )
+s:create_index('t2', {type = 'TREE', hint = true}).hint
+---
+- true
+...
+s:create_index('t3', {type = 'tree', hint = false}).hint
+---
+- false
+...
+s:create_index('h2', {type = 'HASH', hint = true})
+---
+- error: 'Can''t create or modify index ''h2'' in space ''test'': hint is only reasonable
+    with memtx tree index'
+...
+s:create_index('h2', {type = 'hash', hint = false}).hint
+---
+- null
+...
+_ = s:create_index('t4', {type = 'TREE'}):alter({hint = true})
+---
+...
+s.index.t4.hint
+---
+- true
+...
+_ = s:create_index('t5', {type = 'tree'}):alter({hint = false})
+---
+...
+s.index.t5.hint
+---
+- false
+...
+s:create_index('h3', {type = 'haSH'}):alter({hint = true})
 ---
-- error: 'Can''t create or modify index ''test'' in space ''test'': hint is only reasonable
+- error: 'Can''t create or modify index ''h3'' in space ''test'': hint is only reasonable
     with memtx tree index'
 ...
-s:create_index('test', {type = 'hash'}):alter({hint = true})
+s:create_index('h4', {type = 'hash'}):alter({hint = false})
+---
+...
+s.index.h4.hint
+---
+- null
+...
+box.space._index:insert{s.id, s.index.h4.id + 1, "h5", "hash", {hint = true}, {{0, "unsigned"}}}
 ---
-- error: 'Can''t create or modify index ''test'' in space ''test'': hint is only reasonable
+- error: 'Can''t create or modify index ''h5'' in space ''test'': hint is only reasonable
     with memtx tree index'
 ...
+_ = box.space._index:insert{s.id, s.index.h4.id + 1, "h5", "hash", {hint = false}, {{0, "unsigned"}}}
+---
+...
+box.space._index:insert{s.id, s.index.h5.id + 1, "h6", "hash", {hint = "false"}, {{0, "unsigned"}}}
+---
+- error: 'Wrong index options: ''hint'' must be boolean'
+...
 s:create_index('multikey', {hint = true, parts = {{2, 'int', path = '[*]'}}})
 ---
 - error: 'Can''t create or modify index ''multikey'' in space ''test'': multikey index
@@ -910,14 +963,39 @@ s:create_index('func', {hint = true, func = box.func.s.id, parts = {{1, 'unsigne
 s:drop()
 ---
 ...
+-- vinyl hints
 s = box.schema.space.create('test', {engine = 'vinyl'})
 ---
 ...
-s:create_index('test', {type = 'tree', hint = true} )
+s:create_index('i1', {type = 'tree'}).hint
+---
+- null
+...
+s:create_index('i2', {type = 'TREE', hint = true})
 ---
-- error: 'Can''t create or modify index ''test'' in space ''test'': hint is only reasonable
+- error: 'Can''t create or modify index ''i2'' in space ''test'': hint is only reasonable
     with memtx tree index'
 ...
+s:create_index('i2', {type = 'tree', hint = false}).hint
+---
+- null
+...
+s:create_index('i3', {type = 'TREE'}):alter({hint = true})
+---
+- error: 'Can''t create or modify index ''i3'' in space ''test'': hint is only reasonable
+    with memtx tree index'
+...
+_ = s:create_index('i4', {type = 'tree'}):alter({hint = false})
+---
+...
+box.space._index:insert{s.id, s.index.i4.id + 1, "i5", "tree", {hint = true}, {{0, "unsigned"}}}
+---
+- error: 'Can''t create or modify index ''i5'' in space ''test'': hint is only reasonable
+    with memtx tree index'
+...
+_ = box.space._index:insert{s.id, s.index.i4.id + 1, "i6", "tree", {hint = false}, {{0, "unsigned"}}}
+---
+...
 s:drop()
 ---
 ...

test/box/tree_pk.test.lua

@@ -304,20 +304,41 @@ s:drop()
 box.internal.collation.drop('test')
 box.internal.collation.drop('test-ci')
 
--- hints
+-- memtx hints
 s = box.schema.space.create('test')
-s:create_index('test', {type = 'tree', hint = 'true'} )
-s:create_index('test', {type = 'hash', hint = true} )
-s:create_index('test', {type = 'hash'}):alter({hint = true})
+s:create_index('t1', {type = 'TRee'}).hint
+s:create_index('h1', {type = 'hAsh'}).hint
+s:create_index('t2', {type = 'trEE', hint = 'true'})
+s:create_index('t2', {type = 'TREE', hint = true}).hint
+s:create_index('t3', {type = 'tree', hint = false}).hint
+s:create_index('h2', {type = 'HASH', hint = true})
+s:create_index('h2', {type = 'hash', hint = false}).hint
+_ = s:create_index('t4', {type = 'TREE'}):alter({hint = true})
+s.index.t4.hint
+_ = s:create_index('t5', {type = 'tree'}):alter({hint = false})
+s.index.t5.hint
+s:create_index('h3', {type = 'haSH'}):alter({hint = true})
+s:create_index('h4', {type = 'hash'}):alter({hint = false})
+s.index.h4.hint
+box.space._index:insert{s.id, s.index.h4.id + 1, "h5", "hash", {hint = true}, {{0, "unsigned"}}}
+_ = box.space._index:insert{s.id, s.index.h4.id + 1, "h5", "hash", {hint = false}, {{0, "unsigned"}}}
+box.space._index:insert{s.id, s.index.h5.id + 1, "h6", "hash", {hint = "false"}, {{0, "unsigned"}}}
 s:create_index('multikey', {hint = true, parts = {{2, 'int', path = '[*]'}}})
 s:create_index('multikey', {parts = {{2, 'int', path = '[*]'}}}):alter({hint = true})
 lua_code = [[function(tuple) return {tuple[1] + tuple[2]} end]]
 box.schema.func.create('s', {body = lua_code, is_deterministic = true, is_sandboxed = true})
 s:create_index('func', {hint = true, func = box.func.s.id, parts = {{1, 'unsigned'}}})
 s:drop()
 
+-- vinyl hints
 s = box.schema.space.create('test', {engine = 'vinyl'})
-s:create_index('test', {type = 'tree', hint = true} )
+s:create_index('i1', {type = 'tree'}).hint
+s:create_index('i2', {type = 'TREE', hint = true})
+s:create_index('i2', {type = 'tree', hint = false}).hint
+s:create_index('i3', {type = 'TREE'}):alter({hint = true})
+_ = s:create_index('i4', {type = 'tree'}):alter({hint = false})
+box.space._index:insert{s.id, s.index.i4.id + 1, "i5", "tree", {hint = true}, {{0, "unsigned"}}}
+_ = box.space._index:insert{s.id, s.index.i4.id + 1, "i6", "tree", {hint = false}, {{0, "unsigned"}}}
 s:drop()
 
 -- numeric hints