[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