From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 3031F272E2 for ; Fri, 6 Jul 2018 11:15:20 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id dQ4k7k6pL71L for ; Fri, 6 Jul 2018 11:15:20 -0400 (EDT) Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 5845D2178B for ; Fri, 6 Jul 2018 11:15:19 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH] Do not update schema_version on space:truncate(). References: <20180706114625.10152-1-sergepetrenko@tarantool.org> <20180706140754.GA3514@chai> <7de60a16-1273-6c1b-12d5-8e05ef1e3ff2@tarantool.org> <20180706142906.GA10648@chai> From: Sergey Petrenko Message-ID: Date: Fri, 6 Jul 2018 18:15:17 +0300 MIME-Version: 1.0 In-Reply-To: <20180706142906.GA10648@chai> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: ru Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Konstantin Osipov Cc: tarantool-patches@freelists.org 06.07.2018 17:29, Konstantin Osipov пишет: > * Sergey Petrenko [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)