[PATCH 10/12] vinyl: remove superfluous ddl checks
Vladimir Davydov
vdavydov.dev at gmail.com
Sat Apr 7 16:38:07 MSK 2018
There's no need to iterate over all indexes in vinyl_space_prepare_alter
and check if any of them is going to be rebuilt - alter_space_do already
does that for us. All we need to do is raise an error if the build_index
callback is invoked for a non-empty space after recovery is complete.
The tuple format check is pointless too, as it is already done by
CheckSpaceFormat AlterSpaceOp. Remove them.
---
src/box/vinyl.c | 73 ++++++++++++++++-----------------------------------
test/box/alter.result | 4 +--
test/vinyl/ddl.result | 26 +++++++++---------
test/vinyl/gh.result | 2 +-
4 files changed, 38 insertions(+), 67 deletions(-)
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 0c475af4..47804ecc 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -976,58 +976,12 @@ vinyl_init_system_space(struct space *space)
static int
vinyl_space_prepare_alter(struct space *old_space, struct space *new_space)
{
+ (void)new_space;
struct vy_env *env = vy_env(old_space->engine);
if (vinyl_check_wal(env, "DDL") != 0)
return -1;
- /*
- * The space with no indexes can contain no rows.
- * Allow alter.
- */
- if (old_space->index_count == 0)
- return 0;
- struct vy_lsm *pk = vy_lsm(old_space->index[0]);
- /*
- * During WAL recovery, the space may be not empty. But we
- * open existing indexes, not creating new ones. Allow
- * alter.
- */
- 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 (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) {
- struct index_def *old_def, *new_def;
- old_def = space_index_def(old_space, i);
- new_def = space_index_def(new_space, i);
- /*
- * We do not support a full rebuild in
- * vinyl yet.
- */
- if (index_def_change_requires_rebuild(old_def,
- new_def)) {
- diag_set(ClientError, ER_UNSUPPORTED, "Vinyl",
- "changing the definition of "
- "a non-empty index");
- 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;
- }
- if (! tuple_format1_can_store_format2_tuples(new_space->format,
- old_space->format)) {
- diag_set(ClientError, ER_UNSUPPORTED, "Vinyl",
- "changing space format of a non-empty space");
- return -1;
- }
+
return 0;
}
@@ -1036,7 +990,6 @@ vinyl_space_check_format(struct space *space, struct tuple_format *format)
{
(void)format;
struct vy_env *env = vy_env(space->engine);
- /* @sa vy_prepare_alter_space for checks below. */
if (space->index_count == 0)
return 0;
struct vy_lsm *pk = vy_lsm(space->index[0]);
@@ -1045,7 +998,7 @@ vinyl_space_check_format(struct space *space, struct tuple_format *format)
if (pk->stat.disk.count.rows == 0 && pk->stat.memory.count.rows == 0)
return 0;
diag_set(ClientError, ER_UNSUPPORTED, "Vinyl",
- "adding new fields to a non-empty space");
+ "changing format of a non-empty space");
return -1;
}
@@ -1095,6 +1048,24 @@ vinyl_space_build_index(struct space *space, struct index *index,
struct tuple_format *format)
{
(void)format;
+
+ struct vy_env *env = vy_env(space->engine);
+ struct vy_lsm *pk = vy_lsm(space->index[0]);
+
+ /*
+ * During local recovery we are loading existing indexes
+ * from disk, not building new ones.
+ */
+ if (env->status != VINYL_INITIAL_RECOVERY_LOCAL &&
+ env->status != VINYL_FINAL_RECOVERY_LOCAL) {
+ if (pk->stat.disk.count.rows != 0 ||
+ pk->stat.memory.count.rows != 0) {
+ diag_set(ClientError, ER_UNSUPPORTED, "Vinyl",
+ "building an index for a non-empty space");
+ return -1;
+ }
+ }
+
/*
* Unlike Memtx, Vinyl does not need building of a secondary index.
* This is true because of two things:
@@ -1114,7 +1085,7 @@ vinyl_space_build_index(struct space *space, struct index *index,
return -1;
/* Set pointer to the primary key for the new index. */
- vy_lsm_update_pk(vy_lsm(index), vy_lsm(space_index(space, 0)));
+ vy_lsm_update_pk(vy_lsm(index), pk);
return 0;
}
diff --git a/test/box/alter.result b/test/box/alter.result
index 5e3cbf63..945b0cfd 100644
--- a/test/box/alter.result
+++ b/test/box/alter.result
@@ -1244,7 +1244,7 @@ t[5] = 1
...
box.space._space:replace(t)
---
-- error: Vinyl does not support changing space format of a non-empty space
+- error: Vinyl does not support changing format of a non-empty space
...
s:drop()
---
@@ -1558,7 +1558,7 @@ format[2] = {name = 'field2', type = 'unsigned'}
...
s:format(format)
---
-- error: Vinyl does not support changing space format of a non-empty space
+- error: Vinyl does not support changing format of a non-empty space
...
s:drop()
---
diff --git a/test/vinyl/ddl.result b/test/vinyl/ddl.result
index 456977e5..4ea94d36 100644
--- a/test/vinyl/ddl.result
+++ b/test/vinyl/ddl.result
@@ -83,11 +83,11 @@ space:insert({1})
-- fail because of wrong tuple format {1}, but need {1, ...}
index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
---
-- error: Vinyl does not support adding an index to a non-empty space
+- error: Vinyl does not support building an index for a non-empty space
...
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 building an index for a non-empty space
...
#box.space._index:select({space.id})
---
@@ -112,11 +112,11 @@ space:insert({1, 2})
...
index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
---
-- error: Vinyl does not support adding an index to a non-empty space
+- error: Vinyl does not support building an index for a non-empty space
...
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 building an index for a non-empty space
...
#box.space._index:select({space.id})
---
@@ -141,11 +141,11 @@ space:insert({1, 2})
...
index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
---
-- error: Vinyl does not support adding an index to a non-empty space
+- error: Vinyl does not support building an index for a non-empty space
...
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 building an index for a non-empty space
...
#box.space._index:select({space.id})
---
@@ -161,11 +161,11 @@ space:delete({1})
-- must fail because vy_mems have data
index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
---
-- error: Vinyl does not support adding an index to a non-empty space
+- error: Vinyl does not support building an index for a non-empty space
...
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 building an index for a non-empty space
...
box.snapshot()
---
@@ -232,11 +232,11 @@ while space.index.primary:info().run_count ~= 2 do fiber.sleep(0.01) end
-- must fail because vy_runs have data
index2 = space:create_index('secondary', { parts = {2, 'unsigned'} })
---
-- error: Vinyl does not support adding an index to a non-empty space
+- error: Vinyl does not support building an index for a non-empty space
...
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 building an index for a non-empty space
...
-- After compaction the REPLACE + DELETE + DELETE = nothing, so
-- the space is now empty and can be altered.
@@ -291,7 +291,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 building an index for a non-empty space
...
space:select{}
---
@@ -774,7 +774,7 @@ format[2].is_nullable = false
...
space:format(format)
---
-- error: Vinyl does not support changing space format of a non-empty space
+- error: Vinyl does not support changing format of a non-empty space
...
space:drop()
---
@@ -832,7 +832,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 building an index for a non-empty space
...
s.index.secondary.unique
---
diff --git a/test/vinyl/gh.result b/test/vinyl/gh.result
index 9c72acc8..47695acb 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 building an index for a non-empty space
...
s:drop()
---
--
2.11.0
More information about the Tarantool-patches
mailing list