[patches] [PATCH 1/4] memtx: check for new optional fields on format update
Vladimir Davydov
vdavydov.dev at gmail.com
Mon Mar 12 15:35:47 MSK 2018
On Mon, Mar 12, 2018 at 12:53:33AM +0300, Vladislav Shpilevoy wrote:
> When a space format is updated, a new min field count must be
> calculated before a new format construction to check that some
> of fields became optional.
>
> Part of #3229
>
> Signed-off-by: Vladislav Shpilevoy <v.shpilevoy at tarantool.org>
> ---
> src/box/alter.cc | 19 ++++++++++++
> test/engine/ddl.result | 79 ++++++++++++++++++++++++++++++++++++++++++++++++
> test/engine/ddl.test.lua | 26 ++++++++++++++++
> 3 files changed, 124 insertions(+)
>
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 5749740d2..8a62d8ed8 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -1578,6 +1578,25 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
> struct alter_space *alter = alter_space_new(old_space);
> auto alter_guard =
> make_scoped_guard([=] {alter_space_delete(alter);});
> + /*
> + * Calculate a new min_field_count. It can be
> + * changed by resetting space:format(), if an old
> + * format covers some nullable indexed fields in
> + * the format tail. And when the format is reset,
> + * these fields become optional - index
> + * comparators must be updated. See gh-3229.
> + */
Please don't mention GitHub tickets in the source code (tests are OK)
unless absolutely necessary. The code should be self-explanatory as is
and shouldn't require any references to external resources.
> + struct key_def **keys;
> + size_t bsize = old_space->index_count * sizeof(keys[0]);
> + keys = (struct key_def **) region_alloc_xc(&fiber()->gc,
> + bsize);
> + for (uint32_t i = 0; i < old_space->index_count; ++i)
> + keys[i] = old_space->index[i]->def->key_def;
> + alter->new_min_field_count =
> + tuple_format_min_field_count(keys,
> + old_space->index_count,
> + def->fields,
> + def->field_count);
We have a similar logic in on_replace_dd_index(). Can't we move it,
along with all those numerous calls to index_def_update_optionality()
spread over alter.cc, to a single place to avoid code duplication?
alter_space_do() or alter_def(), may be? AFAIR I suggested to do that
when I reviewed the original patch set, but you just said that you
disagreed without any further elaboration...
More information about the Tarantool-patches
mailing list