[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