From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 54ABB287F2 for ; Fri, 5 Oct 2018 12:40:29 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id noUr-FqqdOkg for ; Fri, 5 Oct 2018 12:40:29 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 665F92637A for ; Fri, 5 Oct 2018 12:40:28 -0400 (EDT) From: Imeev Mergen Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: assertion in autoincrement column References: <8418885d-8812-d7d9-d74c-1808e2937153@tarantool.org> Message-ID: Date: Fri, 5 Oct 2018 19:40:25 +0300 MIME-Version: 1.0 In-Reply-To: <8418885d-8812-d7d9-d74c-1808e2937153@tarantool.org> Content-Type: multipart/alternative; boundary="------------D47CBE41AE17373BBF107C26" Content-Language: en-US Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Vladislav Shpilevoy , tarantool-patches@freelists.org Cc: kostja@tarantool.org This is a multi-part message in MIME format. --------------D47CBE41AE17373BBF107C26 Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Hello! Thank you for review! My answers and new patch below. On 10/03/2018 06:19 PM, Vladislav Shpilevoy wrote: > Hi! Thanks for the patch! See 3 comments below. > > On 28/09/2018 19:14, imeevma@tarantool.org wrote: >> In some cases on attempt to insert inappropriate value in >> autoincrement column assertion was thrown. Now it is fixed. > > 1. Firstly, I agree with Kostja - I do not understand why in 'some > cases' a value becomes not integer when I insert an integer. Please, > describe here a reason. Done. > >> >> Closes #3670 >> --- >> Branch: >> https://github.com/tarantool/tarantool/tree/imeevma/gh-3670-assertion-in-autoincrement-column >> Issue: https://github.com/tarantool/tarantool/issues/3670 >> >>   src/box/sql/vdbe.c            |  7 +++++-- >>   test/sql-tap/autoinc.test.lua | 27 ++++++++++++++++++++++++++- >>   2 files changed, 31 insertions(+), 3 deletions(-) >> >> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c >> index 00ffb0c..632fa52 100644 >> --- a/src/box/sql/vdbe.c >> +++ b/src/box/sql/vdbe.c >> @@ -3758,10 +3758,13 @@ case OP_FCopy: {     /* out2 */ >>           /* Flag is set and register is NULL -> do nothing */ >>       } else { >>           assert(memIsValid(pIn1)); >> -        assert(pIn1->flags &  MEM_Int); >>             pOut = &aMem[pOp->p2]; >> -        MemSetTypeFlag(pOut, MEM_Int); >> +        /* >> +         * Flag should be MEM_Int but in some cases it can >> +         * have an other value. See gh-3670. > > 2. Do not ref issues in a code. Explain, why it is not Int. Even if > we could have issue refs, this comment explained nothing - I opened > https://github.com/tarantool/tarantool/issues/3670 and found only > Gulutzan's comment with a test. No an explanation. Fixed. > >> +         */ >> +        MemSetTypeFlag(pOut, pIn1->flags); >>             pOut->u.i = pIn1->u.i; >>       } >> diff --git a/test/sql-tap/autoinc.test.lua >> b/test/sql-tap/autoinc.test.lua >> index dda7061..bd40de9 100755 >> --- a/test/sql-tap/autoinc.test.lua >> +++ b/test/sql-tap/autoinc.test.lua >> @@ -1,6 +1,6 @@ >>   #!/usr/bin/env tarantool >>   test = require("sqltester") >> -test:plan(46) >> +test:plan(48) >>     --!./tcltestrunner.lua >>   -- 2004 November 12 >> @@ -801,4 +801,29 @@ test:do_test( >>           -- >>       }) >>   +-- gh-3670: Assertion with large number in autoincrement column >> +test:do_catchsql_test( >> +    "autoinc-gh-3670-1", >> +    [[ >> +        CREATE TABLE t0 (s1 INT PRIMARY KEY AUTOINCREMENT); >> +        INSERT INTO t0 VALUES (2147483647); >> +        INSERT INTO t0 SELECT max(s1)*max(s1)*max(s1) FROM t0; >> +    ]], { >> +        -- >> +        1, "datatype mismatch" > > 3. Why mismatch? integer * integer * integer is still an > integer. If it is an overflow, then it should print 'overflow', > it is not? An overflow should be detected somewhere. Now it is works the same way it works with columns without autoincrement: create table t4 (s1 int primary key); insert into t4 values (2147483647); insert into t4 select max(s1)*max(s1)*max(s1) from t4; Also, result of query select max(s1)*max(s1)*max(s1) from t4 is a real number: 9.903520300448e+27 > >> +        -- >> +    }) >> + >> +test:do_catchsql_test( >> +    "autoinc-gh-3670-2", >> +    [[ >> +        CREATE TABLE t1 (s1 INT PRIMARY KEY AUTOINCREMENT, s2 CHAR); >> +        INSERT INTO t1 VALUES (1, 'a'); >> +        INSERT INTO t1 SELECT s2, s2 FROM t1; >> +    ]], { >> +        -- >> +        1, "datatype mismatch" >> +        -- >> +    }) >> + >>   test:finish_test() >> New patch: commit 53a229c67b096a09f26df4b5ba73d9a90c46e38f Author: Mergen Imeev Date:   Thu Sep 27 22:42:29 2018 +0300     sql: assertion in autoincrement column     If query is like "INSERT INTO table SELECT ..." then it is     completely fine that the result of SELECT isn't integer. This     value wasn't cheched propertly if it was inserted in column     with autoincrement.     Closes #3670 diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index 7c1015c..de18262 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -3736,10 +3736,15 @@ case OP_FCopy: {     /* out2 */          /* Flag is set and register is NULL -> do nothing  */      } else {          assert(memIsValid(pIn1)); -        assert(pIn1->flags &  MEM_Int);          pOut = &aMem[pOp->p2]; -        MemSetTypeFlag(pOut, MEM_Int); +        /* +         * If query is like "INSERT INTO table SELECT ..." +         * then it is possible then the result of SELECT +         * isn't MEM_Int. It will be checked later in +         * current VDBE. +         */ +        MemSetTypeFlag(pOut, pIn1->flags);          pOut->u.i = pIn1->u.i;      } diff --git a/test/sql-tap/autoinc.test.lua b/test/sql-tap/autoinc.test.lua index dda7061..bd40de9 100755 --- a/test/sql-tap/autoinc.test.lua +++ b/test/sql-tap/autoinc.test.lua @@ -1,6 +1,6 @@  #!/usr/bin/env tarantool  test = require("sqltester") -test:plan(46) +test:plan(48)  --!./tcltestrunner.lua  -- 2004 November 12 @@ -801,4 +801,29 @@ test:do_test(          --      }) +-- gh-3670: Assertion with large number in autoincrement column +test:do_catchsql_test( +    "autoinc-gh-3670-1", +    [[ +        CREATE TABLE t0 (s1 INT PRIMARY KEY AUTOINCREMENT); +        INSERT INTO t0 VALUES (2147483647); +        INSERT INTO t0 SELECT max(s1)*max(s1)*max(s1) FROM t0; +    ]], { +        -- +        1, "datatype mismatch" +        -- +    }) + +test:do_catchsql_test( +    "autoinc-gh-3670-2", +    [[ +        CREATE TABLE t1 (s1 INT PRIMARY KEY AUTOINCREMENT, s2 CHAR); +        INSERT INTO t1 VALUES (1, 'a'); +        INSERT INTO t1 SELECT s2, s2 FROM t1; +    ]], { +        -- +        1, "datatype mismatch" +        -- +    }) +  test:finish_test() --------------D47CBE41AE17373BBF107C26 Content-Type: text/html; charset="utf-8" Content-Transfer-Encoding: 8bit

Hello! Thank you for review! My answers and new patch below.


On 10/03/2018 06:19 PM, Vladislav Shpilevoy wrote:
Hi! Thanks for the patch! See 3 comments below.

On 28/09/2018 19:14, imeevma@tarantool.org wrote:
In some cases on attempt to insert inappropriate value in
autoincrement column assertion was thrown. Now it is fixed.

1. Firstly, I agree with Kostja - I do not understand why in 'some
cases' a value becomes not integer when I insert an integer. Please,
describe here a reason.
Done.


Closes #3670
---
Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-3670-assertion-in-autoincrement-column
Issue: https://github.com/tarantool/tarantool/issues/3670

  src/box/sql/vdbe.c            |  7 +++++--
  test/sql-tap/autoinc.test.lua | 27 ++++++++++++++++++++++++++-
  2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 00ffb0c..632fa52 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -3758,10 +3758,13 @@ case OP_FCopy: {     /* out2 */
          /* Flag is set and register is NULL -> do nothing  */
      } else {
          assert(memIsValid(pIn1));
-        assert(pIn1->flags &  MEM_Int);
            pOut = &aMem[pOp->p2];
-        MemSetTypeFlag(pOut, MEM_Int);
+        /*
+         * Flag should be MEM_Int but in some cases it can
+         * have an other value. See gh-3670.

2. Do not ref issues in a code. Explain, why it is not Int. Even if
we could have issue refs, this comment explained nothing - I opened
https://github.com/tarantool/tarantool/issues/3670 and found only
Gulutzan's comment with a test. No an explanation.
Fixed.

+         */
+        MemSetTypeFlag(pOut, pIn1->flags);
            pOut->u.i = pIn1->u.i;
      }
diff --git a/test/sql-tap/autoinc.test.lua b/test/sql-tap/autoinc.test.lua
index dda7061..bd40de9 100755
--- a/test/sql-tap/autoinc.test.lua
+++ b/test/sql-tap/autoinc.test.lua
@@ -1,6 +1,6 @@
  #!/usr/bin/env tarantool
  test = require("sqltester")
-test:plan(46)
+test:plan(48)
    --!./tcltestrunner.lua
  -- 2004 November 12
@@ -801,4 +801,29 @@ test:do_test(
          -- </autoinc-a69637.2>
      })
  +-- gh-3670: Assertion with large number in autoincrement column
+test:do_catchsql_test(
+    "autoinc-gh-3670-1",
+    [[
+        CREATE TABLE t0 (s1 INT PRIMARY KEY AUTOINCREMENT);
+        INSERT INTO t0 VALUES (2147483647);
+        INSERT INTO t0 SELECT max(s1)*max(s1)*max(s1) FROM t0;
+    ]], {
+        -- <autoinc-10.2>
+        1, "datatype mismatch"

3. Why mismatch? integer * integer * integer is still an
integer. If it is an overflow, then it should print 'overflow',
it is not? An overflow should be detected somewhere.
Now it is works the same way it works with columns without
autoincrement:
create table t4 (s1 int primary key);
insert into t4 values (2147483647);
insert into t4 select max(s1)*max(s1)*max(s1) from t4;

Also, result of query
select max(s1)*max(s1)*max(s1) from t4
is a real number:
9.903520300448e+27

+        -- </autoinc-10.2>
+    })
+
+test:do_catchsql_test(
+    "autoinc-gh-3670-2",
+    [[
+        CREATE TABLE t1 (s1 INT PRIMARY KEY AUTOINCREMENT, s2 CHAR);
+        INSERT INTO t1 VALUES (1, 'a');
+        INSERT INTO t1 SELECT s2, s2 FROM t1;
+    ]], {
+        -- <autoinc-10.2>
+        1, "datatype mismatch"
+        -- </autoinc-10.2>
+    })
+
  test:finish_test()


New patch:
commit 53a229c67b096a09f26df4b5ba73d9a90c46e38f
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Thu Sep 27 22:42:29 2018 +0300

    sql: assertion in autoincrement column
   
    If query is like "INSERT INTO table SELECT ..." then it is
    completely fine that the result of SELECT isn't integer. This
    value wasn't cheched propertly if it was inserted in column
    with autoincrement.
   
    Closes #3670

diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 7c1015c..de18262 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -3736,10 +3736,15 @@ case OP_FCopy: {     /* out2 */
         /* Flag is set and register is NULL -> do nothing  */
     } else {
         assert(memIsValid(pIn1));
-        assert(pIn1->flags &  MEM_Int);
 
         pOut = &aMem[pOp->p2];
-        MemSetTypeFlag(pOut, MEM_Int);
+        /*
+         * If query is like "INSERT INTO table SELECT ..."
+         * then it is possible then the result of SELECT
+         * isn't MEM_Int. It will be checked later in
+         * current VDBE.
+         */
+        MemSetTypeFlag(pOut, pIn1->flags);
 
         pOut->u.i = pIn1->u.i;
     }
diff --git a/test/sql-tap/autoinc.test.lua b/test/sql-tap/autoinc.test.lua
index dda7061..bd40de9 100755
--- a/test/sql-tap/autoinc.test.lua
+++ b/test/sql-tap/autoinc.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(46)
+test:plan(48)
 
 --!./tcltestrunner.lua
 -- 2004 November 12
@@ -801,4 +801,29 @@ test:do_test(
         -- </autoinc-a69637.2>
     })
 
+-- gh-3670: Assertion with large number in autoincrement column
+test:do_catchsql_test(
+    "autoinc-gh-3670-1",
+    [[
+        CREATE TABLE t0 (s1 INT PRIMARY KEY AUTOINCREMENT);
+        INSERT INTO t0 VALUES (2147483647);
+        INSERT INTO t0 SELECT max(s1)*max(s1)*max(s1) FROM t0;
+    ]], {
+        -- <autoinc-10.2>
+        1, "datatype mismatch"
+        -- </autoinc-10.2>
+    })
+
+test:do_catchsql_test(
+    "autoinc-gh-3670-2",
+    [[
+        CREATE TABLE t1 (s1 INT PRIMARY KEY AUTOINCREMENT, s2 CHAR);
+        INSERT INTO t1 VALUES (1, 'a');
+        INSERT INTO t1 SELECT s2, s2 FROM t1;
+    ]], {
+        -- <autoinc-10.2>
+        1, "datatype mismatch"
+        -- </autoinc-10.2>
+    })
+
 test:finish_test()

--------------D47CBE41AE17373BBF107C26--