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