[PATCH 1/1] lua: update index Lua objects on alter, do not replace

[PATCH 1/1] lua: update index Lua objects on alter, do not replace

[Vladislav Shpilevoy]

Before the patch on any space alter Tarantool recreates its
indexes leaving old index objects be invalid. To protect a user
from errors about accessing outdated index object fields lets
update it in place instead of creating a new one.

Closes #3285
---
Branch: https://github.com/tarantool/tarantool/tree/gh-3285-index-modify-on-alter
Issue: https://github.com/tarantool/tarantool/issues/3285

 src/box/lua/space.cc    |  62 ++++++++++++++++++-----
 test/box/alter.result   | 129 ++++++++++++++++++++++++++++++++++++++++++++++++
 test/box/alter.test.lua |  55 +++++++++++++++++++++
 3 files changed, 234 insertions(+), 12 deletions(-)

diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index b03aa46e9..a26d073be 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -198,13 +198,34 @@ lbox_fillspace(struct lua_State *L, struct space *space, int i)
 		lua_settable(L, i);	/* push space.index */
 		lua_getfield(L, i, "index");
 	} else {
-		/* Empty the table. */
-		lua_pushnil(L);  /* first key */
+		/*
+		 * Remove only deleted indexes. Other ones must
+		 * be kept for modifying to avoid existing index
+		 * references invalidation.
+		 */
+		lua_pushnil(L);
 		while (lua_next(L, -2) != 0) {
-			lua_pop(L, 1); /* remove the value. */
-			lua_pushnil(L); /* set the key to nil. */
-			lua_settable(L, -3);
-			lua_pushnil(L); /* start over. */
+			if (lua_isnumber(L, -2)) {
+				uint32_t iid = (uint32_t) lua_tonumber(L, -2);
+				if (space_index(space, iid) == NULL) {
+					lua_pushnumber(L, iid);
+					lua_pushnil(L);
+					lua_settable(L, -5);
+				}
+				lua_pop(L, 1);
+			} else {
+				/*
+				 * Remove all named references due
+				 * to possible renames. They are
+				 * rebuilt below.
+				 */
+				assert(lua_isstring(L, -2));
+				lua_pushvalue(L, -2);
+				lua_pushnil(L);
+				lua_settable(L, -5);
+				lua_pop(L, 2);
+				lua_pushnil(L);
+			}
 		}
 	}
 	/*
@@ -217,8 +238,15 @@ lbox_fillspace(struct lua_State *L, struct space *space, int i)
 			continue;
 		struct index_def *index_def = index->def;
 		struct index_opts *index_opts = &index_def->opts;
-		lua_pushnumber(L, index_def->iid);
-		lua_newtable(L);		/* space.index[k] */
+		lua_rawgeti(L, -1, index_def->iid);
+		if (lua_isnil(L, -1)) {
+			lua_pop(L, 1);
+			lua_pushnumber(L, index_def->iid);
+			lua_newtable(L);
+			lua_settable(L, -3);
+			lua_rawgeti(L, -1, index_def->iid);
+			assert(! lua_isnil(L, -1));
+		}
 
 		if (index_def->type == HASH || index_def->type == TREE) {
 			lua_pushboolean(L, index_opts->is_unique);
@@ -268,10 +296,23 @@ lbox_fillspace(struct lua_State *L, struct space *space, int i)
 
 		lua_settable(L, -3); /* space.index[k].parts */
 
+		lua_pushstring(L, "sequence_id");
 		if (k == 0 && space->sequence != NULL) {
 			lua_pushnumber(L, space->sequence->def->id);
-			lua_setfield(L, -2, "sequence_id");
+		} else {
+			/*
+			 * Remove sequence_id from existing
+			 * index. On new it is ok too - the field
+			 * just is not created.
+			 */
+			lua_pushnil(L);
 		}
+		/*
+		 * Optional attributes must be set via
+		 * 'raw' API to avoid __newindex
+		 * metamethod invocation.
+		 */
+		lua_rawset(L, -3);
 
 		if (space_is_vinyl(space)) {
 			lua_pushstring(L, "options");
@@ -294,9 +335,6 @@ lbox_fillspace(struct lua_State *L, struct space *space, int i)
 
 			lua_settable(L, -3);
 		}
-
-		lua_settable(L, -3); /* space.index[k] */
-		lua_rawgeti(L, -1, index_def->iid);
 		lua_setfield(L, -2, index_def->name);
 	}
 
diff --git a/test/box/alter.result b/test/box/alter.result
index a78dd3e91..6bf91686b 100644
--- a/test/box/alter.result
+++ b/test/box/alter.result
@@ -1548,3 +1548,132 @@ s:format()
 s:drop()
 ---
 ...
+--
+-- gh-3285: index Lua object is not updated on alter.
+--
+box.internal.collation.create('test', 'ICU', 'ru-RU')
+---
+...
+s = box.schema.create_space('test')
+---
+...
+pk = s:create_index('pk')
+---
+...
+sk1 = s:create_index('b', {unique = false})
+---
+...
+sk2 = s:create_index('c', {type = 'hash', parts = {{3, 'unsigned'}}})
+---
+...
+sk3 = s:create_index('d', {parts = {{4, 'string'}}})
+---
+...
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+function index_count()
+    local r = 0
+    for i, _ in pairs(s.index) do
+        r = r + 1
+    end
+    return r / 2
+end;
+---
+...
+function validate_indexes()
+    assert(s == box.space.test)
+    assert(sk1 == nil or sk1 == s.index[sk1.name])
+    assert(sk2 == nil or sk2 == s.index[sk2.name])
+    assert(sk3 == nil or sk3 == s.index[sk3.name])
+end;
+---
+...
+test_run:cmd("setopt delimiter ''");
+---
+- true
+...
+index_count()
+---
+- 4
+...
+sk3:rename('dd')
+---
+...
+sk3.name == 'dd'
+---
+- true
+...
+index_count()
+---
+- 4
+...
+validate_indexes()
+---
+...
+sk3:alter({parts = {{4, 'string', collation = 'test'}}})
+---
+...
+sk3.parts[1].collation == 'test'
+---
+- true
+...
+index_count()
+---
+- 4
+...
+validate_indexes()
+---
+...
+sk3:drop()
+---
+...
+sk3 = nil
+---
+...
+index_count()
+---
+- 3
+...
+validate_indexes()
+---
+...
+sk2:alter({parts = {{4, 'string'}}})
+---
+...
+sk2.parts[1].type == 'string'
+---
+- true
+...
+sk2.parts[1].fieldno == 4
+---
+- true
+...
+index_count()
+---
+- 3
+...
+validate_indexes()
+---
+...
+sk1:alter({unique = true})
+---
+...
+sk1.unique
+---
+- true
+...
+index_count()
+---
+- 3
+...
+validate_indexes()
+---
+...
+s:drop()
+---
+...
+box.internal.collation.drop('test')
+---
+...
diff --git a/test/box/alter.test.lua b/test/box/alter.test.lua
index ab7135848..11b8125fe 100644
--- a/test/box/alter.test.lua
+++ b/test/box/alter.test.lua
@@ -583,3 +583,58 @@ s:format(format)
 s:format()
 
 s:drop()
+
+--
+-- gh-3285: index Lua object is not updated on alter.
+--
+box.internal.collation.create('test', 'ICU', 'ru-RU')
+s = box.schema.create_space('test')
+pk = s:create_index('pk')
+sk1 = s:create_index('b', {unique = false})
+sk2 = s:create_index('c', {type = 'hash', parts = {{3, 'unsigned'}}})
+sk3 = s:create_index('d', {parts = {{4, 'string'}}})
+test_run:cmd("setopt delimiter ';'")
+function index_count()
+    local r = 0
+    for i, _ in pairs(s.index) do
+        r = r + 1
+    end
+    return r / 2
+end;
+function validate_indexes()
+    assert(s == box.space.test)
+    assert(sk1 == nil or sk1 == s.index[sk1.name])
+    assert(sk2 == nil or sk2 == s.index[sk2.name])
+    assert(sk3 == nil or sk3 == s.index[sk3.name])
+end;
+test_run:cmd("setopt delimiter ''");
+index_count()
+
+sk3:rename('dd')
+sk3.name == 'dd'
+index_count()
+validate_indexes()
+
+sk3:alter({parts = {{4, 'string', collation = 'test'}}})
+sk3.parts[1].collation == 'test'
+index_count()
+validate_indexes()
+
+sk3:drop()
+sk3 = nil
+index_count()
+validate_indexes()
+
+sk2:alter({parts = {{4, 'string'}}})
+sk2.parts[1].type == 'string'
+sk2.parts[1].fieldno == 4
+index_count()
+validate_indexes()
+
+sk1:alter({unique = true})
+sk1.unique
+index_count()
+validate_indexes()
+
+s:drop()
+box.internal.collation.drop('test')
-- 
2.15.1 (Apple Git-101)