[tarantool-patches] Re: [PATCH v1 1/1] sql: change the way to set AUTOINCREMENT

["n.pettik" <korablev@tarantool.org>]

> On 27 Jul 2019, at 13:29, imeevma@tarantool.org wrote:
> 
> Prior to this patch, the auto-increment feature could only be set
> in an INTEGER field of PRIMARY KEY if the PRIMARY KEY consisted of
> a single field. It was not possible to use this feature if the
> PRIMARY KEY consists of more than one field. This patch allows to
> set AUTOINCREMENT for any INTEGER field of PRIMARY KEY. This was
> achieved by changing the way the feature is defined. It was
> previously defined after the PRIMARY KEY keywords, but now it must
> follow the definition of the field to which it belongs.
> 
> Example of old definition:
> CREATE TABLE t (i INT, a INT, PRIMARY KEY (a AUTOINCREMENT));
> 
> Example of new definition:
> CREATE TABLE t (i INT, a INT AUTOINCREMENT, PRIMARY KEY (a));
> 
> Closes #4217
> 
> @TarantoolBot document
> Title: The auto-increment feature
> The auto-increment feature allows to replace a NULL inserted in an
> INTEGER field of PRIMARY KEY with a number generated by sequence
> if the field has the feature. This feature can belong to any
> INTEGER field of PRIMARY KEY, but no more than one of the INTEGER
> fields of PRIMARY KEY can have it.

That’s obvious and I guess already documented.
Instead, I’d rather focus on changes provided by
exactly your patch. What is more, this patch introduces
non-compatible grammar changes, which without doubts
must be documented.

Regarding to patch itself, why did you decide to break current
way of declaration of autoincrement feature? If there was any
discussion, please past it here (I failed to find one in server
dev mailing list).

> Definition of the feature should follow the field it belongs to.
> 
> Examples of definition of auto-increment feature:
> CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT);
> CREATE TABLE t (i INT AUTOINCREMENT, a INT, PRIMARY KEY(i));
> CREATE TABLE t (i INT, a INT AUTOINCREMENT, PRIMARY KEY (i, a));
> ---
> 
> @@ -1350,7 +1333,7 @@ sqlEndTable(struct Parse *pParse)
> 	 * Check to see if we need to create an _sequence table
> 	 * for keeping track of autoincrement keys.
> 	 */
> -	if (pParse->create_table_def.has_autoinc) {
> +	if (pParse->create_table_def.autoinc_field != UINT32_MAX) {

Why don’t simply use has_autoinc + autoinc_field?
It wouldn’t affect performance, but IMHO code would
look a bit better.

> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> index d4e1ec8..be0f27a 100644
> --- a/src/box/sql/parse.y
> +++ b/src/box/sql/parse.y
> @@ -266,9 +266,18 @@ ccons ::= NULL onconf(R).        {
>     if (R != ON_CONFLICT_ACTION_ABORT)
>         sql_column_add_nullable_action(pParse, R);
> }
> +ccons ::= AUTOINCR. {

Adding autoincrement to a list of constraint declarations leads to
such unclear error message:

tarantool> CREATE TABLE tt(x INTEGER PRIMARY KEY AUTOINCREMENT AUTOINCREMENT);
---
- error: 'Syntax error in CREATE TABLE: statement cannot have more than one AUTOINCREMENT'
…

Previous version looked better IMHO:

- Syntax error near 'AUTOINCREMENT'
...

> +  if (pParse->create_table_def.autoinc_field != UINT32_MAX) {
> +    diag_set(ClientError, ER_SQL_SYNTAX, "CREATE TABLE", "statement cannot "
> +             "have more than one AUTOINCREMENT”);

Statement can’t have autoinrement specifier by definition -
it’s table’s property. Thus, more accurate error message
would be:
“Table can’t feature more than one AUTOINCREMENT specifier/field” OR
“Table must feature at most one AUTOINCREMENT specifier/field”

Also, check error messages produced by other vendors.

> +    pParse->is_aborted = true;

Why not return right here?

diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index be0f27afd..7c94b4a60 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -271,10 +271,10 @@ ccons ::= AUTOINCR. {
     diag_set(ClientError, ER_SQL_SYNTAX, "CREATE TABLE", "statement cannot "
              "have more than one AUTOINCREMENT");
     pParse->is_aborted = true;
-  } else {
-    pParse->create_table_def.autoinc_field =
-      pParse->create_table_def.new_space->def->field_count - 1;
+    return;
   }
+  pParse->create_table_def.autoinc_field =
+    pParse->create_table_def.new_space->def->field_count - 1;
 }
 ccons ::= NOT NULL onconf(R).    {sql_column_add_nullable_action(pParse, R);}
 ccons ::= cconsname(N) PRIMARY KEY sortorder(Z). {

> diff --git a/test/sql-tap/autoinc.test.lua b/test/sql-tap/autoinc.test.lua
> index 2601968..05d3042 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(47)
> +test:plan(58)
> 
> --!./tcltestrunner.lua
> -- 2004 November 12
> @@ -537,11 +537,13 @@ test:do_catchsql_test(
> 
> 
> +test:do_catchsql_test(
> +    "autoinc-11.6",
> +    [[
> +        CREATE TABLE t (i INT, a INT, b INT AUTOINCREMENT, PRIMARY KEY (a, i));
> +    ]], {
> +        -- <autoinc-11.6>
> +        1, "Can't create or modify index 'pk_unnamed_T_1' in space 'T': sequence field must be a part of the index"
> +        -- </autoinc-11.6>
> +    })
> +
> +-- Make sure that AUTOINCREMENT only works for INTEGER field.

Test below doesn’t really belong to the issue..
Please in this patch add only relevant tests.

> +test:do_catchsql_test(
> +    "autoinc-11.7",
> +    [[
> +        CREATE TABLE t (i TEXT PRIMARY KEY AUTOINCREMENT);
> +    ]], {
> +        -- <autoinc-11.7>
> +        1, "Can't create or modify index 'pk_unnamed_T_1' in space 'T': sequence cannot be used with a non-integer key"
> +        -- </autoinc-11.7>
> +    })
> +
> +test:do_catchsql_test(
> +    "autoinc-11.8",
> +    [[
> +        CREATE TABLE t (i REAL PRIMARY KEY AUTOINCREMENT);
> +    ]], {
> +        -- <autoinc-11.7>
> +        1, "Can't create or modify index 'pk_unnamed_T_1' in space 'T': sequence cannot be used with a non-integer key"
> +        -- </autoinc-11.7>
> +    })
> +
> +test:do_catchsql_test(
> +    "autoinc-11.9",
> +    [[
> +        CREATE TABLE t (i BOOLEAN PRIMARY KEY AUTOINCREMENT);
> +    ]], {
> +        -- <autoinc-11.7>
> +        1, "Can't create or modify index 'pk_unnamed_T_1' in space 'T': sequence cannot be used with a non-integer key"
> +        -- </autoinc-11.7>
> +    })
> +
> +test:do_catchsql_test(
> +    "autoinc-11.10",
> +    [[
> +        CREATE TABLE t (i SCALAR PRIMARY KEY AUTOINCREMENT);
> +    ]], {
> +        -- <autoinc-11.7>
> +        1, "Can't create or modify index 'pk_unnamed_T_1' in space 'T': sequence cannot be used with a non-integer key"
> +        -- </autoinc-11.7>
> +    })
> +
> +-- Make sure that there can be no more than one AUTOINCREMENT.
> +test:do_catchsql_test(
> +    "autoinc-11.11",
> +    [[
> +        CREATE TABLE t (i INT AUTOINCREMENT, a INT AUTOINCREMENT, PRIMARY KEY (i, a));
> +    ]], {
> +        -- <autoinc-11.7>
> +        1, "Syntax error in CREATE TABLE: statement cannot have more than one AUTOINCREMENT"
> +        -- </autoinc-11.7>
> +    })
> +
> test:finish_test()
> diff --git a/test/sql-tap/sql-errors.test.lua b/test/sql-tap/sql-errors.test.lua
> index f452e3c..a93b6ab 100755
> --- a/test/sql-tap/sql-errors.test.lua
> +++ b/test/sql-tap/sql-errors.test.lua
> @@ -62,7 +62,7 @@ test:do_catchsql_test(
> 		CREATE TABLE t5 (i TEXT PRIMARY KEY AUTOINCREMENT);
> 	]], {
> 		-- <sql-errors-1.5>
> -		1,"Failed to create space 'T5': AUTOINCREMENT is only allowed on an INTEGER PRIMARY KEY or INT PRIMARY KEY"
> +		1,"Can't create or modify index 'pk_unnamed_T5_1' in space 'T5': sequence cannot be used with a non-integer key"
> 		-- </sql-errors-1.5>
> 	})
> 
> -- 
> 2.7.4
> 
>