[patches] [PATCH 4/4] Allow to do not specify tail nullable index columns

[Vladimir Davydov]

On Tue, Feb 13, 2018 at 06:28:38PM +0300, Vladislav Shpilevoy wrote:
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index c48e89f9e..cb3b8353d 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -323,7 +323,39 @@ index_def_new_from_tuple(struct tuple *tuple, struct space *space)
>  					     space->def->field_count) != 0)
>  			diag_raise();
>  	}
> -	key_def = key_def_new_with_parts(part_def, part_count);
> +	struct space_def *sd = space->def;
> +	int unchanged_index_count = 0;
> +	struct key_def **keys = NULL;
> +	/*
> +	 * To detect which key parts are optional, min_field_count
> +	 * is required. But min_field_count from the old space
> +	 * format can not be used. For example, consider the case,
> +	 * when a space has no format, has a primary index on the
> +	 * first field and has a single secondary index on a
> +	 * non-nullable second field. Min field count here is 2.
> +	 * Now alter the secondary index to make its part be
> +	 * nullable. The 'space' variable here is the old space,
> +	 * where min_field_count is still 2, but actually it is
> +	 * already 1. Actual min_field_count must be calculated
> +	 * using old unchanged indexes, NEW definition of an
> +	 * updated index and a space format, defined by a user.
> +	 */
> +	for (uint32_t i = 0; i < space->index_count; ++i) {
> +		if (space->index[i]->def->iid != index_id)
> +			++unchanged_index_count;
> +	}
> +	if (unchanged_index_count > 0) {
> +		size_t bsize = unchanged_index_count * sizeof(keys[0]);
> +		keys = (struct key_def **) region_alloc_xc(&fiber()->gc, bsize);
> +		for (uint32_t i = 0, j = 0; i < space->index_count; ++i) {
> +			if (space->index[i]->def->iid != index_id)
> +				keys[j++] = space->index[i]->def->key_def;
> +		}
> +	}
> +	uint32_t min_field_count =
> +		tuple_format_min_field_count(keys, unchanged_index_count,
> +					     sd->fields, sd->field_count);
> +	key_def = key_def_new_with_parts(part_def, part_count, min_field_count);

What will happen if there are two indexes over the same field, nullable
and not nullable, and the one which is not nullable is set to be
nullable? I don't see where you update is_optional then.

AFAIU is_optional should be updated for all indexes in alter_space_do()
or in AlterSpaceOps, not here.

> diff --git a/src/box/key_def.h b/src/box/key_def.h
> index 96f4cdbf9..a7d6eedfb 100644
> --- a/src/box/key_def.h
> +++ b/src/box/key_def.h
> @@ -72,6 +72,8 @@ struct key_part {
>  	struct coll *coll;
>  	/** True if a part can store NULLs. */
>  	bool is_nullable;
> +	/** True, if a part can absense in a tuple. */

'absence' is always a noun, never a verb.
Fix your comments please.

> +	bool is_optional;
>  };
>  
>  struct key_def;
> @@ -126,6 +128,11 @@ struct key_def {
>  	uint32_t unique_part_count;
>  	/** True, if at least one part can store NULL. */
>  	bool is_nullable;
> +	/**
> +	 * True, if some key parts can absense in a tuple. These
> +	 * fields assumed to be MP_NIL.
> +	 */
> +	bool is_optional;

You add is_optional to both key_def and key_part, but I fail to
understand why you need them. You can detect if a field is missing
in a tuple without any flags, right? So why do you need them?

> diff --git a/src/box/tuple_compare.cc b/src/box/tuple_compare.cc
> index dcfb4f636..10c00a6f4 100644
> --- a/src/box/tuple_compare.cc
> +++ b/src/box/tuple_compare.cc
> @@ -589,18 +616,12 @@ tuple_compare_with_key_slowpath(const struct tuple *tuple, const char *key,
>  	return 0;
>  }
>  
> -/**
> - * is_sequential_nullable_tuples used to determine if this
> - * function is used to compare two tuples, which keys are
> - * sequential. In such a case it is necessary to compare them
> - * using primary parts if a NULL was met.
> - */
> -template<bool is_nullable, bool is_sequential_nullable_tuples>
> +template<bool is_nullable>
>  static inline int
>  key_compare_parts(const char *key_a, const char *key_b, uint32_t part_count,
>  		  const struct key_def *key_def)

To ease review, removal of is_sequential_nullable_tuples template
parameter should be done in a separate patch.

> -static int
> -tuple_compare_sequential_nullable(const struct tuple *tuple_a,
> -				  const struct tuple *tuple_b,
> -				  const struct key_def *key_def)

Turning tuple_compare_sequential_nullable into a template parameter
should be done in a separate patch. Folding it in the patch removing
is_sequential_nullable_tuples (see above) would be OK.