From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: From: Vladimir Davydov Subject: [PATCH 09/12] alter: zap space_def_check_compatibility Date: Sat, 7 Apr 2018 16:38:06 +0300 Message-Id: <1fe6a022d9106cf0de6557915fe556f6c17b204d.1523105106.git.vdavydov.dev@gmail.com> In-Reply-To: References: In-Reply-To: References: To: kostja@tarantool.org Cc: tarantool-patches@freelists.org List-ID: space_def_check_compatibility takes two space definitions, original and the one created by alter, and checks that they are compatible, i.e. - space id and engine do not differ - temporary flag is not flipped - field definitions are compatible The last two checks are performed only if the space is empty, which is indicated by 'is_space_empty' argument. This function is called by space_vtab::prepare_alter engine callbacks with is_space_empty set to false and by generic space ALTER code with is_space_empty set to true. The reason for this is that we do not know whether the space is empty when ALTER is initiated. Actually, there's no need at all to assure that field definitions are compatible: it is guaranteed by space_vtab::check_format callback, though the error message is a bit different but still clear. After removing this piece of code from space_def_check_compatibility, this function doesn't make sense any more: we can fold space id and engine checks right into on_replace_dd_space where they originally resided; as for the temporary flag, it can be set only for memtx spaces so we can move this check to memtx_space_prepare_alter. Let's do this and remove this weird function. --- src/box/alter.cc | 14 +++++++------ src/box/memtx_space.c | 13 ++++++++---- src/box/space_def.c | 49 -------------------------------------------- src/box/space_def.h | 26 ----------------------- src/box/vinyl.c | 3 --- test/box/alter.result | 21 +++++++------------ test/box/alter_limits.result | 9 +++----- 7 files changed, 27 insertions(+), 108 deletions(-) diff --git a/src/box/alter.cc b/src/box/alter.cc index f5996850..d05c6483 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -1639,12 +1639,14 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event) access_check_ddl(def->name, def->uid, SC_SPACE, PRIV_A, true); auto def_guard = make_scoped_guard([=] { space_def_delete(def); }); - /* - * Check basic options. Assume the space to be - * empty, because we can not calculate here - * a size of a vinyl space. - */ - space_def_check_compatibility_xc(old_space->def, def, true); + if (def->id != space_id(old_space)) + tnt_raise(ClientError, ER_ALTER_SPACE, + space_name(old_space), + "space id is immutable"); + if (strcmp(def->engine_name, old_space->def->engine_name) != 0) + tnt_raise(ClientError, ER_ALTER_SPACE, + space_name(old_space), + "can not change space engine"); /* * Allow change of space properties, but do it * in WAL-error-safe mode. diff --git a/src/box/memtx_space.c b/src/box/memtx_space.c index be687288..ae266cc9 100644 --- a/src/box/memtx_space.c +++ b/src/box/memtx_space.c @@ -825,12 +825,17 @@ memtx_space_prepare_alter(struct space *old_space, struct space *new_space) { struct memtx_space *old_memtx_space = (struct memtx_space *)old_space; struct memtx_space *new_memtx_space = (struct memtx_space *)new_space; + + if (old_memtx_space->bsize != 0 && + old_space->def->opts.temporary != new_space->def->opts.temporary) { + diag_set(ClientError, ER_ALTER_SPACE, old_space->def->name, + "can not switch temporary flag on a non-empty space"); + return -1; + } + new_memtx_space->replace = old_memtx_space->replace; new_memtx_space->bsize = old_memtx_space->bsize; - bool is_empty = old_space->index_count == 0 || - index_size(old_space->index[0]) == 0; - return space_def_check_compatibility(old_space->def, - new_space->def, is_empty); + return 0; } /* }}} DDL */ diff --git a/src/box/space_def.c b/src/box/space_def.c index ecb5ad72..7349c214 100644 --- a/src/box/space_def.c +++ b/src/box/space_def.c @@ -142,52 +142,3 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count, } return def; } - -int -space_def_check_compatibility(const struct space_def *old_def, - const struct space_def *new_def, - bool is_space_empty) -{ - if (strcmp(new_def->engine_name, old_def->engine_name) != 0) { - diag_set(ClientError, ER_ALTER_SPACE, old_def->name, - "can not change space engine"); - return -1; - } - if (new_def->id != old_def->id) { - diag_set(ClientError, ER_ALTER_SPACE, old_def->name, - "space id is immutable"); - return -1; - } - if (is_space_empty) - return 0; - - if (new_def->exact_field_count != 0 && - new_def->exact_field_count != old_def->exact_field_count) { - diag_set(ClientError, ER_ALTER_SPACE, old_def->name, - "can not change field count on a non-empty space"); - return -1; - } - if (new_def->opts.temporary != old_def->opts.temporary) { - diag_set(ClientError, ER_ALTER_SPACE, old_def->name, - "can not switch temporary flag on a non-empty space"); - return -1; - } - uint32_t field_count = MIN(new_def->field_count, old_def->field_count); - for (uint32_t i = 0; i < field_count; ++i) { - enum field_type old_type = old_def->fields[i].type; - enum field_type new_type = new_def->fields[i].type; - if (!field_type1_contains_type2(new_type, old_type) && - !field_type1_contains_type2(old_type, new_type)) { - const char *msg = - tt_sprintf("Can not change a field type from "\ - "%s to %s on a not empty space", - field_type_strs[old_type], - field_type_strs[new_type]); - diag_set(ClientError, ER_ALTER_SPACE, old_def->name, - msg); - return -1; - } - } - return 0; -} - diff --git a/src/box/space_def.h b/src/box/space_def.h index 54fe2c71..97c7e138 100644 --- a/src/box/space_def.h +++ b/src/box/space_def.h @@ -133,22 +133,6 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count, const struct space_opts *opts, const struct field_def *fields, uint32_t field_count); -/** - * Check that a space with @an old_def can be altered to have - * @a new_def. - * @param old_def Old space definition. - * @param new_def New space definition. - * @param is_space_empty True, if a space is empty. - * - * @retval 0 Space definition can be altered to @a new_def. - * @retval -1 Client error. - */ -int -space_def_check_compatibility(const struct space_def *old_def, - const struct space_def *new_def, - bool is_space_empty); - - #if defined(__cplusplus) } /* extern "C" */ @@ -178,16 +162,6 @@ space_def_new_xc(uint32_t id, uint32_t uid, uint32_t exact_field_count, return ret; } -static inline void -space_def_check_compatibility_xc(const struct space_def *old_def, - const struct space_def *new_def, - bool is_space_empty) -{ - if (space_def_check_compatibility(old_def, new_def, - is_space_empty) != 0) - diag_raise(); -} - #endif /* __cplusplus */ #endif /* TARANTOOL_BOX_SPACE_DEF_H_INCLUDED */ diff --git a/src/box/vinyl.c b/src/box/vinyl.c index cbafa122..0c475af4 100644 --- a/src/box/vinyl.c +++ b/src/box/vinyl.c @@ -1017,9 +1017,6 @@ vinyl_space_prepare_alter(struct space *old_space, struct space *new_space) } } } - 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"); diff --git a/test/box/alter.result b/test/box/alter.result index 347de477..5e3cbf63 100644 --- a/test/box/alter.result +++ b/test/box/alter.result @@ -1244,8 +1244,7 @@ t[5] = 1 ... box.space._space:replace(t) --- -- error: 'Can''t modify space ''test'': can not change field count on a non-empty - space' +- error: Vinyl does not support changing space format of a non-empty space ... s:drop() --- @@ -1333,8 +1332,7 @@ ok_format_change(3, 'any') -- unsigned --X--> string fail_format_change(3, 'string') --- -- 'Can''t modify space ''test'': Can not change a field type from unsigned to string - on a not empty space' +- 'Tuple field 3 type does not match one required by operation: expected string' ... -- unsigned -----> number ok_format_change(3, 'number') @@ -1351,8 +1349,7 @@ ok_format_change(3, 'scalar') -- unsigned --X--> map fail_format_change(3, 'map') --- -- 'Can''t modify space ''test'': Can not change a field type from unsigned to map - on a not empty space' +- 'Tuple field 3 type does not match one required by operation: expected map' ... -- string -----> any ok_format_change(4, 'any') @@ -1365,8 +1362,7 @@ ok_format_change(4, 'scalar') -- string --X--> boolean fail_format_change(4, 'boolean') --- -- 'Can''t modify space ''test'': Can not change a field type from string to boolean - on a not empty space' +- 'Tuple field 4 type does not match one required by operation: expected boolean' ... -- number -----> any ok_format_change(5, 'any') @@ -1409,8 +1405,7 @@ ok_format_change(7, 'scalar') -- boolean --X--> string fail_format_change(7, 'string') --- -- 'Can''t modify space ''test'': Can not change a field type from boolean to string - on a not empty space' +- 'Tuple field 7 type does not match one required by operation: expected string' ... -- scalar -----> any ok_format_change(8, 'any') @@ -1428,8 +1423,7 @@ ok_format_change(9, 'any') -- array --X--> scalar fail_format_change(9, 'scalar') --- -- 'Can''t modify space ''test'': Can not change a field type from array to scalar - on a not empty space' +- 'Tuple field 9 type does not match one required by operation: expected scalar' ... -- map -----> any ok_format_change(10, 'any') @@ -1438,8 +1432,7 @@ ok_format_change(10, 'any') -- map --X--> scalar fail_format_change(10, 'scalar') --- -- 'Can''t modify space ''test'': Can not change a field type from map to scalar on - a not empty space' +- 'Tuple field 10 type does not match one required by operation: expected scalar' ... s:drop() --- diff --git a/test/box/alter_limits.result b/test/box/alter_limits.result index 93e99dbe..4fd80a37 100644 --- a/test/box/alter_limits.result +++ b/test/box/alter_limits.result @@ -285,8 +285,7 @@ FIELD_COUNT = 4 -- increase field_count -- error box.space['_space']:update(s.id, {{"=", FIELD_COUNT + 1, 3}}) --- -- error: 'Can''t modify space ''test'': can not change field count on a non-empty - space' +- error: Tuple field count 2 does not match space field count 3 ... s:select{} --- @@ -295,8 +294,7 @@ s:select{} -- decrease field_count - error box.space['_space']:update(s.id, {{"=", FIELD_COUNT + 1, 1}}) --- -- error: 'Can''t modify space ''test'': can not change field count on a non-empty - space' +- error: Tuple field count 2 does not match space field count 1 ... -- remove field_count - ok _ = box.space['_space']:update(s.id, {{"=", FIELD_COUNT + 1, 0}}) @@ -309,8 +307,7 @@ s:select{} -- increase field_count - error box.space['_space']:update(s.id, {{"=", FIELD_COUNT + 1, 3}}) --- -- error: 'Can''t modify space ''test'': can not change field count on a non-empty - space' +- error: Tuple field count 2 does not match space field count 3 ... s:truncate() --- -- 2.11.0