<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On 26 Feb 2019, at 18:36, Imeev Mergen <<a href="mailto:imeevma@tarantool.org" class="">imeevma@tarantool.org</a>> wrote:</div><div class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">Hi! Thank you for review! My answers below.</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">On 2/26/19 5:47 PM, n.pettik wrote:</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><blockquote type="cite" class="">diff --git a/src/box/sql.c b/src/box/sql.c<br class="">index 580f3fa..116e3e8 100644<br class="">--- a/src/box/sql.c<br class="">+++ b/src/box/sql.c<br class="">@@ -1362,13 +1362,8 @@ sql_checks_resolve_space_def_reference(ExprList *expr_list,<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>parser.parse_only = true;<br class=""><br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>sql_resolve_self_reference(&parser, def, NC_IsCheck, NULL, expr_list);<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span>int rc = 0;<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span>if (parser.rc != SQL_OK) {<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>/* Tarantool error may be already set with diag. */<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>if (parser.rc != SQL_TARANTOOL_ERROR)<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>diag_set(ClientError, ER_SQL, parser.zErrMsg);<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>rc = -1;<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span>}<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span>if (parser.rc != SQL_OK)<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>return -1;<br class=""></blockquote>Since now we have only one possible RC, lets remove<br class="">its name and simply check (parser.rc != 0).<br class="">Or, as suggested Konstantin, better replace rc with bool is_aborted flag.<br class=""></blockquote><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">Do you mind if I do this in a new patch?</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""></div></blockquote><div><br class=""></div><div>Sure, but place it before this one.</div><br class=""><blockquote type="cite" class=""><div class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><blockquote type="cite" class="">diff --git a/src/box/sql/build.c b/src/box/sql/build.c<br class="">index deb5b89..6afca4a 100644<br class="">--- a/src/box/sql/build.c<br class="">+++ b/src/box/sql/build.c<br class="">@@ -493,16 +493,10 @@ sql_column_add_nullable_action(struct Parse *parser,<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>struct field_def *field = &def->fields[def->field_count - 1];<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>if (field->nullable_action != ON_CONFLICT_ACTION_DEFAULT &&<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-converted-space"> </span> nullable_action != field->nullable_action) {<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>/* Prevent defining nullable_action many times. */<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>const char *err_msg =<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>tt_sprintf("NULL declaration for column '%s' of table "<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-converted-space"> </span> "'%s' has been already set to '%s'",<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-converted-space"> </span> field->name, def->name,<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-converted-space"> </span> on_conflict_action_strs[field-><br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-converted-space"> </span> nullable_action]);<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>diag_set(ClientError, ER_SQL, err_msg);<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>parser->rc = SQL_TARANTOOL_ERROR;<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>parser->nErr++;<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>sqlErrorMsg(parser, "NULL declaration for column '%s' of "\<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-converted-space"> </span> "table '%s' has been already set to '%s'",<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-converted-space"> </span> field->name, def->name,<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-converted-space"> </span> on_conflict_action_strs[field-> nullable_action]);<br class=""></blockquote>This looks like step back in our attempt at using diag_set.<br class="">We do you need to incapsulate diag into sqlErrorMsg?<br class=""></blockquote><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">I thought that it was good idea to incapsulate all SQL errors that</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""></div></blockquote><div><br class=""></div><div>No, it wasn’t.</div><br class=""><blockquote type="cite" class=""><div class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">cannot be fixed in just parse_context->rc = SQL_TARANTOOL_ERROR</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">and diag_set(). Here it uses tt_printf() in addition to these two</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">commands.</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""></div></blockquote><div><br class=""></div><div>It’s OK. Revert this (and other) change(s), please. Lets use pure</div><div>diag_set + abort flag.</div><br class=""><blockquote type="cite" class=""><div class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><blockquote type="cite" class=""><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>return;<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>}<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>field->nullable_action = nullable_action;<br class="">diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c<br class="">index 5170c7f..a7bf3b3 100644<br class="">--- a/src/box/sql/delete.c<br class="">+++ b/src/box/sql/delete.c<br class="">@@ -94,31 +94,22 @@ sql_table_truncate(struct Parse *parse, struct SrcList *tab_list)<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>struct space *space = space_by_name(tab_name);<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>if (space == NULL) {<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>diag_set(ClientError, ER_NO_SUCH_SPACE, tab_name);<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>goto tarantool_error;<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>sql_parser_error(parse);<br class=""></blockquote>Look, anyway you remove this function in next commit.<br class="">Next time please consider order of refactoring.<br class=""></blockquote><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">You are right, I will return to what was before and refactor in</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">the next commit.</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""></div></blockquote><div><br class=""></div><div>Don’t waste time now, it was just advice to keep in mind.</div><br class=""><blockquote type="cite" class=""><div class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><blockquote type="cite" class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>}<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>if (! rlist_empty(&space->parent_fk_constraint)) {<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>const char *err_msg =<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>tt_sprintf("can not truncate space '%s' because other "<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-converted-space"> </span> "objects depend on it", space->def->name);<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>diag_set(ClientError, ER_SQL, err_msg);<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>goto tarantool_error;<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>sqlErrorMsg(parse, "can not truncate space '%s' because other "<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-converted-space"> </span> "objects depend on it", space->def->name);<br class=""></blockquote>Replace invocation of sqlErrorMsg with diag_set + parser->rc.<br class="">The same in other places.<br class=""></blockquote><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">Answer is the same as in second question.</span><br style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; text-decoration: none;" class=""><br class=""><blockquote type="cite" class="">@@ -146,11 +145,7 @@ sqlPrepare(sql * db,<span class="Apple-tab-span" style="white-space: pre;"> </span>/* Database handle. */<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>*ppStmt = (sql_stmt *) sParse.pVdbe;<br class=""><span class="Apple-tab-span" style="white-space: pre;"> </span>}<br class=""><br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span>if (zErrMsg) {<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>sqlErrorWithMsg(db, rc, "%s", zErrMsg);<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span>} else {<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span><span class="Apple-tab-span" style="white-space: pre;"> </span>sqlError(db, rc);<br class="">-<span class="Apple-tab-span" style="white-space: pre;"> </span>}<br class="">+<span class="Apple-tab-span" style="white-space: pre;"> </span>sqlError(db, rc);<br class=""></blockquote>sqlError seems to be useless/dead. Please, make a note somewhere<br class="">to remove it as follow-up to error-refactoring patch-set.<br class=""></blockquote><span style="caret-color: rgb(0, 0, 0); font-family: Helvetica; font-size: 12px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; text-decoration: none; float: none; display: inline !important;" class="">Do you mind if I do this in a new patch?</span></div></blockquote><br class=""></div><div>You don’t have to do it right now, it can wait till 2.2</div><div>Just file this issue somewhere.</div><br class=""></body></html>