[Tarantool-patches] [tarantool-patches] [PATCH v4 07/20] refactoring: remove exceptions from index_def_new_from_tuple
Sergey Ostanevich
sergos at tarantool.org
Thu Oct 31 08:02:05 MSK 2019
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 at 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 at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20191031/fd15916c/attachment.html>
More information about the Tarantool-patches
mailing list