From f24facdcb80ae8b08dedcd6a989b9342ef8cfaa0 Mon Sep 17 00:00:00 2001
From: Konstantin Osipov <kostja@tarantool.org>
Date: Tue, 20 Feb 2018 21:13:36 +0300
Subject: [PATCH] alter: a follow up on the fix for gh-3008
Update comments and move space_def_check_compatibility to
space_def.[hc].
---
src/box/space.c | 48 ----------------------------------------
src/box/space.h | 25 ---------------------
src/box/space_def.c | 50 ++++++++++++++++++++++++++++++++++++++++++
src/box/space_def.h | 26 ++++++++++++++++++++++
src/box/tuple_format.c | 20 +++++++++++------
src/box/tuple_format.h | 15 ++++++-------
src/box/vinyl.c | 2 +-
test/box/alter.result | 2 +-
test/vinyl/ddl.result | 2 +-
9 files changed, 99 insertions(+), 91 deletions(-)
diff --git a/src/box/space.c b/src/box/space.c
index 1682022716..3f0097e650 100644
--- a/src/box/space.c
+++ b/src/box/space.c
@@ -263,54 +263,6 @@ index_name_by_id(struct space *space, uint32_t id)
return NULL;
}
-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;
-}
-
/**
* Run BEFORE triggers registered for a space. If a trigger
* changes the current statement, this function updates the
diff --git a/src/box/space.h b/src/box/space.h
index 8918dbd8df..6408eedcb7 100644
--- a/src/box/space.h
+++ b/src/box/space.h
@@ -287,21 +287,6 @@ space_index_def(struct space *space, int n);
const char *
index_name_by_id(struct space *space, uint32_t id);
-/**
- * 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);
-
/**
* Check whether or not the current user can be granted
* the requested access to the space.
@@ -442,16 +427,6 @@ space_fill_index_map(struct space *space);
#if defined(__cplusplus)
} /* extern "C" */
-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();
-}
-
static inline struct space *
space_new_xc(struct space_def *space_def, struct rlist *key_list)
{
diff --git a/src/box/space_def.c b/src/box/space_def.c
index ce5872ffb2..ecb5ad723c 100644
--- a/src/box/space_def.c
+++ b/src/box/space_def.c
@@ -31,6 +31,7 @@
#include "space_def.h"
#include "diag.h"
+#include "error.h"
const struct space_opts space_opts_default = {
/* .temporary = */ false,
@@ -141,3 +142,52 @@ 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 97c7e13809..54fe2c712c 100644
--- a/src/box/space_def.h
+++ b/src/box/space_def.h
@@ -133,6 +133,22 @@ 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" */
@@ -162,6 +178,16 @@ 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/tuple_format.c b/src/box/tuple_format.c
index f0eca24bb3..e42fc039ed 100644
--- a/src/box/tuple_format.c
+++ b/src/box/tuple_format.c
@@ -291,15 +291,18 @@ tuple_format1_can_store_format2_tuples(const struct tuple_format *format1,
for (uint32_t i = 0; i < format1->field_count; ++i) {
const struct tuple_field *field1 = &format1->fields[i];
/*
- * The field is formatted in format1, but not
- * formatted in format2.
+ * The field has a data type in format1, but has
+ * no data type in format2.
*/
if (i >= format2->field_count) {
/*
- * The field can be defined with no type,
- * but with a name - it is not
- * restriction. Nullability is necessary
- * if a field is absend in some tuples.
+ * The field can get a name added
+ * for it, and this doesn't require a data
+ * check.
+ * If the field is defined as not
+ * nullable, however, we need a data
+ * check, since old data may contain
+ * NULLs or miss the subject field.
*/
if (field1->type == FIELD_TYPE_ANY &&
field1->is_nullable)
@@ -310,7 +313,10 @@ tuple_format1_can_store_format2_tuples(const struct tuple_format *format1,
const struct tuple_field *field2 = &format2->fields[i];
if (! field_type1_contains_type2(field1->type, field2->type))
return false;
- /* Nullability removal - format is restricted. */
+ /*
+ * Do not allow transition from nullable to non-nullable:
+ * it would require a check of all data in the space.
+ */
if (field2->is_nullable && !field1->is_nullable)
return false;
}
diff --git a/src/box/tuple_format.h b/src/box/tuple_format.h
index c047cdb655..77c8e404f3 100644
--- a/src/box/tuple_format.h
+++ b/src/box/tuple_format.h
@@ -200,14 +200,13 @@ tuple_format_new(struct tuple_format_vtab *vtab, struct key_def * const *keys,
uint32_t space_field_count, struct tuple_dictionary *dict);
/**
- * Check, if @a format1 can store ANY!!! tuples of @a format2. For
- * example, if a field is not nullable in the format1 and the same
- * field is nullable in the format2, or the field type is integer
- * in the format1 and unsigned in the format2, then the format1
- * can not store the format2 tuples.
- * @param format1 Tuple format, that possibly can store tuples of
- * @a format2.
- * @param format2 Tuple format 2.
+ * Check, if @a format1 can store any tuples of @a format2. For
+ * example, if a field is not nullable in format1 and the same
+ * field is nullable in format2, or the field type is integer
+ * in format1 and unsigned in format2, then format1 can not store
+ * format2 tuples.
+ * @param format1 tuple format to check for compatibility of
+ * @param format2 tuple format to check compatibility with
*
* @retval True, if @a format1 can store any tuples of @a format2.
*/
diff --git a/src/box/vinyl.c b/src/box/vinyl.c
index 2dc11170d3..69db2c81d3 100644
--- a/src/box/vinyl.c
+++ b/src/box/vinyl.c
@@ -1068,7 +1068,7 @@ vinyl_space_prepare_alter(struct space *old_space, struct space *new_space)
if (! tuple_format1_can_store_format2_tuples(new_space->format,
old_space->format)) {
diag_set(ClientError, ER_UNSUPPORTED, "Vinyl",
- "non-empty space format incompatible change");
+ "changing space format of a non-empty space");
return -1;
}
return 0;
diff --git a/test/box/alter.result b/test/box/alter.result
index eaf79f1d0a..00e1cb7912 100644
--- a/test/box/alter.result
+++ b/test/box/alter.result
@@ -1565,7 +1565,7 @@ format[2] = {name = 'field2', type = 'unsigned'}
...
s:format(format)
---
-- error: Vinyl does not support non-empty space format incompatible change
+- error: Vinyl does not support changing space format of a non-empty space
...
s:drop()
---
diff --git a/test/vinyl/ddl.result b/test/vinyl/ddl.result
index a2c68e428c..6f393c69e6 100644
--- a/test/vinyl/ddl.result
+++ b/test/vinyl/ddl.result
@@ -770,7 +770,7 @@ format[2].is_nullable = false
...
space:format(format)
---
-- error: Vinyl does not support non-empty space format incompatible change
+- error: Vinyl does not support changing space format of a non-empty space
...
space:drop()
---
--
GitLab