Hi!

Thanks for explanation! 
The patch is still LGTM.

Sergos 



Sent from Mail.ru app for iOS


Wednesday, 30 October 2019, 20:08 +0300 from Ilya Kosarev  <i.kosarev@tarantool.org>:
>Hi!
>
>Thanks for your review.
>We are using diag_raise here as far as index_def_check_sequence is already refactored and we need to process the error it returns, while sequence_field_from_tuple itself is not being refactored in this patch, so we can't return an error here, only raise an exception.
>
>Sincerely,
>Ilya Kosarev
>
>>Среда, 30 октября 2019, 11:59 +03:00 от Sergey Ostanevich < sergos@tarantool.org >:
>>
>>Hi!
>>
>>LGTM, just a few nits in comments below
>>
>>> index 8d565e189..10a5f8d64 100644
>>> --- a/src/box/alter.cc
>>> +++ b/src/box/alter.cc
>>> @@ -118,10 +118,10 @@ access_check_ddl(const char *name, uint32_t object_id, uint32_t owner_uid,
>>>  }
>>> 
>>>  /**
>>> - * Throw an exception if the given index definition
>>> + * Return an error if the given index definition
>>>   * is incompatible with a sequence.
>>>   */
>>> -static void
>>> +static int
>>>  index_def_check_sequence(struct index_def *index_def, uint32_t sequence_fieldno,
>>>  			 const char *sequence_path, uint32_t sequence_path_len,
>>>  			 const char *space_name)
>>> @@ -142,24 +142,27 @@ index_def_check_sequence(struct index_def *index_def, uint32_t sequence_fieldno,
>>>  		}
>>>  	}
>>>  	if (sequence_part == NULL) {
>>> -		tnt_raise(ClientError, ER_MODIFY_INDEX, index_def->name,
>>> -			  space_name, "sequence field must be a part of "
>>> -			  "the index");
>>> +		diag_set(ClientError, ER_MODIFY_INDEX, index_def->name,
>>> +			 space_name, "sequence field must be a part of "
>>> +				     "the index");
>>> +		return -1;
>>>  	}
>>>  	enum field_type type = sequence_part->type;
>>>  	if (type != FIELD_TYPE_UNSIGNED && type != FIELD_TYPE_INTEGER) {
>>> -		tnt_raise(ClientError, ER_MODIFY_INDEX, index_def->name,
>>> -			  space_name, "sequence cannot be used with "
>>> -			  "a non-integer key");
>>> +		diag_set(ClientError, ER_MODIFY_INDEX, index_def->name,
>>> +			 space_name, "sequence cannot be used with "
>>> +				     "a non-integer key");
>>> +		return -1;
>>>  	}
>>> +	return 0;
>>>  }
>>> 
>>>  /**
>>>   * Support function for index_def_new_from_tuple(..)
>>> - * Checks tuple (of _index space) and throws a nice error if it is invalid
>>> + * Checks tuple (of _index space) and sets a nice diag if it is invalid
>>Should be 'Return an error', same as for the case above.
>>
>>> @@ -4066,8 +4099,9 @@ sequence_field_from_tuple(struct space *space, struct tuple *tuple,
>>>  		if (path_len == 0)
>>>  			path_raw = NULL;
>>>  	}
>>> -	index_def_check_sequence(pk->def, fieldno, path_raw, path_len,
>>> -				 space_name(space));
>>> +	if (index_def_check_sequence(pk->def, fieldno, path_raw, path_len,
>>> +				 space_name(space)) != 0)
>>> +		diag_raise();
>>
>>I wonder why we leave some diag_raise() and not diag_set() plus return
>>-1 or NULL?
>>
>>
>>regards,
>>Sergos
>>
>
>
>-- 
>Ilya Kosarev