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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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