[patches] [PATCH 1/1] vinyl: forbid vinyl index key definition alter

Vladislav Shpilevoy v.shpilevoy at tarantool.org
Mon Feb 19 13:50:15 MSK 2018


Vinyl index key definition is stored in vylog even if an index is
empty, and we while do not have a method to update it. So vinyl
index key definition alter is forbidden even on an empty space.

Closes #3169

Signed-off-by: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
---
 src/box/vinyl.c               | 38 ++++++++++++---------
 test/engine/truncate.result   |  2 +-
 test/engine/truncate.test.lua |  2 +-
 test/vinyl/ddl.result         | 78 +++++++++++++++++++++++++++++++------------
 test/vinyl/ddl.test.lua       | 30 +++++++++++++----
 test/vinyl/gh.result          |  2 +-
 6 files changed, 105 insertions(+), 47 deletions(-)

diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 03ad2bf4a..d5d5adc11 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1023,19 +1023,11 @@ vinyl_space_prepare_alter(struct space *old_space, struct space *new_space)
 	 */
 	if (env->status != VINYL_ONLINE)
 		return 0;
-	/* The space is empty. Allow alter. */
-	if (pk->stat.disk.count.rows == 0 &&
-	    pk->stat.memory.count.rows == 0)
-		return 0;
-	if (space_def_check_compatibility(old_space->def, new_space->def,
-					  false) != 0)
-		return -1;
-	if (old_space->index_count < new_space->index_count) {
-		diag_set(ClientError, ER_UNSUPPORTED, "Vinyl",
-			 "adding an index to a non-empty space");
-		return -1;
-	}
-
+	/*
+	 * Regardless of the space emptyness, key definition of an
+	 * existing index can not be changed, because key
+	 * definition is already in vylog. See #3169.
+	 */
 	if (old_space->index_count == new_space->index_count) {
 		/* Check index_defs to be unchanged. */
 		for (uint32_t i = 0; i < old_space->index_count; ++i) {
@@ -1053,13 +1045,27 @@ vinyl_space_prepare_alter(struct space *old_space, struct space *new_space)
 					 new_def->key_def->parts,
 					 new_def->key_def->part_count) != 0) {
 				diag_set(ClientError, ER_UNSUPPORTED, "Vinyl",
-					 "changing the definition of a non-empty "\
-					 "index");
+					 "changing the definition of an index");
 				return -1;
 			}
 		}
 	}
-	/* Drop index or a change in index options. */
+	if (pk->stat.disk.count.rows == 0 &&
+	    pk->stat.memory.count.rows == 0)
+		return 0;
+	/*
+	 * Since space format is not persisted in vylog, it can be
+	 * altered on non-empty space to some state, compatible
+	 * with the old one.
+	 */
+	if (space_def_check_compatibility(old_space->def, new_space->def,
+					  false) != 0)
+		return -1;
+	if (old_space->index_count < new_space->index_count) {
+		diag_set(ClientError, ER_UNSUPPORTED, "Vinyl",
+			 "adding an index to a non-empty space");
+		return -1;
+	}
 	return 0;
 }
 
diff --git a/test/engine/truncate.result b/test/engine/truncate.result
index 2222804b5..b6e1a99a2 100644
--- a/test/engine/truncate.result
+++ b/test/engine/truncate.result
@@ -391,7 +391,7 @@ _ = s3:insert{789, 987}
 ...
 -- Check that index drop, create, and alter called after space
 -- truncate do not break recovery (gh-2615)
-s4 = box.schema.create_space('test4', {engine = engine})
+s4 = box.schema.create_space('test4', {engine = 'memtx'})
 ---
 ...
 _ = s4:create_index('i1', {parts = {1, 'string'}})
diff --git a/test/engine/truncate.test.lua b/test/engine/truncate.test.lua
index df2797a12..66cbd1568 100644
--- a/test/engine/truncate.test.lua
+++ b/test/engine/truncate.test.lua
@@ -175,7 +175,7 @@ s3:truncate()
 _ = s3:insert{789, 987}
 -- Check that index drop, create, and alter called after space
 -- truncate do not break recovery (gh-2615)
-s4 = box.schema.create_space('test4', {engine = engine})
+s4 = box.schema.create_space('test4', {engine = 'memtx'})
 _ = s4:create_index('i1', {parts = {1, 'string'}})
 _ = s4:create_index('i3', {parts = {3, 'string'}})
 _ = s4:insert{'zzz', 111, 'yyy'}
diff --git a/test/vinyl/ddl.result b/test/vinyl/ddl.result
index 45a383442..17c5c5ca3 100644
--- a/test/vinyl/ddl.result
+++ b/test/vinyl/ddl.result
@@ -47,7 +47,7 @@ index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ...
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of a non-empty index
+- error: Vinyl does not support changing the definition of an index
 ...
 #box.space._index:select({space.id})
 ---
@@ -76,7 +76,7 @@ index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ...
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of a non-empty index
+- error: Vinyl does not support changing the definition of an index
 ...
 #box.space._index:select({space.id})
 ---
@@ -105,7 +105,7 @@ index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ...
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of a non-empty index
+- error: Vinyl does not support changing the definition of an index
 ...
 #box.space._index:select({space.id})
 ---
@@ -125,7 +125,7 @@ index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ...
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of a non-empty index
+- error: Vinyl does not support changing the definition of an index
 ...
 box.snapshot()
 ---
@@ -134,33 +134,27 @@ box.snapshot()
 while space.index.primary:info().rows ~= 0 do fiber.sleep(0.01) end
 ---
 ...
--- after a dump REPLACE + DELETE = nothing, so the space is empty now and
--- can be altered.
+-- After a dump REPLACE + DELETE = nothing, so the space is empty
+-- but an index can not be altered.
 index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ---
 ...
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
+- error: Vinyl does not support changing the definition of an index
 ...
-#box.space._index:select({space.id})
----
-- 2
-...
-box.space._index:get{space.id, 0}[6]
+-- Space format still can be altered.
+format = {}
 ---
-- [[0, 'unsigned'], [1, 'unsigned']]
 ...
-space:insert({1, 2})
+format[1] = {name = 'field1', type = 'unsigned'}
 ---
-- [1, 2]
 ...
-index:select{}
+format[2] = {name = 'field2', type = 'unsigned'}
 ---
-- - [1, 2]
 ...
-index2:select{}
+space:format(format)
 ---
-- - [1, 2]
 ...
 space:drop()
 ---
@@ -196,7 +190,7 @@ index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ...
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of a non-empty index
+- error: Vinyl does not support changing the definition of an index
 ...
 -- After compaction the REPLACE + DELETE + DELETE = nothing, so
 -- the space is now empty and can be altered.
@@ -222,8 +216,10 @@ while space.index.primary:info().rows ~= 0 do fiber.sleep(0.01) end
 index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 ---
 ...
+-- Can not alter an index even if it becames empty after dump.
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 ---
+- error: Vinyl does not support changing the definition of an index
 ...
 space:drop()
 ---
@@ -251,7 +247,7 @@ space:auto_increment{3}
 ...
 box.space._index:replace{space.id, 0, 'pk', 'tree', {unique=true}, {{0, 'unsigned'}, {1, 'unsigned'}}}
 ---
-- error: Vinyl does not support changing the definition of a non-empty index
+- error: Vinyl does not support changing the definition of an index
 ...
 space:select{}
 ---
@@ -709,7 +705,7 @@ s.index.secondary.unique
 ...
 s.index.secondary:alter{unique = true} -- error
 ---
-- error: Vinyl does not support changing the definition of a non-empty index
+- error: Vinyl does not support changing the definition of an index
 ...
 s.index.secondary.unique
 ---
@@ -727,3 +723,43 @@ s.index.secondary:select(10)
 s:drop()
 ---
 ...
+--
+-- gh-3169: vinyl index key definition can not be altered even if
+-- the index is empty.
+--
+s = box.schema.space.create('vinyl', {engine = 'vinyl'})
+---
+...
+i = s:create_index('pk')
+---
+...
+i:alter{parts = {1, 'integer'}}
+---
+- error: Vinyl does not support changing the definition of an index
+...
+_ = s:replace{-1}
+---
+- error: 'Tuple field 1 type does not match one required by operation: expected unsigned'
+...
+_ = s:replace{1}
+---
+...
+_ = s:replace{-2}
+---
+- error: 'Tuple field 1 type does not match one required by operation: expected unsigned'
+...
+_ = s:replace{3}
+---
+...
+_ = s:replace{-3}
+---
+- error: 'Tuple field 1 type does not match one required by operation: expected unsigned'
+...
+s:select{}
+---
+- - [1]
+  - [3]
+...
+s:drop()
+---
+...
diff --git a/test/vinyl/ddl.test.lua b/test/vinyl/ddl.test.lua
index 53205df99..98a19653f 100644
--- a/test/vinyl/ddl.test.lua
+++ b/test/vinyl/ddl.test.lua
@@ -48,15 +48,15 @@ space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 box.snapshot()
 while space.index.primary:info().rows ~= 0 do fiber.sleep(0.01) end
 
--- after a dump REPLACE + DELETE = nothing, so the space is empty now and
--- can be altered.
+-- After a dump REPLACE + DELETE = nothing, so the space is empty
+-- but an index can not be altered.
 index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
-#box.space._index:select({space.id})
-box.space._index:get{space.id, 0}[6]
-space:insert({1, 2})
-index:select{}
-index2:select{}
+-- Space format still can be altered.
+format = {}
+format[1] = {name = 'field1', type = 'unsigned'}
+format[2] = {name = 'field2', type = 'unsigned'}
+space:format(format)
 space:drop()
 
 space = box.schema.space.create('test', { engine = 'vinyl' })
@@ -80,6 +80,7 @@ box.snapshot()
 -- Wait until the dump is finished.
 while space.index.primary:info().rows ~= 0 do fiber.sleep(0.01) end
 index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
+-- Can not alter an index even if it becames empty after dump.
 space.index.primary:alter({parts = {1, 'unsigned', 2, 'unsigned'}})
 
 space:drop()
@@ -267,3 +268,18 @@ s.index.secondary.unique
 s:insert{2, 10}
 s.index.secondary:select(10)
 s:drop()
+
+--
+-- gh-3169: vinyl index key definition can not be altered even if
+-- the index is empty.
+--
+s = box.schema.space.create('vinyl', {engine = 'vinyl'})
+i = s:create_index('pk')
+i:alter{parts = {1, 'integer'}}
+_ = s:replace{-1}
+_ = s:replace{1}
+_ = s:replace{-2}
+_ = s:replace{3}
+_ = s:replace{-3}
+s:select{}
+s:drop()
diff --git a/test/vinyl/gh.result b/test/vinyl/gh.result
index 9c72acc81..32e4b6fb3 100644
--- a/test/vinyl/gh.result
+++ b/test/vinyl/gh.result
@@ -144,7 +144,7 @@ s:insert{5, 5}
 ...
 s.index.primary:alter({parts={2,'unsigned'}})
 ---
-- error: Vinyl does not support changing the definition of a non-empty index
+- error: Vinyl does not support changing the definition of an index
 ...
 s:drop()
 ---
-- 
2.14.3 (Apple Git-98)




More information about the Tarantool-patches mailing list