[tarantool-patches] Re: [PATCH] Do not update schema_version on space:truncate().
- From: Sergey Petrenko <sergepetrenko@xxxxxxxxxxxxx>
- To: Konstantin Osipov <kostja@xxxxxxxxxxxxx>
- Date: Fri, 6 Jul 2018 18:15:17 +0300
06.07.2018 17:29, Konstantin Osipov пишет:
* Sergey Petrenko <sergepetrenko@xxxxxxxxxxxxx> [18/07/06 17:27]:
Will fix. I thought it'd be a good idea to only increment when we already
know
we won't be returning to the old state.
So I should increase schema_version in UpdateSchemaVersion::alter() and
decrease it in
UpdateSchemaVersion::rollback() ?
Never decrement. Reloading a schema extra time won't break
anything.
And what about creation/drop of a space?
Move
schema_version increment to on_replace_dd_space()
OK.
Fixed. See the new diff below:
diff --git a/src/box/alter.cc b/src/box/alter.cc
index 5dec2519d..ec3b91bff 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1305,6 +1305,27 @@ RebuildIndex::~RebuildIndex()
index_def_delete(new_index_def);
}
+
+/**
+ * UpdateSchemaVersion - increment schema_version. Used on
+ * in alter_space_do(), i.e. when creating or dropping
+ * an index, altering a space.
+ */
+class UpdateSchemaVersion: public AlterSpaceOp
+{
+public:
+ UpdateSchemaVersion(struct alter_space * alter)
+ :AlterSpaceOp(alter) {}
+ virtual void alter(struct alter_space *alter);
+};
+
+void
+UpdateSchemaVersion::alter(struct alter_space *alter)
+{
+ (void)alter;
+ ++schema_version;
+}
+
/* }}} */
/**
@@ -1508,6 +1529,13 @@ on_replace_dd_space(struct trigger * /* trigger
*/, void *event)
* execution on a replica.
*/
(void) space_cache_replace(space);
+ /*
+ * Do not forget to update schema_version right after
+ * inserting the space to the space_cache, since no
+ * AlterSpaceOps are registered in case of space
+ * create.
+ */
+ ++schema_version;
/*
* So may happen that until the DDL change record
* is written to the WAL, the space is used for
@@ -1552,6 +1580,12 @@ on_replace_dd_space(struct trigger * /* trigger
*/, void *event)
* execution on a replica.
*/
struct space *space = space_cache_delete(space_id(old_space));
+ /*
+ * Do not forget to update schema_version right after
+ * deleting the space from the space_cache, since no
+ * AlterSpaceOps are registered in case of space drop.
+ */
+ ++schema_version;
struct trigger *on_commit =
txn_alter_trigger_new(on_drop_space_commit, space);
txn_on_commit(txn, on_commit);
@@ -1603,6 +1637,8 @@ on_replace_dd_space(struct trigger * /* trigger
*/, void *event)
def_guard.is_active = false;
/* Create MoveIndex ops for all space indexes. */
alter_space_move_indexes(alter, 0, old_space->index_id_max + 1);
+ /* Remember to update schema_version. */
+ (void) new UpdateSchemaVersion(alter);
alter_space_do(txn, alter);
alter_guard.is_active = false;
}
@@ -1800,6 +1836,8 @@ on_replace_dd_index(struct trigger * /* trigger
*/, void *event)
* old space.
*/
alter_space_move_indexes(alter, iid + 1, old_space->index_id_max + 1);
+ /* Add an op to update schema_version on commit. */
+ (void) new UpdateSchemaVersion(alter);
alter_space_do(txn, alter);
scoped_guard.is_active = false;
}
diff --git a/src/box/index.cc b/src/box/index.cc
index 69fc76116..30cc0a684 100644
--- a/src/box/index.cc
+++ b/src/box/index.cc
@@ -433,7 +433,7 @@ iterator_create(struct iterator *it, struct index
*index)
{
it->next = NULL;
it->free = NULL;
- it->schema_version = schema_version;
+ it->schema_state = schema_state;
it->space_id = index->def->space_id;
it->index_id = index->def->iid;
it->index = index;
@@ -443,15 +443,15 @@ int
iterator_next(struct iterator *it, struct tuple **ret)
{
assert(it->next != NULL);
- if (unlikely(it->schema_version != schema_version)) {
+ if (unlikely(it->schema_state != schema_state)) {
struct space *space = space_by_id(it->space_id);
if (space == NULL)
goto invalidate;
struct index *index = space_index(space, it->index_id);
if (index != it->index ||
- index->schema_version > it->schema_version)
+ index->schema_state > it->schema_state)
goto invalidate;
- it->schema_version = schema_version;
+ it->schema_state = schema_state;
}
return it->next(it, ret);
@@ -478,7 +478,7 @@ index_create(struct index *index, struct engine *engine,
index->vtab = vtab;
index->engine = engine;
index->def = def;
- index->schema_version = schema_version;
+ index->schema_state = schema_state;
return 0;
}
diff --git a/src/box/index.h b/src/box/index.h
index 3c478c6d6..cc9de57a5 100644
--- a/src/box/index.h
+++ b/src/box/index.h
@@ -233,8 +233,8 @@ struct iterator {
int (*next)(struct iterator *it, struct tuple **ret);
/** Destroy the iterator. */
void (*free)(struct iterator *);
- /** Schema version at the time of the last index lookup. */
- uint32_t schema_version;
+ /** Schema state at the time of the last index lookup. */
+ uint32_t schema_state;
/** ID of the space the iterator is for. */
uint32_t space_id;
/** ID of the index the iterator is for. */
@@ -242,7 +242,7 @@ struct iterator {
/**
* Pointer to the index the iterator is for.
* Guaranteed to be valid only if the schema
- * version has not changed since the last lookup.
+ * state has not changed since the last lookup.
*/
struct index *index;
};
@@ -408,8 +408,8 @@ struct index {
struct engine *engine;
/* Description of a possibly multipart key. */
struct index_def *def;
- /* Schema version at the time of construction. */
- uint32_t schema_version;
+ /* Schema state at the time of construction. */
+ uint32_t schema_state;
};
/**
diff --git a/src/box/schema.cc b/src/box/schema.cc
index 1b96f978c..02b4f54ee 100644
--- a/src/box/schema.cc
+++ b/src/box/schema.cc
@@ -58,7 +58,15 @@ static struct mh_i32ptr_t *spaces;
static struct mh_i32ptr_t *funcs;
static struct mh_strnptr_t *funcs_by_name;
static struct mh_i32ptr_t *sequences;
+/** Public change counter. On its update clients need to fetch
+ * new space data from the instance. */
uint32_t schema_version = 0;
+/**
+ * Internal change counter. Grows faster, than the public one,
+ * because we need to remember when to update pointers to already
+ * non-existent space objects on space:truncate() operation.
+ */
+uint32_t schema_state = 0;
uint32_t dd_version_id = version_id(1, 6, 4);
struct rlist on_alter_space = RLIST_HEAD_INITIALIZER(on_alter_space);
@@ -157,7 +165,7 @@ space_cache_delete(uint32_t id)
assert(k != mh_end(spaces));
struct space *space = (struct space *)mh_i32ptr_node(spaces, k)->val;
mh_i32ptr_del(spaces, k, NULL);
- schema_version++;
+ schema_state++;
return space;
}
@@ -175,7 +183,7 @@ space_cache_replace(struct space *space)
panic_syserror("Out of memory for the data "
"dictionary cache.");
}
- schema_version++;
+ schema_state++;
return p_old ? (struct space *) p_old->val : NULL;
}
diff --git a/src/box/schema.h b/src/box/schema.h
index 56f39b3fe..dcc764aef 100644
--- a/src/box/schema.h
+++ b/src/box/schema.h
@@ -42,6 +42,7 @@ extern "C" {
#endif /* defined(__cplusplus) */
extern uint32_t schema_version;
+extern uint32_t schema_state;
/**
* Persistent version of the schema, stored in _schema["version"].
@@ -68,14 +69,14 @@ box_schema_version();
static inline struct space *
space_cache_find(uint32_t id)
{
- static uint32_t prev_schema_version = 0;
+ static uint32_t prev_schema_state = 0;
static struct space *space = NULL;
- if (prev_schema_version != schema_version)
+ if (prev_schema_state != schema_state)
space = NULL;
if (space && space->def->id == id)
return space;
if ((space = space_by_id(id))) {
- prev_schema_version = schema_version;
+ prev_schema_state = schema_state;
return space;
}
diag_set(ClientError, ER_NO_SUCH_SPACE, int2str(id));
diff --git a/src/box/sysview_index.c b/src/box/sysview_index.c
index 0bec30282..6a2667db2 100644
--- a/src/box/sysview_index.c
+++ b/src/box/sysview_index.c
@@ -66,7 +66,7 @@ sysview_iterator_next(struct iterator *iterator,
struct tuple **ret)
assert(iterator->free == sysview_iterator_free);
struct sysview_iterator *it = sysview_iterator(iterator);
*ret = NULL;
- if (it->source->schema_version != schema_version)
+ if (it->source->schema_state != schema_state)
return 0; /* invalidate iterator */
struct sysview_index *index = (struct sysview_index *)iterator->index;
int rc;
diff --git a/src/box/vy_index.c b/src/box/vy_index.c
index d9160041b..1d92bebd3 100644
--- a/src/box/vy_index.c
+++ b/src/box/vy_index.c
@@ -217,7 +217,7 @@ vy_index_new(struct vy_index_env *index_env, struct
vy_cache_env *cache_env,
index->mem = vy_mem_new(mem_env, *index->env->p_generation,
cmp_def, format, index->mem_format_with_colmask,
- index->upsert_format, schema_version);
+ index->upsert_format, schema_state);
if (index->mem == NULL)
goto fail_mem;
@@ -764,7 +764,7 @@ vy_index_rotate_mem(struct vy_index *index)
mem = vy_mem_new(index->mem->env, *index->env->p_generation,
index->cmp_def, index->mem_format,
index->mem_format_with_colmask,
- index->upsert_format, schema_version);
+ index->upsert_format, schema_state);
if (mem == NULL)
return -1;
diff --git a/src/box/vy_mem.c b/src/box/vy_mem.c
index 629634bbf..d7f62fbba 100644
--- a/src/box/vy_mem.c
+++ b/src/box/vy_mem.c
@@ -98,7 +98,7 @@ struct vy_mem *
vy_mem_new(struct vy_mem_env *env, int64_t generation,
const struct key_def *cmp_def, struct tuple_format *format,
struct tuple_format *format_with_colmask,
- struct tuple_format *upsert_format, uint32_t schema_version)
+ struct tuple_format *upsert_format, uint32_t schema_state)
{
struct vy_mem *index = calloc(1, sizeof(*index));
if (!index) {
@@ -111,7 +111,7 @@ vy_mem_new(struct vy_mem_env *env, int64_t generation,
index->max_lsn = -1;
index->cmp_def = cmp_def;
index->generation = generation;
- index->schema_version = schema_version;
+ index->schema_state = schema_state;
index->format = format;
tuple_format_ref(format);
index->format_with_colmask = format_with_colmask;
diff --git a/src/box/vy_mem.h b/src/box/vy_mem.h
index d60032219..6f4af3c8d 100644
--- a/src/box/vy_mem.h
+++ b/src/box/vy_mem.h
@@ -174,8 +174,8 @@ struct vy_mem {
const struct key_def *cmp_def;
/** version is initially 0 and is incremented on every write */
uint32_t version;
- /** Schema version at the time of creation. */
- uint32_t schema_version;
+ /** Schema state at the time of creation. */
+ uint32_t schema_state;
/**
* Generation of statements stored in the tree.
* Used as lsregion allocator identifier.
@@ -250,7 +250,7 @@ vy_mem_wait_pinned(struct vy_mem *mem)
* @param format_with_colmask Format for tuples, which have
* column mask.
* @param upsert_format Format for UPSERT tuples.
- * @param schema_version Schema version.
+ * @param schema_state Schema state.
* @retval new vy_mem instance on success.
* @retval NULL on error, check diag.
*/
@@ -258,7 +258,7 @@ struct vy_mem *
vy_mem_new(struct vy_mem_env *env, int64_t generation,
const struct key_def *cmp_def, struct tuple_format *format,
struct tuple_format *format_with_colmask,
- struct tuple_format *upsert_format, uint32_t schema_version);
+ struct tuple_format *upsert_format, uint32_t schema_state);
/**
* Delete in-memory level.
diff --git a/src/box/vy_tx.c b/src/box/vy_tx.c
index 01130020b..019320ca4 100644
--- a/src/box/vy_tx.c
+++ b/src/box/vy_tx.c
@@ -45,7 +45,7 @@
#include "iproto_constants.h"
#include "iterator_type.h"
#include "salad/stailq.h"
-#include "schema.h" /* schema_version */
+#include "schema.h" /* schema_state */
#include "trigger.h"
#include "trivia/util.h"
#include "tuple.h"
@@ -414,11 +414,11 @@ vy_tx_write_prepare(struct txv *v)
* In this case we need to dump the tree as is in order to
* guarantee dump consistency.
*
- * - Schema version has increased after the tree was created.
+ * - Schema state has increased after the tree was created.
* We have to seal the tree, because we don't support mixing
* statements of different formats in the same tree.
*/
- if (unlikely(index->mem->schema_version != schema_version ||
+ if (unlikely(index->mem->schema_state != schema_state ||
index->mem->generation != *index->env->p_generation)) {
if (vy_index_rotate_mem(index) != 0)
return -1;
diff --git a/test/engine/ddl.result b/test/engine/ddl.result
index c3cb1efc2..b6b6faf5b 100644
--- a/test/engine/ddl.result
+++ b/test/engine/ddl.result
@@ -388,6 +388,63 @@ s:drop()
---
...
--
+-- gh-3414: do not increase schema_version on space:truncate()
+--
+-- update schema_version on space.create()
+sch_ver = box.internal.schema_version
+---
+...
+v = sch_ver()
+---
+...
+s = box.schema.create_space('test')
+---
+...
+v + 1 == sch_ver()
+---
+- true
+...
+-- update schema_version on space:create_index()
+prim = s:create_index("primary")
+---
+...
+v + 2 == sch_ver()
+---
+- true
+...
+-- do not change schema_version on space.truncate()
+s:truncate()
+---
+...
+v + 2 == sch_ver()
+---
+- true
+...
+-- update schema_version on index.alter()
+prim:alter{name="new_primary"}
+---
+...
+v + 3 == sch_ver()
+---
+- true
+...
+-- update schema_version on index.drop()
+box.schema.index.drop(s.id, 0)
+---
+...
+v + 4 == sch_ver()
+---
+- true
+...
+-- update schema_version on space.drop()
+s:drop()
+---
+...
+v + 5 == sch_ver()
+---
+- true
+...
+--
-- gh-3229: update optionality if a space format is changed too,
-- not only when indexes are updated.
--
diff --git a/test/engine/ddl.test.lua b/test/engine/ddl.test.lua
index 25381a3c8..e73c7dbb0 100644
--- a/test/engine/ddl.test.lua
+++ b/test/engine/ddl.test.lua
@@ -137,6 +137,30 @@ pk = s:create_index('pk')
pk.parts[1].type
s:drop()
+--
+-- gh-3414: do not increase schema_version on space:truncate()
+--
+-- update schema_version on space.create()
+sch_ver = box.internal.schema_version
+v = sch_ver()
+s = box.schema.create_space('test')
+v + 1 == sch_ver()
+-- update schema_version on space:create_index()
+prim = s:create_index("primary")
+v + 2 == sch_ver()
+-- do not change schema_version on space.truncate()
+s:truncate()
+v + 2 == sch_ver()
+-- update schema_version on index.alter()
+prim:alter{name="new_primary"}
+v + 3 == sch_ver()
+-- update schema_version on index.drop()
+box.schema.index.drop(s.id, 0)
+v + 4 == sch_ver()
+-- update schema_version on space.drop()
+s:drop()
+v + 5 == sch_ver()
+
--
-- gh-3229: update optionality if a space format is changed too,
-- not only when indexes are updated.
diff --git a/test/unit/vy_point_lookup.c b/test/unit/vy_point_lookup.c
index 963329c9f..914833e02 100644
--- a/test/unit/vy_point_lookup.c
+++ b/test/unit/vy_point_lookup.c
@@ -12,6 +12,7 @@
#include "identifier.h"
uint32_t schema_version;
+uint32_t schema_state;
static int
write_run(struct vy_run *run, const char *dir_name,
--
2.15.2 (Apple Git-101.1)
Other related posts:
