From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Vladimir Davydov Subject: [PATCH 10/12] vinyl: remove superfluous ddl checks Date: Sat, 7 Apr 2018 16:38:07 +0300 Message-Id: In-Reply-To: References: In-Reply-To: References: To: kostja@tarantool.org Cc: tarantool-patches@freelists.org List-ID: 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