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

Tarantool development patches archive
 help / color / mirror / Atom feed

* [tarantool-patches] [PATCH v1 1/1] sql: change the way to set AUTOINCREMENT
@ 2019-07-27 10:29 imeevma
  2019-08-09 14:48 ` [tarantool-patches] " n.pettik
  0 siblings, 1 reply; 4+ messages in thread
From: imeevma @ 2019-07-27 10:29 UTC (permalink / raw)
  To: korablev; +Cc: tarantool-patches

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.

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));
---
https://github.com/tarantool/tarantool/issues/4217
https://github.com/tarantool/tarantool/tree/imeevma/gh-4217-autoinc-on-any-field-of-pk

 src/box/sql/build.c              |  33 +++------
 src/box/sql/parse.y              |  21 +++---
 src/box/sql/parse_def.h          |   9 ++-
 test/sql-tap/autoinc.test.lua    | 148 +++++++++++++++++++++++++++++++++++++--
 test/sql-tap/sql-errors.test.lua |   2 +-
 5 files changed, 172 insertions(+), 41 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 0a6759e..53f21da 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -625,12 +625,6 @@ sqlAddPrimaryKey(struct Parse *pParse)
 		sql_create_index(pParse);
 		if (db->mallocFailed)
 			goto primary_key_exit;
-	} else if (pParse->create_table_def.has_autoinc) {
-		diag_set(ClientError, ER_CREATE_SPACE, space->def->name,
-			 "AUTOINCREMENT is only allowed on an INTEGER PRIMARY "\
-			 "KEY or INT PRIMARY KEY");
-		pParse->is_aborted = true;
-		goto primary_key_exit;
 	} else {
 		sql_create_index(pParse);
 		pList = NULL;
@@ -1049,18 +1043,9 @@ emitNewSysSequenceRecord(Parse *pParse, int reg_seq_id, const char *seq_name)
 }
 
 static int
-emitNewSysSpaceSequenceRecord(Parse *pParse, int reg_space_id, int reg_seq_id,
-			      struct index_def *idx_def)
+emitNewSysSpaceSequenceRecord(Parse *pParse, int reg_space_id, int reg_seq_id)
 {
-	struct key_part *part = &idx_def->key_def->parts[0];
-	int fieldno = part->fieldno;
-	char *path = NULL;
-	if (part->path != NULL) {
-		path = sqlDbStrNDup(pParse->db, part->path, part->path_len);
-		if (path == NULL)
-			return -1;
-		path[part->path_len] = 0;
-	}
+	uint32_t autoinc_field = pParse->create_table_def.autoinc_field;
 
 	Vdbe *v = sqlGetVdbe(pParse);
 	int first_col = pParse->nMem + 1;
@@ -1076,12 +1061,10 @@ emitNewSysSpaceSequenceRecord(Parse *pParse, int reg_space_id, int reg_seq_id,
 	sqlVdbeAddOp2(v, OP_Bool, true, first_col + 3);
 
 	/* 4. Field id. */
-	sqlVdbeAddOp2(v, OP_Integer, fieldno, first_col + 4);
+	sqlVdbeAddOp2(v, OP_Integer, autoinc_field, first_col + 4);
 
 	/* 5. Field path. */
-	sqlVdbeAddOp4(v, OP_String8, 0, first_col + 5, 0,
-		      path != NULL ? path : "",
-		      path != NULL ? P4_DYNAMIC : P4_STATIC );
+	sqlVdbeAddOp4(v, OP_String8, 0, first_col + 5, 0, "", P4_STATIC);
 
 	sqlVdbeAddOp3(v, OP_MakeRecord, first_col + 1, 5, first_col);
 	return first_col;
@@ -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) {
 		assert(reg_space_id != 0);
 		/* Do an insertion into _sequence. */
 		int reg_seq_id = ++pParse->nMem;
@@ -1363,9 +1346,9 @@ sqlEndTable(struct Parse *pParse)
 		save_record(pParse, BOX_SEQUENCE_ID, reg_seq_record + 1, 1,
 			    v->nOp - 1, true);
 		/* Do an insertion into _space_sequence. */
-		int reg_space_seq_record = emitNewSysSpaceSequenceRecord(pParse,
-							reg_space_id, reg_seq_id,
-							new_space->index[0]->def);
+		int reg_space_seq_record =
+			emitNewSysSpaceSequenceRecord(pParse, reg_space_id,
+						      reg_seq_id);
 		sqlVdbeAddOp3(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID, 0,
 				  reg_space_seq_record);
 		save_record(pParse, BOX_SPACE_SEQUENCE_ID,
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. {
+  if (pParse->create_table_def.autoinc_field != UINT32_MAX) {
+    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;
+  }
+}
 ccons ::= NOT NULL onconf(R).    {sql_column_add_nullable_action(pParse, R);}
-ccons ::= cconsname(N) PRIMARY KEY sortorder(Z) autoinc(I). {
-  pParse->create_table_def.has_autoinc = I;
+ccons ::= cconsname(N) PRIMARY KEY sortorder(Z). {
   create_index_def_init(&pParse->create_index_def, NULL, &N, NULL,
                         SQL_INDEX_TYPE_CONSTRAINT_PK, Z, false);
   sqlAddPrimaryKey(pParse);
@@ -295,11 +304,6 @@ ccons ::= cconsname(N) REFERENCES nm(T) eidlist_opt(TA) matcharg(M) refargs(R).
 ccons ::= defer_subclause(D).    {fk_constraint_change_defer_mode(pParse, D);}
 ccons ::= COLLATE id(C).        {sqlAddCollateType(pParse, &C);}
 
-// The optional AUTOINCREMENT keyword
-%type autoinc {int}
-autoinc(X) ::= .          {X = 0;}
-autoinc(X) ::= AUTOINCR.  {X = 1;}
-
 // The next group of rules parses the arguments to a REFERENCES clause
 // that determine if the referential integrity checking is deferred or
 // or immediate and which determine what action to take if a ref-integ
@@ -337,8 +341,7 @@ init_deferred_pred_opt(A) ::= .                       {A = 0;}
 init_deferred_pred_opt(A) ::= INITIALLY DEFERRED.     {A = 1;}
 init_deferred_pred_opt(A) ::= INITIALLY IMMEDIATE.    {A = 0;}
 
-tcons ::= cconsname(N) PRIMARY KEY LP sortlist(X) autoinc(I) RP. {
-  pParse->create_table_def.has_autoinc = I;
+tcons ::= cconsname(N) PRIMARY KEY LP sortlist(X) RP. {
   create_index_def_init(&pParse->create_index_def, NULL, &N, X,
                         SQL_INDEX_TYPE_CONSTRAINT_PK, SORT_ORDER_ASC, false);
   sqlAddPrimaryKey(pParse);
diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
index 557e415..7d4bca3 100644
--- a/src/box/sql/parse_def.h
+++ b/src/box/sql/parse_def.h
@@ -212,8 +212,12 @@ struct create_table_def {
 	uint32_t check_count;
 	/** Check constraint appeared in CREATE TABLE stmt. */
 	struct rlist new_check;
-	/** True, if table to be created has AUTOINCREMENT PK. */
-	bool has_autoinc;
+	/**
+	 * Field number of field with AUTOINCREMENT. If its value
+	 * is UINT32_MAX than there is no field with
+	 * AUTOINCREMENT.
+	 */
+	uint32_t autoinc_field;
 };
 
 struct create_view_def {
@@ -461,6 +465,7 @@ create_table_def_init(struct create_table_def *table_def, struct Token *name,
 			       if_not_exists);
 	rlist_create(&table_def->new_fkey);
 	rlist_create(&table_def->new_check);
+	table_def->autoinc_field = UINT32_MAX;
 }
 
 static inline void
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(
 
 -- Allow the AUTOINCREMENT keyword inside the parentheses
 -- on a separate PRIMARY KEY designation.
+-- UPDATE: Changed in #4217. Now AUTOINCREMENT must follow the
+-- definition of the column to which it belongs.
 --
 test:do_execsql_test(
     "autoinc-7.1",
     [[
-        CREATE TABLE t7(x INTEGER, y REAL, PRIMARY KEY(x AUTOINCREMENT));
+        CREATE TABLE t7(x INTEGER AUTOINCREMENT, y REAL, PRIMARY KEY(x));
         INSERT INTO t7(y) VALUES(123);
         INSERT INTO t7(y) VALUES(234);
         DELETE FROM t7;
@@ -561,7 +563,7 @@ test:do_catchsql_test(
         CREATE TABLE t8(x TEXT PRIMARY KEY AUTOINCREMENT);
     ]], {
         -- <autoinc-7.2>
-        1, "Failed to create space 'T8': AUTOINCREMENT is only allowed on an INTEGER PRIMARY KEY or INT PRIMARY KEY"
+        1, "Can't create or modify index 'pk_unnamed_T8_1' in space 'T8': sequence cannot be used with a non-integer key"
         -- </autoinc-7.2>
     })
 
@@ -627,7 +629,7 @@ test:do_test(
     function()
         return test:execsql([[
             DROP TABLE IF EXISTS t7;
-            CREATE TABLE t7(x INT, y REAL, PRIMARY KEY(x AUTOINCREMENT));
+            CREATE TABLE t7(x INT AUTOINCREMENT, y REAL, PRIMARY KEY(x));
             INSERT INTO t7(y) VALUES(123);
             INSERT INTO t7(y) VALUES(234);
             DELETE FROM t7;
@@ -814,4 +816,142 @@ test:do_catchsql_test(
         -- </autoinc-gh-3670>
     })
 
+--
+-- gh-4217: make sure that AUTOINCREMENT can be used for any
+-- INTEGER field of PRIMARY KEY.
+--
+
+--
+-- Make sure AUTOINCREMENT works for a PRIMARY KEY that contains
+-- one column.
+--
+test:do_execsql_test(
+    "autoinc-11.1",
+    [[
+        CREATE TABLE t (i INT PRIMARY KEY AUTOINCREMENT, a INT);
+        INSERT INTO t VALUES (NULL, 1), (NULL, 1), (NULL, 1);
+        SELECT * FROM t;
+    ]], {
+        -- <autoinc-11.1>
+        1, 1, 2, 1, 3, 1
+        -- </autoinc-11.1>
+    })
+
+test:do_execsql_test(
+    "autoinc-11.2",
+    [[
+        DROP TABLE IF EXISTS t;
+        CREATE TABLE t (i INT AUTOINCREMENT, a INT, PRIMARY KEY(i));
+        INSERT INTO t VALUES (NULL, 1), (NULL, 1), (NULL, 1);
+        SELECT * FROM t;
+    ]], {
+        -- <autoinc-11.1>
+        1, 1, 2, 1, 3, 1
+        -- </autoinc-11.1>
+    })
+
+--
+-- Make sure AUTOINCREMENT works for any INTEGER field of PRIMARY
+-- KEY that contains more than one column.
+--
+test:do_execsql_test(
+    "autoinc-11.3",
+    [[
+        DROP TABLE t;
+        CREATE TABLE t (i TEXT, a INT AUTOINCREMENT, PRIMARY KEY (i, a));
+        INSERT INTO t VALUES (1, NULL), (1, NULL), (1, NULL);
+        SELECT * FROM t;
+    ]], {
+        -- <autoinc-11.3>
+        "1", 1, "1", 2, "1", 3
+        -- </autoinc-11.3>
+    })
+
+test:do_execsql_test(
+    "autoinc-11.4",
+    [[
+        DROP TABLE t;
+        CREATE TABLE t (i TEXT, a INT AUTOINCREMENT, PRIMARY KEY (a, i));
+        INSERT INTO t VALUES (1, NULL), (1, NULL), (1, NULL);
+        SELECT * FROM t;
+    ]], {
+        -- <autoinc-11.4>
+        "1", 1, "1", 2, "1", 3
+        -- </autoinc-11.4>
+    })
+
+-- Make sure that AUTOINCREMENT only works for PRIMARY KEY.
+test:do_catchsql_test(
+    "autoinc-11.5",
+    [[
+        DROP TABLE t;
+        CREATE TABLE t (i INT PRIMARY KEY, a INT AUTOINCREMENT);
+    ]], {
+        -- <autoinc-11.5>
+        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.5>
+    })
+
+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: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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [tarantool-patches] Re: [PATCH v1 1/1] sql: change the way to set AUTOINCREMENT
  2019-07-27 10:29 [tarantool-patches] [PATCH v1 1/1] sql: change the way to set AUTOINCREMENT imeevma
@ 2019-08-09 14:48 ` n.pettik
  2019-08-28 11:47   ` Mergen Imeev
  0 siblings, 1 reply; 4+ messages in thread
From: n.pettik @ 2019-08-09 14:48 UTC (permalink / raw)
  To: tarantool-patches; +Cc: Imeev Mergen



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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [tarantool-patches] Re: [PATCH v1 1/1] sql: change the way to set AUTOINCREMENT
  2019-08-09 14:48 ` [tarantool-patches] " n.pettik
@ 2019-08-28 11:47   ` Mergen Imeev
       [not found]     ` <20190918172225.GB89644@tarantool.org>
  0 siblings, 1 reply; 4+ messages in thread
From: Mergen Imeev @ 2019-08-28 11:47 UTC (permalink / raw)
  To: n.pettik; +Cc: tarantool-patches

Hi! Thank you fore review. My answers and new patch below.

On Fri, Aug 09, 2019 at 05:48:54PM +0300, n.pettik wrote:
> 
> 
> > 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.
> 
Fixed. Now all previous ways to set AUTOINCREMENT remained
unchanged.

> 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).
> 
Fixed. Now discussion can be found in dev mail 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.
> 
Fixed.

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

> > +  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”
> 
Fixed.

> Also, check error messages produced by other vendors.
> 
Something similar I found in MySQL, and its error is:
"Incorrect table definition; there can be only one auto
column and it must be defined as a key"

> > +    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). {
> 
Fixed.

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

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


New patch:

From f71cec86f8a19c60c2c71f78d25c473e7495b2e3 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Tue, 27 Aug 2019 14:03:58 +0300
Subject: [PATCH] sql: AUTOINCREMENT for multipart PK

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 consisted of more than one field. This patch defines
two ways to set AUTOINCREMENT for any INTEGER or UNSIGNED field of
PRIMARY KEY.

Closes #4217

@TarantoolBot document
Title: The auto-increment feature for multipart PK
The auto-increment feature can be set to any INTEGER or UNSIGNED
field of PRIMARY KEY using one of two ways:
1) AUTOINCREMENT in column definition:
CREATE TABLE t (i INT, a INT AUTOINCREMENT, PRIMARY KEY (i, a));
CREATE TABLE t (i INT AUTOINCREMENT, a INT, PRIMARY KEY (i, a));
2) AUTOINCREMENT in PRIMARY KEY definition:
CREATE TABLE t (i INT, a INT, PRIMARY KEY (i, a AUTOINCREMENT));
CREATE TABLE t (i INT, a INT, PRIMARY KEY (i AUTOINCREMENT, a));

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index b4f114e..eb0263b 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -521,12 +521,6 @@ sqlAddPrimaryKey(struct Parse *pParse)
 		sql_create_index(pParse);
 		if (db->mallocFailed)
 			goto primary_key_exit;
-	} else if (pParse->create_table_def.has_autoinc) {
-		diag_set(ClientError, ER_CREATE_SPACE, space->def->name,
-			 "AUTOINCREMENT is only allowed on an INTEGER PRIMARY "\
-			 "KEY or INT PRIMARY KEY");
-		pParse->is_aborted = true;
-		goto primary_key_exit;
 	} else {
 		sql_create_index(pParse);
 		pList = NULL;
@@ -943,11 +937,36 @@ emitNewSysSequenceRecord(Parse *pParse, int reg_seq_id, const char *seq_name)
 
 static int
 emitNewSysSpaceSequenceRecord(Parse *pParse, int reg_space_id, int reg_seq_id,
+			      struct space_def *space_def,
 			      struct index_def *idx_def)
 {
-	struct key_part *part = &idx_def->key_def->parts[0];
-	int fieldno = part->fieldno;
+	uint32_t i;
 	char *path = NULL;
+	uint32_t fieldno = pParse->create_table_def.autoinc_field_id;
+
+	char *name = pParse->create_table_def.autoinc_field_name;
+	if (name != NULL) {
+		for (i = 0; i < space_def->field_count; ++i) {
+			if (strcmp(space_def->fields[i].name, name) == 0)
+				break;
+		}
+		assert(i < space_def->field_count);
+		fieldno = i;
+	}
+
+	struct key_part *part;
+	for (i = 0; i < idx_def->key_def->part_count; ++i) {
+		part = &idx_def->key_def->parts[i];
+		if (part->fieldno == fieldno)
+			break;
+	}
+	if (i == idx_def->key_def->part_count) {
+		diag_set(ClientError, ER_MODIFY_INDEX, idx_def->name,
+			 space_def->name, "sequence field must be a part of "
+			 "the index");
+		pParse->is_aborted = true;
+		return -1;
+	}
 	if (part->path != NULL) {
 		path = sqlDbStrNDup(pParse->db, part->path, part->path_len);
 		if (path == NULL)
@@ -1249,9 +1268,11 @@ sqlEndTable(struct Parse *pParse)
 						 new_space->def->name);
 		sqlVdbeAddOp2(v, OP_SInsert, BOX_SEQUENCE_ID, reg_seq_record);
 		/* Do an insertion into _space_sequence. */
-		int reg_space_seq_record = emitNewSysSpaceSequenceRecord(pParse,
-							reg_space_id, reg_seq_id,
-							new_space->index[0]->def);
+		int reg_space_seq_record =
+			emitNewSysSpaceSequenceRecord(pParse, reg_space_id,
+						      reg_seq_id,
+						      new_space->def,
+						      new_space->index[0]->def);
 		sqlVdbeAddOp2(v, OP_SInsert, BOX_SPACE_SEQUENCE_ID,
 			      reg_space_seq_record);
 	}
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index be3c5c3..371da79 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -225,8 +225,34 @@ create_table_end ::= . { sqlEndTable(pParse); }
  */
 
 columnlist ::= columnlist COMMA tcons.
-columnlist ::= columnlist COMMA columnname carglist.
-columnlist ::= columnname carglist.
+columnlist ::= columnlist COMMA columnname carglist autoinc(I). {
+  if (I == 1) {
+    if (pParse->create_table_def.has_autoinc) {
+      diag_set(ClientError, ER_SQL_SYNTAX, "CREATE TABLE", "Table must feature "
+               "at most one AUTOINCREMENT field");
+      pParse->is_aborted = true;
+      return;
+    }
+    pParse->create_table_def.has_autoinc = true;
+    pParse->create_table_def.autoinc_field_id =
+      pParse->create_table_def.new_space->def->field_count - 1;
+    assert(pParse->create_table_def.autoinc_field_name == NULL);
+  }
+}
+columnlist ::= columnname carglist autoinc(I). {
+  if (I == 1) {
+    if (pParse->create_table_def.has_autoinc) {
+      diag_set(ClientError, ER_SQL_SYNTAX, "CREATE TABLE", "Table must feature "
+               "at most one AUTOINCREMENT field");
+      pParse->is_aborted = true;
+      return;
+    }
+    pParse->create_table_def.has_autoinc = true;
+    pParse->create_table_def.autoinc_field_id =
+      pParse->create_table_def.new_space->def->field_count - 1;
+    assert(pParse->create_table_def.autoinc_field_name == NULL);
+  }
+}
 columnlist ::= tcons.
 columnname(A) ::= nm(A) typedef(Y). {sqlAddColumn(pParse,&A,&Y);}
 
@@ -299,8 +325,7 @@ ccons ::= NULL onconf(R).        {
         sql_column_add_nullable_action(pParse, R);
 }
 ccons ::= NOT NULL onconf(R).    {sql_column_add_nullable_action(pParse, R);}
-ccons ::= cconsname(N) PRIMARY KEY sortorder(Z) autoinc(I). {
-  pParse->create_table_def.has_autoinc = I;
+ccons ::= cconsname(N) PRIMARY KEY sortorder(Z). {
   create_index_def_init(&pParse->create_index_def, NULL, &N, NULL,
                         SQL_INDEX_TYPE_CONSTRAINT_PK, Z, false);
   sqlAddPrimaryKey(pParse);
@@ -369,8 +394,7 @@ init_deferred_pred_opt(A) ::= .                       {A = 0;}
 init_deferred_pred_opt(A) ::= INITIALLY DEFERRED.     {A = 1;}
 init_deferred_pred_opt(A) ::= INITIALLY IMMEDIATE.    {A = 0;}
 
-tcons ::= cconsname(N) PRIMARY KEY LP sortlist(X) autoinc(I) RP. {
-  pParse->create_table_def.has_autoinc = I;
+tcons ::= cconsname(N) PRIMARY KEY LP sortlist_with_autoinc(X) RP. {
   create_index_def_init(&pParse->create_index_def, NULL, &N, X,
                         SQL_INDEX_TYPE_CONSTRAINT_PK, SORT_ORDER_ASC, false);
   sqlAddPrimaryKey(pParse);
@@ -760,6 +784,63 @@ sortlist(A) ::= expr(Y) sortorder(Z). {
   sqlExprListSetSortOrder(A,Z);
 }
 
+// the sortlist_with_autoinc non-terminal stores a list of
+// expression where each expression is a name of the column in
+// PRIMARY KEY definition and optionally followed by ASC or DESC
+// to indicate the sort order and by AUTOINCREMENT to indicate
+// that sequence should be attached to the column.
+//
+%type sortlist_with_autoinc {ExprList*}
+%destructor sortlist_with_autoinc {sql_expr_list_delete(pParse->db, $$);}
+
+sortlist_with_autoinc(A) ::= sortlist_with_autoinc(A) COMMA expr(Y) sortorder(Z) autoinc(I). {
+  if (I == 1) {
+    if (pParse->create_table_def.has_autoinc) {
+      diag_set(ClientError, ER_SQL_SYNTAX, "CREATE TABLE", "Table must feature "
+               "at most one AUTOINCREMENT field");
+      pParse->is_aborted = true;
+      return;
+    }
+    pParse->create_table_def.has_autoinc = true;
+    struct Expr *name = sqlExprSkipCollate(Y.pExpr);
+    if (name->op != TK_ID) {
+      diag_set(ClientError, ER_INDEX_DEF_UNSUPPORTED, "Expressions");
+      pParse->is_aborted = true;
+      return;
+    }
+    char *name_str = region_alloc(&pParse->region, strlen(name->u.zToken) + 1);
+    strcpy(name_str, name->u.zToken);
+    pParse->create_table_def.autoinc_field_name = name_str;
+    assert(pParse->create_table_def.autoinc_field_id == 0);
+  }
+  A = sql_expr_list_append(pParse->db,A,Y.pExpr);
+  sqlExprListSetSortOrder(A,Z);
+}
+sortlist_with_autoinc(A) ::= expr(Y) sortorder(Z) autoinc(I). {
+  if (I == 1) {
+    if (pParse->create_table_def.has_autoinc) {
+      diag_set(ClientError, ER_SQL_SYNTAX, "CREATE TABLE", "Table must feature "
+               "at most one AUTOINCREMENT field");
+      pParse->is_aborted = true;
+      return;
+    }
+    pParse->create_table_def.has_autoinc = true;
+    struct Expr *name = sqlExprSkipCollate(Y.pExpr);
+    if (name->op != TK_ID) {
+      diag_set(ClientError, ER_INDEX_DEF_UNSUPPORTED, "Expressions");
+      pParse->is_aborted = true;
+      return;
+    }
+    char *name_str = region_alloc(&pParse->region, strlen(name->u.zToken) + 1);
+    strcpy(name_str, name->u.zToken);
+    pParse->create_table_def.autoinc_field_name = name_str;
+    assert(pParse->create_table_def.autoinc_field_id == 0);
+  }
+  /* A-overwrites-Y. */
+  A = sql_expr_list_append(pParse->db,NULL,Y.pExpr);
+  sqlExprListSetSortOrder(A,Z);
+}
+
 %type sortorder {int}
 
 sortorder(A) ::= ASC.           {A = SORT_ORDER_ASC;}
diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
index 557e415..e4c153a 100644
--- a/src/box/sql/parse_def.h
+++ b/src/box/sql/parse_def.h
@@ -214,6 +214,16 @@ struct create_table_def {
 	struct rlist new_check;
 	/** True, if table to be created has AUTOINCREMENT PK. */
 	bool has_autoinc;
+	/**
+	 * Id of field with AUTOINCREMENT in case this keyword
+	 * follow definition of the field.
+	 */
+	uint32_t autoinc_field_id;
+	/**
+	 * Name of field with AUTOINCREMENT in case this keyword
+	 * defined in PK definition.
+	 */
+	char *autoinc_field_name;
 };
 
 struct create_view_def {
@@ -461,6 +471,8 @@ create_table_def_init(struct create_table_def *table_def, struct Token *name,
 			       if_not_exists);
 	rlist_create(&table_def->new_fkey);
 	rlist_create(&table_def->new_check);
+	table_def->autoinc_field_id = 0;
+	table_def->autoinc_field_name = NULL;
 }
 
 static inline void
diff --git a/test/sql-tap/autoinc.test.lua b/test/sql-tap/autoinc.test.lua
index 1d0be8a..de466e6 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(55)
 
 --!./tcltestrunner.lua
 -- 2004 November 12
@@ -561,7 +561,7 @@ test:do_catchsql_test(
         CREATE TABLE t8(x TEXT PRIMARY KEY AUTOINCREMENT);
     ]], {
         -- <autoinc-7.2>
-        1, "Failed to create space 'T8': AUTOINCREMENT is only allowed on an INTEGER PRIMARY KEY or INT PRIMARY KEY"
+        1, "Can't create or modify index 'pk_unnamed_T8_1' in space 'T8': sequence cannot be used with a non-integer key"
         -- </autoinc-7.2>
     })
 
@@ -814,4 +814,97 @@ test:do_catchsql_test(
         -- </autoinc-gh-3670>
     })
 
+--
+-- gh-4217: make sure that AUTOINCREMENT can be used for any
+-- INTEGER field of PRIMARY KEY.
+--
+
+test:do_execsql_test(
+    "autoinc-11.1",
+    [[
+        CREATE TABLE t11_1 (i INT AUTOINCREMENT, a INT, PRIMARY KEY(i, a));
+        INSERT INTO t11_1 VALUES (NULL, 1), (NULL, 1), (NULL, 1);
+        SELECT * FROM t11_1;
+    ]], {
+        -- <autoinc-11.1>
+        1, 1, 2, 1, 3, 1
+        -- </autoinc-11.1>
+    })
+
+test:do_execsql_test(
+    "autoinc-11.2",
+    [[
+        CREATE TABLE t11_2 (i INT AUTOINCREMENT, a INT, PRIMARY KEY(a, i));
+        INSERT INTO t11_2 VALUES (NULL, 1), (NULL, 1), (NULL, 1);
+        SELECT * FROM t11_2;
+    ]], {
+        -- <autoinc-11.2>
+        1, 1, 2, 1, 3, 1
+        -- </autoinc-11.2>
+    })
+
+test:do_execsql_test(
+    "autoinc-11.3",
+    [[
+        CREATE TABLE t11_3 (i INT, a INT, PRIMARY KEY(i AUTOINCREMENT, a));
+        INSERT INTO t11_3 VALUES (NULL, 1), (NULL, 1), (NULL, 1);
+        SELECT * FROM t11_3;
+    ]], {
+        -- <autoinc-11.3>
+        1, 1, 2, 1, 3, 1
+        -- </autoinc-11.3>
+    })
+
+test:do_execsql_test(
+    "autoinc-11.4",
+    [[
+        CREATE TABLE t11_4 (i INT, a INT, PRIMARY KEY(a, i AUTOINCREMENT));
+        INSERT INTO t11_4 VALUES (NULL, 1), (NULL, 1), (NULL, 1);
+        SELECT * FROM t11_4;
+    ]], {
+        -- <autoinc-11.4>
+        1, 1, 2, 1, 3, 1
+        -- </autoinc-11.4>
+    })
+
+test:do_catchsql_test(
+    "autoinc-11.5",
+    [[
+        CREATE TABLE t11_5 (i INT, a INT, PRIMARY KEY(a, i COLLATE "unicode_ci" AUTOINCREMENT));
+    ]], {
+        -- <autoinc-11.5>
+        1, "Wrong index options (field 2): collation is reasonable only for string and scalar parts"
+        -- </autoinc-11.5>
+    })
+
+test:do_catchsql_test(
+    "autoinc-11.6",
+    [[
+        CREATE TABLE t11_6 (i INT, a INT, b INT AUTOINCREMENT, PRIMARY KEY(a, i));
+    ]], {
+        -- <autoinc-11.6>
+        1, "Can't create or modify index 'pk_unnamed_T11_6_1' in space 'T11_6': sequence field must be a part of the index"
+        -- </autoinc-11.6>
+    })
+
+test:do_catchsql_test(
+    "autoinc-11.7",
+    [[
+        CREATE TABLE t11_7 (i INT AUTOINCREMENT, a INT AUTOINCREMENT, PRIMARY KEY(a, i));
+    ]], {
+        -- <autoinc-11.7>
+        1, "Syntax error in CREATE TABLE: Table must feature at most one AUTOINCREMENT field"
+        -- </autoinc-11.7>
+    })
+
+test:do_catchsql_test(
+    "autoinc-11.8",
+    [[
+        CREATE TABLE t11_8 (i INT, a INT, PRIMARY KEY(a AUTOINCREMENT, i AUTOINCREMENT));
+    ]], {
+        -- <autoinc-11.8>
+        1, "Syntax error in CREATE TABLE: Table must feature at most one AUTOINCREMENT field"
+        -- </autoinc-11.8>
+    })
+
 test:finish_test()
diff --git a/test/sql-tap/sql-errors.test.lua b/test/sql-tap/sql-errors.test.lua
index d11c5c5..f56bcc0 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>
 	})
 

^ permalink raw reply	[flat|nested] 4+ messages in thread

[parent not found: <20190918172225.GB89644@tarantool.org>]

[parent not found: <20190921072240.GA9659@tarantool.org>]

[parent not found: <20190925110445.GA32291@tarantool.org>]

[parent not found: <20190926132044.GA10775@tarantool.org>]

[parent not found: <20190930135607.GA58871@tarantool.org>]

[parent not found: <20191009143056.GA12954@tarantool.org>]

* Re: [Tarantool-patches] [tarantool-patches] [PATCH v1 1/1] sql: change the way to set AUTOINCREMENT
       [not found]               ` <20191009143056.GA12954@tarantool.org>
@ 2019-10-14 12:28                 ` Nikita Pettik
  0 siblings, 0 replies; 4+ messages in thread
From: Nikita Pettik @ 2019-10-14 12:28 UTC (permalink / raw)
  To: Mergen Imeev; +Cc: tarantool-patches, tarantool-patches

On 09 Oct 17:30, Mergen Imeev wrote:
> Hi! Thank you for review. My answers, diff and new patch
> below.

Pushed with following code-style changes:

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index edaf5d09c..233f56734 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -3221,7 +3221,6 @@ int
 sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name,
                    uint32_t *fieldno)
 {
-       uint32_t i;
        struct space_def *def = parse_context->create_table_def.new_space->def;
        struct Expr *name = sqlExprSkipCollate(field_name);
        if (name->op != TK_ID) {
@@ -3229,6 +3228,7 @@ sql_fieldno_by_name(struct Parse *parse_context, struct Expr *field_name,
                parse_context->is_aborted = true;
                return -1;
        }
+       uint32_t i;
        for (i = 0; i < def->field_count; ++i) {
                if (strcmp(def->fields[i].name, name->u.zToken) == 0)
                        break;
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 639f409ae..ed59a875a 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -230,6 +230,7 @@ columnlist ::= columnlist COMMA columnname carglist autoinc(I). {
   if (I == 1 && sql_add_autoincrement(pParse, fieldno) != 0)
     return;
 }
+
 columnlist ::= columnname carglist autoinc(I). {
   uint32_t fieldno = pParse->create_table_def.new_space->def->field_count - 1;
   if (I == 1 && sql_add_autoincrement(pParse, fieldno) != 0)
@@ -376,7 +377,7 @@ init_deferred_pred_opt(A) ::= .                       {A = 0;}
 init_deferred_pred_opt(A) ::= INITIALLY DEFERRED.     {A = 1;}
 init_deferred_pred_opt(A) ::= INITIALLY IMMEDIATE.    {A = 0;}
 
-tcons ::= cconsname(N) PRIMARY KEY LP sortlist_with_autoinc(X) RP. {
+tcons ::= cconsname(N) PRIMARY KEY LP col_list_with_autoinc(X) RP. {
   create_index_def_init(&pParse->create_index_def, NULL, &N, X,
                         SQL_INDEX_TYPE_CONSTRAINT_PK, SORT_ORDER_ASC, false);
   sqlAddPrimaryKey(pParse);
@@ -767,26 +768,32 @@ sortlist(A) ::= expr(Y) sortorder(Z). {
 }
 
 /**
- * The sortlist_with_autoinc rule works in the same way as the
- * sortlist rule, except that it can work with the AUTOINCREMENT
- * keyword and cannot specify the sort order.
+ * Non-terminal rule to store a list of columns within PRIMARY KEY
+ * declaration.
  */
-%type sortlist_with_autoinc {ExprList*}
-%destructor sortlist_with_autoinc {sql_expr_list_delete(pParse->db, $$);}
+%type col_list_with_autoinc {ExprList*}
+%destructor col_list_with_autoinc {sql_expr_list_delete(pParse->db, $$);}
 
-sortlist_with_autoinc(A) ::= sortlist_with_autoinc(A) COMMA expr(Y)
+col_list_with_autoinc(A) ::= col_list_with_autoinc(A) COMMA expr(Y)
                              autoinc(I). {
   uint32_t fieldno;
-  if (I == 1 && (sql_fieldno_by_name(pParse, Y.pExpr, &fieldno) != 0 ||
-                 sql_add_autoincrement(pParse, fieldno) != 0))
-    return;
+  if (I == 1) {
+    if (sql_fieldno_by_name(pParse, Y.pExpr, &fieldno) != 0)
+      return;
+    if (sql_add_autoincrement(pParse, fieldno) != 0)
+      return;
+  }
   A = sql_expr_list_append(pParse->db, A, Y.pExpr);
 }
-sortlist_with_autoinc(A) ::= expr(Y) autoinc(I). {
-  uint32_t fieldno;
-  if (I == 1 && (sql_fieldno_by_name(pParse, Y.pExpr, &fieldno) != 0 ||
-                 sql_add_autoincrement(pParse, fieldno) != 0))
-    return;
+
+col_list_with_autoinc(A) ::= expr(Y) autoinc(I). {
+  if (I == 1) {
+    uint32_t fieldno = 0;
+    if (sql_fieldno_by_name(pParse, Y.pExpr, &fieldno) != 0)
+      return;
+    if (sql_add_autoincrement(pParse, fieldno) != 0)
+      return;
+  }
   /* A-overwrites-Y. */
   A = sql_expr_list_append(pParse->db, NULL, Y.pExpr);
 }
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index de4a96030..35fc81dfb 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4424,16 +4424,20 @@ vdbe_emit_stat_space_clear(struct Parse *parse, const char *stat_table_name,
  * of PRIMARY KEY.
  *
  * @param parse_context Parsing context.
- * @param fieldno Field number in new space format.
+ * @param fieldno Field number in space format under construction.
  *
  * @retval 0 on success.
- * @retval -1 on error.
+ * @retval -1 if table already has declared AUTOINCREMENT feature.
  */
 int
 sql_add_autoincrement(struct Parse *parse_context, uint32_t fieldno);
 
 /**
- * Get fieldno by field name.
+ * Get fieldno by field name. At the moment of forming space format
+ * there's no tuple dictionary, so we can't use hash, in contrast to
+ * tuple_fieldno_by_name(). However, AUTOINCREMENT can occur at most
+ * once in table's definition, so it's not a big deal if we use O(n)
+ * search.
  *
  * @param parse_context Parsing context.
  * @param field_name Expr that contains field name.
diff --git a/test/sql-tap/autoinc.test.lua b/test/sql-tap/autoinc.test.lua
index 83d62d723..39e47966b 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(56)
+test:plan(57)
 
 --!./tcltestrunner.lua
 -- 2004 November 12
@@ -826,9 +826,7 @@ test:do_execsql_test(
         INSERT INTO t11_1 VALUES (NULL, 1), (NULL, 1), (NULL, 1);
         SELECT * FROM t11_1;
     ]], {
-        -- <autoinc-11.1>
         1, 1, 2, 1, 3, 1
-        -- </autoinc-11.1>
     })
 
 test:do_execsql_test(
@@ -838,9 +836,7 @@ test:do_execsql_test(
         INSERT INTO t11_2 VALUES (NULL, 1), (NULL, 1), (NULL, 1);
         SELECT * FROM t11_2;
     ]], {
-        -- <autoinc-11.2>
         1, 1, 2, 1, 3, 1
-        -- </autoinc-11.2>
     })
 
 test:do_execsql_test(
@@ -850,9 +846,7 @@ test:do_execsql_test(
         INSERT INTO t11_3 VALUES (NULL, 1), (NULL, 1), (NULL, 1);
         SELECT * FROM t11_3;
     ]], {
-        -- <autoinc-11.3>
         1, 1, 2, 1, 3, 1
-        -- </autoinc-11.3>
     })
 
 test:do_execsql_test(
@@ -862,9 +856,7 @@ test:do_execsql_test(
         INSERT INTO t11_4 VALUES (NULL, 1), (NULL, 1), (NULL, 1);
         SELECT * FROM t11_4;
     ]], {
-        -- <autoinc-11.4>
         1, 1, 2, 1, 3, 1
-        -- </autoinc-11.4>
     })
 
 test:do_catchsql_test(
@@ -872,9 +864,7 @@ test:do_catchsql_test(
     [[
         CREATE TABLE t11_5 (i INT, a INT, PRIMARY KEY(a, i COLLATE "unicode_ci" AUTOINCREMENT));
     ]], {
-        -- <autoinc-11.5>
         1, "Wrong index options (field 2): collation is reasonable only for string and scalar parts"
-        -- </autoinc-11.5>
     })
 
 test:do_catchsql_test(
@@ -882,9 +872,7 @@ test:do_catchsql_test(
     [[
         CREATE TABLE t11_6 (i INT, a INT, b INT AUTOINCREMENT, PRIMARY KEY(a, i));
     ]], {
-        -- <autoinc-11.6>
         1, "Can't create or modify index 'pk_unnamed_T11_6_1' in space 'T11_6': sequence field must be a part of the index"
-        -- </autoinc-11.6>
     })
 
 test:do_catchsql_test(
@@ -892,9 +880,7 @@ test:do_catchsql_test(
     [[
         CREATE TABLE t11_7 (i INT AUTOINCREMENT, a INT AUTOINCREMENT, PRIMARY KEY(a, i));
     ]], {
-        -- <autoinc-11.7>
         1, "Syntax error in CREATE TABLE: Table must feature at most one AUTOINCREMENT field"
-        -- </autoinc-11.7>
     })
 
 test:do_catchsql_test(
@@ -902,19 +888,23 @@ test:do_catchsql_test(
     [[
         CREATE TABLE t11_8 (i INT, a INT, PRIMARY KEY(a AUTOINCREMENT, i AUTOINCREMENT));
     ]], {
-        -- <autoinc-11.8>
         1, "Syntax error in CREATE TABLE: Table must feature at most one AUTOINCREMENT field"
-        -- </autoinc-11.8>
     })
 
 test:do_catchsql_test(
-    "autoinc-11.8",
+    "autoinc-11.9",
     [[
         CREATE TABLE t11_9 (i INT, PRIMARY KEY(a AUTOINCREMENT), a INT);
     ]], {
-        -- <autoinc-11.8>
         1, "Can’t resolve field 'A'"
-        -- </autoinc-11.8>
     })
 
+test:do_catchsql_test(
+    "autoinc-11.10",
+    [[
+        CREATE TABLE t11_8 (i INT, a INT, PRIMARY KEY(a, 1 AUTOINCREMENT));
+    ]], {
+        1, "Expressions are prohibited in an index definition"
+})
+
 

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2019-10-14 12:28 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-27 10:29 [tarantool-patches] [PATCH v1 1/1] sql: change the way to set AUTOINCREMENT imeevma
2019-08-09 14:48 ` [tarantool-patches] " n.pettik
2019-08-28 11:47   ` Mergen Imeev
     [not found]     ` <20190918172225.GB89644@tarantool.org>
     [not found]       ` <20190921072240.GA9659@tarantool.org>
     [not found]         ` <20190925110445.GA32291@tarantool.org>
     [not found]           ` <20190926132044.GA10775@tarantool.org>
     [not found]             ` <20190930135607.GA58871@tarantool.org>
     [not found]               ` <20191009143056.GA12954@tarantool.org>
2019-10-14 12:28                 ` [Tarantool-patches] [tarantool-patches] " Nikita Pettik

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox