<HTML><BODY><div id="composeWebView_editable_content" data-mailruapp-compose-id="composeWebView_editable_content" style="text-align: left;"><div>Hi!</div><div><br></div><div>Thanks for explanation! </div><div>The patch is still LGTM.</div><div><br></div><div>Sergos </div><div><br></div><div id="mail-app-auto-default-signature"><br><br>Sent from Mail.ru app for iOS<br></div><br><br>Wednesday, 30 October 2019, 20:08 +0300 from Ilya Kosarev <i.kosarev@tarantool.org>:<br><div id="composeWebView_previouse_content" data-mailruapp-compose-id="composeWebView_previouse_content"><blockquote id="mail-app-auto-quote" style="border-left-width: 1px; border-left-style: solid; border-left-color: rgb(11, 102, 249); margin: 10px 0px 10px 5px; padding: 0px 0px 0px 10px; display: inherit;" data-darkosha-id="1:1"><div class="js-helper js-readmsg-msg" data-darkosha-id="1:2" style="">
<style type="text/css" data-darkosha-id="1:3" style=""></style>
<div data-darkosha-id="1:4" style="">
<base target="_self" href="https://e.mail.ru/" data-darkosha-id="1:5" style="">
<div id="style_15724553081270395498_BODY" data-darkosha-id="1:6" style=""><div class="class_1572470285" data-darkosha-id="1:7" style="">
<p style="font-family: Arial, Tahoma, Verdana, sans-serif;" data-mce-style="font-family: Arial, Tahoma, Verdana, sans-serif;" data-darkosha-id="1:8">Hi!<br data-darkosha-id="1:9" style=""><br data-darkosha-id="1:10" style="">Thanks for your review.</p><p style="font-family: Arial, Tahoma, Verdana, sans-serif;" data-mce-style="font-family: Arial, Tahoma, Verdana, sans-serif;" data-darkosha-id="1:11">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;" data-darkosha-id="1:12"><span style="color: rgb(34, 34, 34); font-family: Rubik, Arial, sans-serif; font-size: 17px;" data-mce-style="color: #222222;font-family: Rubik, Arial, sans-serif;font-size: 17px;" data-darkosha-id="1:13"><br data-darkosha-id="1:14" style=""></span>Sincerely,<br data-darkosha-id="1:15" style="">Ilya Kosarev</p><br data-darkosha-id="1:16" style=""><br data-darkosha-id="1:17" style=""><blockquote style="border-left-width: 1px; border-left-style: solid; border-left-color: rgb(8, 87, 166); margin: 10px 0px 10px 10px; padding: 0px 0px 0px 10px;" data-darkosha-id="1:18">
Среда, 30 октября 2019, 11:59 +03:00 от Sergey Ostanevich <<a href="mailto:sergos@tarantool.org" data-darkosha-id="1:19" style="">sergos@tarantool.org</a>>:<br data-darkosha-id="1:20" style="">
<br data-darkosha-id="1:21" style="">
<div id="" data-darkosha-id="1:22" style="">
<div class="js-helper_mailru_css_attribute_postfix js-readmsg-msg_mailru_css_attribute_postfix" data-darkosha-id="1:23" style="">
<style data-darkosha-id="1:24" style=""></style>
<div data-darkosha-id="1:25" style="">
<div id="style_15724259831461674275_BODY_mailru_css_attribute_postfix" data-darkosha-id="1:26" style="">Hi!<br data-darkosha-id="1:27" style="">
<br data-darkosha-id="1:28" style="">
LGTM, just a few nits in comments below<br data-darkosha-id="1:29" style="">
<br data-darkosha-id="1:30" style="">
> index 8d565e189..10a5f8d64 100644<br data-darkosha-id="1:31" style="">
> --- a/src/box/alter.cc<br data-darkosha-id="1:32" style="">
> +++ b/src/box/alter.cc<br data-darkosha-id="1:33" style="">
> @@ -118,10 +118,10 @@ access_check_ddl(const char *name, uint32_t object_id, uint32_t owner_uid,<br data-darkosha-id="1:34" style="">
> }<br data-darkosha-id="1:35" style="">
> <br data-darkosha-id="1:36" style="">
> /**<br data-darkosha-id="1:37" style="">
> - * Throw an exception if the given index definition<br data-darkosha-id="1:38" style="">
> + * Return an error if the given index definition<br data-darkosha-id="1:39" style="">
> * is incompatible with a sequence.<br data-darkosha-id="1:40" style="">
> */<br data-darkosha-id="1:41" style="">
> -static void<br data-darkosha-id="1:42" style="">
> +static int<br data-darkosha-id="1:43" style="">
> index_def_check_sequence(struct index_def *index_def, uint32_t sequence_fieldno,<br data-darkosha-id="1:44" style="">
> const char *sequence_path, uint32_t sequence_path_len,<br data-darkosha-id="1:45" style="">
> const char *space_name)<br data-darkosha-id="1:46" style="">
> @@ -142,24 +142,27 @@ index_def_check_sequence(struct index_def *index_def, uint32_t sequence_fieldno,<br data-darkosha-id="1:47" style="">
> }<br data-darkosha-id="1:48" style="">
> }<br data-darkosha-id="1:49" style="">
> if (sequence_part == NULL) {<br data-darkosha-id="1:50" style="">
> - tnt_raise(ClientError, ER_MODIFY_INDEX, index_def->name,<br data-darkosha-id="1:51" style="">
> - space_name, "sequence field must be a part of "<br data-darkosha-id="1:52" style="">
> - "the index");<br data-darkosha-id="1:53" style="">
> + diag_set(ClientError, ER_MODIFY_INDEX, index_def->name,<br data-darkosha-id="1:54" style="">
> + space_name, "sequence field must be a part of "<br data-darkosha-id="1:55" style="">
> + "the index");<br data-darkosha-id="1:56" style="">
> + return -1;<br data-darkosha-id="1:57" style="">
> }<br data-darkosha-id="1:58" style="">
> enum field_type type = sequence_part->type;<br data-darkosha-id="1:59" style="">
> if (type != FIELD_TYPE_UNSIGNED && type != FIELD_TYPE_INTEGER) {<br data-darkosha-id="1:60" style="">
> - tnt_raise(ClientError, ER_MODIFY_INDEX, index_def->name,<br data-darkosha-id="1:61" style="">
> - space_name, "sequence cannot be used with "<br data-darkosha-id="1:62" style="">
> - "a non-integer key");<br data-darkosha-id="1:63" style="">
> + diag_set(ClientError, ER_MODIFY_INDEX, index_def->name,<br data-darkosha-id="1:64" style="">
> + space_name, "sequence cannot be used with "<br data-darkosha-id="1:65" style="">
> + "a non-integer key");<br data-darkosha-id="1:66" style="">
> + return -1;<br data-darkosha-id="1:67" style="">
> }<br data-darkosha-id="1:68" style="">
> + return 0;<br data-darkosha-id="1:69" style="">
> }<br data-darkosha-id="1:70" style="">
> <br data-darkosha-id="1:71" style="">
> /**<br data-darkosha-id="1:72" style="">
> * Support function for index_def_new_from_tuple(..)<br data-darkosha-id="1:73" style="">
> - * Checks tuple (of _index space) and throws a nice error if it is invalid<br data-darkosha-id="1:74" style="">
> + * Checks tuple (of _index space) and sets a nice diag if it is invalid<br data-darkosha-id="1:75" style="">
Should be 'Return an error', same as for the case above.<br data-darkosha-id="1:76" style="">
<br data-darkosha-id="1:77" style="">
> @@ -4066,8 +4099,9 @@ sequence_field_from_tuple(struct space *space, struct tuple *tuple,<br data-darkosha-id="1:78" style="">
> if (path_len == 0)<br data-darkosha-id="1:79" style="">
> path_raw = NULL;<br data-darkosha-id="1:80" style="">
> }<br data-darkosha-id="1:81" style="">
> - index_def_check_sequence(pk->def, fieldno, path_raw, path_len,<br data-darkosha-id="1:82" style="">
> - space_name(space));<br data-darkosha-id="1:83" style="">
> + if (index_def_check_sequence(pk->def, fieldno, path_raw, path_len,<br data-darkosha-id="1:84" style="">
> + space_name(space)) != 0)<br data-darkosha-id="1:85" style="">
> + diag_raise();<br data-darkosha-id="1:86" style="">
<br data-darkosha-id="1:87" style="">
I wonder why we leave some diag_raise() and not diag_set() plus return<br data-darkosha-id="1:88" style="">
-1 or NULL?<br data-darkosha-id="1:89" style="">
<br data-darkosha-id="1:90" style="">
<br data-darkosha-id="1:91" style="">
regards,<br data-darkosha-id="1:92" style="">
Sergos<br data-darkosha-id="1:93" style="">
<br data-darkosha-id="1:94" style="">
</div>
</div>
</div>
</div>
</blockquote>
<br data-darkosha-id="1:95" style="">
<br data-darkosha-id="1:96" style="">-- <br data-darkosha-id="1:97" style="">Ilya Kosarev<br data-darkosha-id="1:98" style="">
</div></div>
<base target="_self" href="https://e.mail.ru/" data-darkosha-id="1:99" style="">
</div>
</div></blockquote></div></div></BODY></HTML>