<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Hello! Thank you for review! My answers and new patch below.<br>
</p>
<br>
<div class="moz-cite-prefix">On 10/03/2018 06:19 PM, Vladislav
Shpilevoy wrote:<br>
</div>
<blockquote type="cite"
cite="mid:8418885d-8812-d7d9-d74c-1808e2937153@tarantool.org">Hi!
Thanks for the patch! See 3 comments below. <br>
<br>
On 28/09/2018 19:14, <a class="moz-txt-link-abbreviated"
href="mailto:imeevma@tarantool.org">imeevma@tarantool.org</a>
wrote: <br>
<blockquote type="cite">In some cases on attempt to insert
inappropriate value in <br>
autoincrement column assertion was thrown. Now it is fixed. <br>
</blockquote>
<br>
1. Firstly, I agree with Kostja - I do not understand why in 'some
<br>
cases' a value becomes not integer when I insert an integer.
Please, <br>
describe here a reason. <br>
</blockquote>
Done.<br>
<blockquote type="cite"
cite="mid:8418885d-8812-d7d9-d74c-1808e2937153@tarantool.org"> <br>
<blockquote type="cite"> <br>
Closes #3670 <br>
--- <br>
Branch:
<a class="moz-txt-link-freetext"
href="https://github.com/tarantool/tarantool/tree/imeevma/gh-3670-assertion-in-autoincrement-column">https://github.com/tarantool/tarantool/tree/imeevma/gh-3670-assertion-in-autoincrement-column</a><br>
Issue: <a class="moz-txt-link-freetext"
href="https://github.com/tarantool/tarantool/issues/3670">https://github.com/tarantool/tarantool/issues/3670</a>
<br>
<br>
src/box/sql/vdbe.c | 7 +++++-- <br>
test/sql-tap/autoinc.test.lua | 27 ++++++++++++++++++++++++++-
<br>
2 files changed, 31 insertions(+), 3 deletions(-) <br>
<br>
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c <br>
index 00ffb0c..632fa52 100644 <br>
--- a/src/box/sql/vdbe.c <br>
+++ b/src/box/sql/vdbe.c <br>
@@ -3758,10 +3758,13 @@ case OP_FCopy: { /* out2 */ <br>
/* Flag is set and register is NULL -> do nothing
*/ <br>
} else { <br>
assert(memIsValid(pIn1)); <br>
- assert(pIn1->flags & MEM_Int); <br>
pOut = &aMem[pOp->p2]; <br>
- MemSetTypeFlag(pOut, MEM_Int); <br>
+ /* <br>
+ * Flag should be MEM_Int but in some cases it can <br>
+ * have an other value. See gh-3670. <br>
</blockquote>
<br>
2. Do not ref issues in a code. Explain, why it is not Int. Even
if <br>
we could have issue refs, this comment explained nothing - I
opened <br>
<a class="moz-txt-link-freetext"
href="https://github.com/tarantool/tarantool/issues/3670">https://github.com/tarantool/tarantool/issues/3670</a>
and found only <br>
Gulutzan's comment with a test. No an explanation. <br>
</blockquote>
Fixed.<br>
<blockquote type="cite"
cite="mid:8418885d-8812-d7d9-d74c-1808e2937153@tarantool.org"> <br>
<blockquote type="cite">+ */ <br>
+ MemSetTypeFlag(pOut, pIn1->flags); <br>
pOut->u.i = pIn1->u.i; <br>
} <br>
diff --git a/test/sql-tap/autoinc.test.lua
b/test/sql-tap/autoinc.test.lua <br>
index dda7061..bd40de9 100755 <br>
--- a/test/sql-tap/autoinc.test.lua <br>
+++ b/test/sql-tap/autoinc.test.lua <br>
@@ -1,6 +1,6 @@ <br>
#!/usr/bin/env tarantool <br>
test = require("sqltester") <br>
-test:plan(46) <br>
+test:plan(48) <br>
--!./tcltestrunner.lua <br>
-- 2004 November 12 <br>
@@ -801,4 +801,29 @@ test:do_test( <br>
-- </autoinc-a69637.2> <br>
}) <br>
+-- gh-3670: Assertion with large number in autoincrement
column <br>
+test:do_catchsql_test( <br>
+ "autoinc-gh-3670-1", <br>
+ [[ <br>
+ CREATE TABLE t0 (s1 INT PRIMARY KEY AUTOINCREMENT); <br>
+ INSERT INTO t0 VALUES (2147483647); <br>
+ INSERT INTO t0 SELECT max(s1)*max(s1)*max(s1) FROM t0;
<br>
+ ]], { <br>
+ -- <autoinc-10.2> <br>
+ 1, "datatype mismatch" <br>
</blockquote>
<br>
3. Why mismatch? integer * integer * integer is still an <br>
integer. If it is an overflow, then it should print 'overflow', <br>
it is not? An overflow should be detected somewhere. <br>
</blockquote>
Now it is works the same way it works with columns without<br>
autoincrement:<br>
create table t4 (s1 int primary key);<br>
insert into t4 values (2147483647);<br>
insert into t4 select max(s1)*max(s1)*max(s1) from t4;<br>
<br>
Also, result of query<br>
select max(s1)*max(s1)*max(s1) from t4<br>
is a real number:<br>
9.903520300448e+27<br>
<blockquote type="cite"
cite="mid:8418885d-8812-d7d9-d74c-1808e2937153@tarantool.org"> <br>
<blockquote type="cite">+ -- </autoinc-10.2> <br>
+ }) <br>
+ <br>
+test:do_catchsql_test( <br>
+ "autoinc-gh-3670-2", <br>
+ [[ <br>
+ CREATE TABLE t1 (s1 INT PRIMARY KEY AUTOINCREMENT, s2
CHAR); <br>
+ INSERT INTO t1 VALUES (1, 'a'); <br>
+ INSERT INTO t1 SELECT s2, s2 FROM t1; <br>
+ ]], { <br>
+ -- <autoinc-10.2> <br>
+ 1, "datatype mismatch" <br>
+ -- </autoinc-10.2> <br>
+ }) <br>
+ <br>
test:finish_test() <br>
<br>
</blockquote>
</blockquote>
<br>
New patch:<br>
commit 53a229c67b096a09f26df4b5ba73d9a90c46e38f<br>
Author: Mergen Imeev <a class="moz-txt-link-rfc2396E" href="mailto:imeevma@gmail.com"><imeevma@gmail.com></a><br>
Date: Thu Sep 27 22:42:29 2018 +0300<br>
<br>
sql: assertion in autoincrement column<br>
<br>
If query is like "INSERT INTO table SELECT ..." then it is<br>
completely fine that the result of SELECT isn't integer. This<br>
value wasn't cheched propertly if it was inserted in column<br>
with autoincrement.<br>
<br>
Closes #3670<br>
<br>
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c<br>
index 7c1015c..de18262 100644<br>
--- a/src/box/sql/vdbe.c<br>
+++ b/src/box/sql/vdbe.c<br>
@@ -3736,10 +3736,15 @@ case OP_FCopy: { /* out2 */<br>
/* Flag is set and register is NULL -> do nothing */<br>
} else {<br>
assert(memIsValid(pIn1));<br>
- assert(pIn1->flags & MEM_Int);<br>
<br>
pOut = &aMem[pOp->p2];<br>
- MemSetTypeFlag(pOut, MEM_Int);<br>
+ /*<br>
+ * If query is like "INSERT INTO table SELECT ..."<br>
+ * then it is possible then the result of SELECT<br>
+ * isn't MEM_Int. It will be checked later in<br>
+ * current VDBE.<br>
+ */<br>
+ MemSetTypeFlag(pOut, pIn1->flags);<br>
<br>
pOut->u.i = pIn1->u.i;<br>
}<br>
diff --git a/test/sql-tap/autoinc.test.lua
b/test/sql-tap/autoinc.test.lua<br>
index dda7061..bd40de9 100755<br>
--- a/test/sql-tap/autoinc.test.lua<br>
+++ b/test/sql-tap/autoinc.test.lua<br>
@@ -1,6 +1,6 @@<br>
#!/usr/bin/env tarantool<br>
test = require("sqltester")<br>
-test:plan(46)<br>
+test:plan(48)<br>
<br>
--!./tcltestrunner.lua<br>
-- 2004 November 12<br>
@@ -801,4 +801,29 @@ test:do_test(<br>
-- </autoinc-a69637.2><br>
})<br>
<br>
+-- gh-3670: Assertion with large number in autoincrement column<br>
+test:do_catchsql_test(<br>
+ "autoinc-gh-3670-1",<br>
+ [[<br>
+ CREATE TABLE t0 (s1 INT PRIMARY KEY AUTOINCREMENT);<br>
+ INSERT INTO t0 VALUES (2147483647);<br>
+ INSERT INTO t0 SELECT max(s1)*max(s1)*max(s1) FROM t0;<br>
+ ]], {<br>
+ -- <autoinc-10.2><br>
+ 1, "datatype mismatch"<br>
+ -- </autoinc-10.2><br>
+ })<br>
+<br>
+test:do_catchsql_test(<br>
+ "autoinc-gh-3670-2",<br>
+ [[<br>
+ CREATE TABLE t1 (s1 INT PRIMARY KEY AUTOINCREMENT, s2
CHAR);<br>
+ INSERT INTO t1 VALUES (1, 'a');<br>
+ INSERT INTO t1 SELECT s2, s2 FROM t1;<br>
+ ]], {<br>
+ -- <autoinc-10.2><br>
+ 1, "datatype mismatch"<br>
+ -- </autoinc-10.2><br>
+ })<br>
+<br>
test:finish_test()<br>
<br>
</body>
</html>