[patches] [PATCH 1/4] memtx: check for new optional fields on format update

Vladimir Davydov vdavydov.dev at gmail.com
Wed Mar 14 12:01:51 MSK 2018


On Mon, Mar 12, 2018 at 05:32:14PM +0300, v.shpilevoy at tarantool.org wrote:
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 8a62d8ed8..22eecd8a5 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -1584,7 +1584,7 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
>                  * 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.
> +                * comparators must be updated.
>                  */
>                 struct key_def **keys;
>                 size_t bsize = old_space->index_count * sizeof(keys[0]);
> 
> > 
> > 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...
> 
> No, this logic can not be moved into alter_space_do or another common
> alter function, because 1) optionality update must be done before
> alter_space_move_indexes, in which new min field count is used to
> decide, do we need move an index, or modify it. Alter_def is later; 2)
> alter_space_do can not do optionality update, because it would be
> encapsulation violation. Alter space do just executes a sequence of
> virtual functions. It must not be updated, when _sequence, _collation
> and other system spaces are changed.

OK, I see. Now I'm inclined to think that it was a mistake to use a
template for has_optional_parts in the first place. It saves us just a
couple of ifs for nullable indexes - nobody even knows what the
performance gain is, because no tests have been run. At the same time it
makes format and key_def dependent on each other - now you have to
update comparators of all indexes when you update the space format.

Kostja, I think we should remove has_optional_parts template parameter
altogether and use is_nullable instead (see tuple_compare.cc). IMO it
just isn't worth the code complexity it introduces.



More information about the Tarantool-patches mailing list