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

[v.shpilevoy at tarantool.org]

All your remarks are fixed. See below for details.

> 17 февр. 2018 г., в 23:19, Vladimir Davydov <vdavydov.dev at gmail.com> написал(а):
> 
> 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.

Fixed. Now ModifyIndex is used instead of MoveIndex when is_optional is updated in an index by alteration of another index. Is_optional is calculated in on_replace_dd_index only.

> 
>> 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.

Fixed here and in other places.

> 
>> +	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?

I decided to leave this flag for assertions. With no this flag a crash from your next letter would not occurred.

> 
>> 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.

Done.

> 
>> -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.

Done.