[PATCH 09/12] alter: zap space_def_check_compatibility
Vladimir Davydov
vdavydov.dev at gmail.com
Sat Apr 7 16:38:06 MSK 2018
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
More information about the Tarantool-patches
mailing list