<HTML><BODY><p style="font-family: Arial, Tahoma, Verdana, sans-serif;" data-mce-style="font-family: Arial, Tahoma, Verdana, sans-serif;">Hi!<br><br>Thanks for your review.</p><p style="font-family: Arial, Tahoma, Verdana, sans-serif;" data-mce-style="font-family: Arial, Tahoma, Verdana, sans-serif;">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.</p><p style="font-family: Arial, Tahoma, Verdana, sans-serif;" data-mce-style="font-family: Arial, Tahoma, Verdana, sans-serif;"><span style="color: #222222;font-family: Rubik, Arial, sans-serif;font-size: 17px;" data-mce-style="color: #222222; font-family: Rubik, Arial, sans-serif; font-size: 17px;"><br></span>Sincerely,<br>Ilya Kosarev</p><br><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">
Среда, 30 октября 2019, 11:59 +03:00 от Sergey Ostanevich <sergos@tarantool.org>:<br>
<br>
<div id="">
<div class="js-helper js-readmsg-msg">
<style type="text/css"></style>
<div>
<div id="style_15724259831461674275_BODY">Hi!<br>
<br>
LGTM, just a few nits in comments below<br>
<br>
> index 8d565e189..10a5f8d64 100644<br>
> --- a/src/box/alter.cc<br>
> +++ b/src/box/alter.cc<br>
> @@ -118,10 +118,10 @@ access_check_ddl(const char *name, uint32_t object_id, uint32_t owner_uid,<br>
> }<br>
> <br>
> /**<br>
> - * Throw an exception if the given index definition<br>
> + * Return an error if the given index definition<br>
> * is incompatible with a sequence.<br>
> */<br>
> -static void<br>
> +static int<br>
> index_def_check_sequence(struct index_def *index_def, uint32_t sequence_fieldno,<br>
> const char *sequence_path, uint32_t sequence_path_len,<br>
> const char *space_name)<br>
> @@ -142,24 +142,27 @@ index_def_check_sequence(struct index_def *index_def, uint32_t sequence_fieldno,<br>
> }<br>
> }<br>
> if (sequence_part == NULL) {<br>
> - tnt_raise(ClientError, ER_MODIFY_INDEX, index_def->name,<br>
> - space_name, "sequence field must be a part of "<br>
> - "the index");<br>
> + diag_set(ClientError, ER_MODIFY_INDEX, index_def->name,<br>
> + space_name, "sequence field must be a part of "<br>
> + "the index");<br>
> + return -1;<br>
> }<br>
> enum field_type type = sequence_part->type;<br>
> if (type != FIELD_TYPE_UNSIGNED && type != FIELD_TYPE_INTEGER) {<br>
> - tnt_raise(ClientError, ER_MODIFY_INDEX, index_def->name,<br>
> - space_name, "sequence cannot be used with "<br>
> - "a non-integer key");<br>
> + diag_set(ClientError, ER_MODIFY_INDEX, index_def->name,<br>
> + space_name, "sequence cannot be used with "<br>
> + "a non-integer key");<br>
> + return -1;<br>
> }<br>
> + return 0;<br>
> }<br>
> <br>
> /**<br>
> * Support function for index_def_new_from_tuple(..)<br>
> - * Checks tuple (of _index space) and throws a nice error if it is invalid<br>
> + * Checks tuple (of _index space) and sets a nice diag if it is invalid<br>
Should be 'Return an error', same as for the case above.<br>
<br>
> @@ -4066,8 +4099,9 @@ sequence_field_from_tuple(struct space *space, struct tuple *tuple,<br>
> if (path_len == 0)<br>
> path_raw = NULL;<br>
> }<br>
> - index_def_check_sequence(pk->def, fieldno, path_raw, path_len,<br>
> - space_name(space));<br>
> + if (index_def_check_sequence(pk->def, fieldno, path_raw, path_len,<br>
> + space_name(space)) != 0)<br>
> + diag_raise();<br>
<br>
I wonder why we leave some diag_raise() and not diag_set() plus return<br>
-1 or NULL?<br>
<br>
<br>
regards,<br>
Sergos<br>
<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
<br>-- <br>Ilya Kosarev<br></BODY></HTML>