[tarantool-patches] Re: [PATCH] Do not update schema_version on space:truncate().

[Sergey Petrenko <sergepetrenko@tarantool.org>]

06.07.2018 17:29, Konstantin Osipov пишет:
> * Sergey Petrenko <sergepetrenko@tarantool.org> [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)