vinyl: fix crash on extending secondary key parts with primary

If a secondary index is altered in such a way that its key parts are
extended with the primary key parts, rebuild isn't required because
`cmp_def` doesn't change, see `vinyl_index_def_change_requires_rebuild`.
In this case `vinyl_index_update_def` will try to update `key_def` and
`cmp_def` in-place with `key_def_copy`. This will lead to a crash
because the number of parts in the new `key_def` is greater.

We can't use `key_def_dup` instead of `key_def_copy` there because
there may be read iterators using the old `key_def` by pointer so
there's no other option but to force rebuild in this case.

The bug was introduced in commit 6481706 ("vinyl: use update_def
index method to update vy_lsm on ddl").

Closes tarantool#10095

NO_DOC=bug fix

Author: locker

changelogs/unreleased/gh-10095-vy-index-update-def-fix.md

@@ -0,0 +1,4 @@
+## bugfix/vinyl
+
+* Fixed a bug when a DDL operation crashed in case of extending the key parts
+  of a secondary index with the primary index key parts (gh-10095).

src/box/vinyl.c

@@ -923,6 +923,11 @@ vinyl_index_update_def(struct index *index)
 {
 	struct vy_lsm *lsm = vy_lsm(index);
 	lsm->opts = index->def->opts;
+	/*
+	 * Sic: We copy key definitions in-place instead of reallocating them
+	 * because they may be used by read iterators by pointer, for example,
+	 * see vy_run_iterator.
+	 */
 	key_def_copy(lsm->key_def, index->def->key_def);
 	key_def_copy(lsm->cmp_def, index->def->cmp_def);
 }
@@ -954,7 +959,9 @@ vinyl_index_def_change_requires_rebuild(struct index *index,
 		return true;
 
 	assert(index_depends_on_pk(index));
+	const struct key_def *old_key_def = old_def->key_def;
 	const struct key_def *old_cmp_def = old_def->cmp_def;
+	const struct key_def *new_key_def = new_def->key_def;
 	const struct key_def *new_cmp_def = new_def->cmp_def;
 
 	/*
@@ -964,8 +971,15 @@ vinyl_index_def_change_requires_rebuild(struct index *index,
 	 * won't reveal such statements, but we may still need to
 	 * compare them to statements inserted after ALTER hence
 	 * we can't narrow field types without index rebuild.
+	 *
+	 * Sic: If secondary index key parts are extended with primary
+	 * index key parts, cmp_def (hence the sorting order) will stay
+	 * the same, but we still have to rebuild the index because
+	 * the new key_def has more parts so we can't update it in-place,
+	 * see vinyl_index_update_def().
 	 */
-	if (old_cmp_def->part_count != new_cmp_def->part_count)
+	if (old_key_def->part_count != new_key_def->part_count ||
+	    old_cmp_def->part_count != new_cmp_def->part_count)
 		return true;
 
 	for (uint32_t i = 0; i < new_cmp_def->part_count; i++) {

test/vinyl-luatest/gh_10095_update_index_def_test.lua

@@ -0,0 +1,35 @@
+local server = require('luatest.server')
+local t = require('luatest')
+
+local g = t.group()
+
+g.before_all(function(cg)
+    cg.server = server:new()
+    cg.server:start()
+end)
+
+g.after_all(function(cg)
+    cg.server:drop()
+end)
+
+g.after_each(function(cg)
+    cg.server:exec(function()
+        if box.space.test ~= nil then
+            box.space.test:drop()
+        end
+    end)
+end)
+
+g.test_update_index_def = function(cg)
+    cg.server:exec(function()
+        local s = box.schema.create_space('test', {engine = 'vinyl'})
+        s:create_index('pk')
+        s:create_index('sk', {parts = {{2, 'unsigned'}}})
+        s:insert({1, 10, 100})
+        t.assert_equals(s.index.sk:get({10}), {1, 10, 100})
+        s.index.sk:alter({parts = {{2, 'unsigned'}, {1, 'unsigned'}}})
+        t.assert_equals(s.index.sk:get({10, 1}), {1, 10, 100})
+        s.index.sk:alter({parts = {{2, 'unsigned'}, {3, 'unsigned'}}})
+        t.assert_equals(s.index.sk:get({10, 100}), {1, 10, 100})
+    end)
+end