[tarantool-patches] [PATCH v1 1/1] sql: assertion in autoincrement column

[tarantool-patches] [PATCH v1 1/1] sql: assertion in autoincrement column

[imeevma]

In some cases on attempt to insert inappropriate value in
autoincrement column assertion was thrown. Now it is fixed.

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.
+		 */
+		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()
-- 
2.7.4

[tarantool-patches] Re: [PATCH v1 1/1] sql: assertion in autoincrement column

[Konstantin Osipov]

* imeevma@tarantool.org <imeevma@tarantool.org> [18/09/28 19:18]:
>  		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.
> +		 */
> +		MemSetTypeFlag(pOut, pIn1->flags);

Please make the comment more explicit. I should not have to dig
into commit history and find the relevant test case to understand
what's going on here.

>  
>  		pOut->u.i = pIn1->u.i;
>  	}
-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

[tarantool-patches] Re: [PATCH v1 1/1] sql: assertion in autoincrement column

[Imeev Mergen]

Hello! Thank you for review! New patch below.


On 09/29/2018 07:27 AM, Konstantin Osipov wrote:
> * imeevma@tarantool.org <imeevma@tarantool.org> [18/09/28 19:18]:
>>   		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.
>> +		 */
>> +		MemSetTypeFlag(pOut, pIn1->flags);
> Please make the comment more explicit. I should not have to dig
> into commit history and find the relevant test case to understand
> what's going on here.
Done.
>
>>   
>>   		pOut->u.i = pIn1->u.i;
>>   	}

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()

[tarantool-patches] Re: [PATCH v1 1/1] sql: assertion in autoincrement column

[Imeev Mergen]

Hello! Thank you for review! New patch below.


On 09/29/2018 07:27 AM, Konstantin Osipov wrote:
> * imeevma@tarantool.org <imeevma@tarantool.org> [18/09/28 19:18]:
>>   		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.
>> +		 */
>> +		MemSetTypeFlag(pOut, pIn1->flags);
> Please make the comment more explicit. I should not have to dig
> into commit history and find the relevant test case to understand
> what's going on here.
Done.
>
>>   
>>   		pOut->u.i = pIn1->u.i;
>>   	}

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()

[tarantool-patches] Re: [PATCH v1 1/1] sql: assertion in autoincrement column

[Imeev Mergen]

[-- Attachment #1: Type: text/plain, Size: 13921 bytes --]

Hello! Thank you for review! Diff between two last versions
and new patch below.

On 10/11/2018 03:48 PM, Никита Петтик wrote:
>> 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
> Nit: cheched? propertly?
> Correct version I guess should be:
> ’This values wouldn’t be checked properly if it was inserted ...'
Fixed.
>>      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.
>> +         */
> How OP_FCopy is related to this ticket?
> Comment to this opcodes clearly says:
>
> * Copy integer value of register P1 in root frame in to register P2 of current
> * frame
>
> Mb you need to fix code generation for that query, but I doubt even that.
> AFAIU initial problem lies in ticket #3735. Due to this behaviour (converting INT
> to FLOAT for overflowed integers) we are trying to insert wrong value to
> source table. Hence, this patch is likely to be useless.
That is not right. For example (it is current test actually):

CREATE TABLE t1 (s1 INT PRIMARY KEY AUTOINCREMENT, s2 CHAR);
INSERT INTO t1 VALUES (1, 'a');
INSERT INTO t1 SELECT s2, s2 FROM t1;
>> +        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”
> Still: why ‘datatype mismatch’ and not ‘integer overflow’?
> I guess from now this test belongs to
> https://github.com/tarantool/tarantool/issues/3735
Removed this test and added commented issue 3735
https://github.com/tarantool/tarantool/issues/3735#issuecomment-428990257
>> +        -- </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>
>> +    })
>> +

*Diff between two last versions:*

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 5862917..e98f844 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -35,6 +35,7 @@
   */
  #include "sqliteInt.h"
  #include "tarantoolInt.h"
+#include "vdbeInt.h"
  #include "box/session.h"
  #include "box/schema.h"
  #include "bit/bit.h"
@@ -669,6 +670,11 @@ sqlite3Insert(Parse * pParse,    /* Parser context */
                       */
                      sqlite3VdbeAddOp3(v, OP_Column, srcTab,
                                j, regTmp);
+                    sqlite3VdbeAddOp2(v, OP_IsNull,
+                              regTmp,
+                              v->nOp + 2);
+                    sqlite3VdbeAddOp1(v, OP_MustBeInt,
+                              regTmp);
                      sqlite3VdbeAddOp2(v, OP_FCopy, regTmp,
                                iRegStore);
                      sqlite3VdbeChangeP3(v, -1,
@@ -685,6 +691,14 @@ sqlite3Insert(Parse * pParse,    /* Parser context */
                           * autoinc-ed value with select result
                           * in case that result is NULL
                           */
+                        sqlite3VdbeAddOp2(v, OP_IsNull,
+                                  regFromSelect
+                                  + j,
+                                  v->nOp + 2);
+                        sqlite3VdbeAddOp1(v,
+                                  OP_MustBeInt,
+                                  regFromSelect
+                                  + j);
                          sqlite3VdbeAddOp2(v, OP_FCopy,
                                    regFromSelect
                                    + j,
@@ -714,6 +728,14 @@ sqlite3Insert(Parse * pParse,    /* Parser context */
                           * autoinc-ed value with select result
                           * in case that result is NULL
                           */
+                        sqlite3VdbeAddOp2(v, OP_IsNull,
+                                  pList->a[j].
+                                  pExpr->iTable,
+                                  v->nOp + 2);
+                        sqlite3VdbeAddOp1(v,
+                                  OP_MustBeInt,
+                                  pList->a[j].
+                                  pExpr->iTable);
                          sqlite3VdbeAddOp2(v, OP_FCopy,
                                    pList->a[j].
                                    pExpr->iTable,
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index de18262..7c1015c 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -3736,15 +3736,10 @@ 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];
-        /*
-         * 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);
+        MemSetTypeFlag(pOut, MEM_Int);

          pOut->u.i = pIn1->u.i;
      }
diff --git a/test/sql-tap/autoinc.test.lua b/test/sql-tap/autoinc.test.lua
index bd40de9..fa45c0e 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(48)
+test:plan(47)

  --!./tcltestrunner.lua
  -- 2004 November 12
@@ -803,27 +803,15 @@ 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;
-    ]], {
-        -- <autoinc-10.2>
-        1, "datatype mismatch"
-        -- </autoinc-10.2>
-    })
-
-test:do_catchsql_test(
-    "autoinc-gh-3670-2",
+    "autoinc-gh-3670",
      [[
          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>
+        -- <autoinc-gh-3670>
          1, "datatype mismatch"
-        -- </autoinc-10.2>
+        -- </autoinc-gh-3670>
      })

  test:finish_test()

*New patch:*

commit 37bbfe1fe46c3242137de67b0032a769316aed5f
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. These
     values wouldn’t be checked properly if it was inserted in
     column with autoincrement. This patch fixes mentioned check.

     Closes #3670

diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 5862917..e98f844 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -35,6 +35,7 @@
   */
  #include "sqliteInt.h"
  #include "tarantoolInt.h"
+#include "vdbeInt.h"
  #include "box/session.h"
  #include "box/schema.h"
  #include "bit/bit.h"
@@ -669,6 +670,11 @@ sqlite3Insert(Parse * pParse,    /* Parser context */
                       */
                      sqlite3VdbeAddOp3(v, OP_Column, srcTab,
                                j, regTmp);
+                    sqlite3VdbeAddOp2(v, OP_IsNull,
+                              regTmp,
+                              v->nOp + 2);
+                    sqlite3VdbeAddOp1(v, OP_MustBeInt,
+                              regTmp);
                      sqlite3VdbeAddOp2(v, OP_FCopy, regTmp,
                                iRegStore);
                      sqlite3VdbeChangeP3(v, -1,
@@ -685,6 +691,14 @@ sqlite3Insert(Parse * pParse,    /* Parser context */
                           * autoinc-ed value with select result
                           * in case that result is NULL
                           */
+                        sqlite3VdbeAddOp2(v, OP_IsNull,
+                                  regFromSelect
+                                  + j,
+                                  v->nOp + 2);
+                        sqlite3VdbeAddOp1(v,
+                                  OP_MustBeInt,
+                                  regFromSelect
+                                  + j);
                          sqlite3VdbeAddOp2(v, OP_FCopy,
                                    regFromSelect
                                    + j,
@@ -714,6 +728,14 @@ sqlite3Insert(Parse * pParse,    /* Parser context */
                           * autoinc-ed value with select result
                           * in case that result is NULL
                           */
+                        sqlite3VdbeAddOp2(v, OP_IsNull,
+                                  pList->a[j].
+                                  pExpr->iTable,
+                                  v->nOp + 2);
+                        sqlite3VdbeAddOp1(v,
+                                  OP_MustBeInt,
+                                  pList->a[j].
+                                  pExpr->iTable);
                          sqlite3VdbeAddOp2(v, OP_FCopy,
                                    pList->a[j].
                                    pExpr->iTable,
diff --git a/test/sql-tap/autoinc.test.lua b/test/sql-tap/autoinc.test.lua
index dda7061..fa45c0e 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(47)

  --!./tcltestrunner.lua
  -- 2004 November 12
@@ -801,4 +801,17 @@ test:do_test(
          -- </autoinc-a69637.2>
      })

+-- gh-3670: Assertion with large number in autoincrement column
+test:do_catchsql_test(
+    "autoinc-gh-3670",
+    [[
+        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-gh-3670>
+        1, "datatype mismatch"
+        -- </autoinc-gh-3670>
+    })
+
  test:finish_test()


[-- Attachment #2: Type: text/html, Size: 17125 bytes --]

[tarantool-patches] Re: [PATCH v1 1/1] sql: assertion in autoincrement column

[n.pettik]

LGTM.

[tarantool-patches] Re: [PATCH v1 1/1] sql: assertion in autoincrement column

[Vladislav Shpilevoy]

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.

> 
> 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.

> +		 */
> +		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.

> +        -- </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()
> 

[tarantool-patches] Re: [PATCH v1 1/1] sql: assertion in autoincrement column

[Imeev Mergen]

[-- Attachment #1: Type: text/plain, Size: 6425 bytes --]

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()


[-- Attachment #2: Type: text/html, Size: 9620 bytes --]

[tarantool-patches] Re: [PATCH v1 1/1] sql: assertion in autoincrement column

[Vladislav Shpilevoy]

Hi! Thanks for the fixes! See 3 comments below.

>> 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

1. The thing you said does not correlate with my
question. Why mismatch? I multiplied integer three
times, it should not auto turn into double then. It
should throw 'overflow' error.

> 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.

2. As you said above, it is not about autoincrement. Why in the
commit message do you say on the contrary?

3. It is not linked with "insert into table select". This fails
as well:

box.sql.execute("CREATE TABLE t0 (s1 INT PRIMARY KEY)")
box.sql.execute("INSERT INTO t0 VALUES (2147483647*2147483647*2147483647)")

> 
>      Closes #3670
> 

[tarantool-patches] Re: [PATCH v1 1/1] sql: assertion in autoincrement column

[Imeev Mergen]

Hello! Thank you for review! After discussion it was decided to
left patch as it is and create new ticket about changing types
after SELECT.

Nikita, can you review this patch?

On 10/10/2018 01:21 PM, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes! See 3 comments below.
>
>>> 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
>
> 1. The thing you said does not correlate with my
> question. Why mismatch? I multiplied integer three
> times, it should not auto turn into double then. It
> should throw 'overflow' error.
>
>> 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.
>
> 2. As you said above, it is not about autoincrement. Why in the
> commit message do you say on the contrary?
>
> 3. It is not linked with "insert into table select". This fails
> as well:
>
> box.sql.execute("CREATE TABLE t0 (s1 INT PRIMARY KEY)")
> box.sql.execute("INSERT INTO t0 VALUES 
> (2147483647*2147483647*2147483647)")
>
>>
>>      Closes #3670
>>

[tarantool-patches] Re: [PATCH v1 1/1] sql: assertion in autoincrement column

[Kirill Yukhin]

Hello,
On 28 Sep 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.
> 
> Closes #3670
> ---
> Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-3670-assertion-in-autoincrement-column
> Issue: https://github.com/tarantool/tarantool/issues/3670
I've checked your patch into 2.1 branch.

--
Regards, Kirill Yukhin