From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 52B6F26A19 for ; Fri, 9 Aug 2019 10:48:58 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id iMYQ0faJTf4G for ; Fri, 9 Aug 2019 10:48:58 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 7A5DA2699D for ; Fri, 9 Aug 2019 10:48:57 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: change the way to set AUTOINCREMENT From: "n.pettik" In-Reply-To: Date: Fri, 9 Aug 2019 17:48:54 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <35811B45-3F54-4D97-9D1C-E1437A043949@tarantool.org> References: Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org Cc: Imeev Mergen > On 27 Jul 2019, at 13:29, imeevma@tarantool.org wrote: >=20 > 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. >=20 > Example of old definition: > CREATE TABLE t (i INT, a INT, PRIMARY KEY (a AUTOINCREMENT)); >=20 > Example of new definition: > CREATE TABLE t (i INT, a INT AUTOINCREMENT, PRIMARY KEY (a)); >=20 > Closes #4217 >=20 > @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=E2=80=99s obvious and I guess already documented. Instead, I=E2=80=99d 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. >=20 > 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)); > --- >=20 > @@ -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 !=3D UINT32_MAX) { Why don=E2=80=99t simply use has_autoinc + autoinc_field? It wouldn=E2=80=99t 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 ::=3D NULL onconf(R). { > if (R !=3D ON_CONFLICT_ACTION_ABORT) > sql_column_add_nullable_action(pParse, R); > } > +ccons ::=3D 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' =E2=80=A6 Previous version looked better IMHO: - Syntax error near 'AUTOINCREMENT' ... > + if (pParse->create_table_def.autoinc_field !=3D UINT32_MAX) { > + diag_set(ClientError, ER_SQL_SYNTAX, "CREATE TABLE", "statement = cannot " > + "have more than one AUTOINCREMENT=E2=80=9D); Statement can=E2=80=99t have autoinrement specifier by definition - it=E2=80=99s table=E2=80=99s property. Thus, more accurate error message would be: =E2=80=9CTable can=E2=80=99t feature more than one AUTOINCREMENT = specifier/field=E2=80=9D OR =E2=80=9CTable must feature at most one AUTOINCREMENT specifier/field=E2=80= =9D Also, check error messages produced by other vendors. > + pParse->is_aborted =3D 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 ::=3D AUTOINCR. { diag_set(ClientError, ER_SQL_SYNTAX, "CREATE TABLE", "statement = cannot " "have more than one AUTOINCREMENT"); pParse->is_aborted =3D true; - } else { - pParse->create_table_def.autoinc_field =3D - pParse->create_table_def.new_space->def->field_count - 1; + return; } + pParse->create_table_def.autoinc_field =3D + pParse->create_table_def.new_space->def->field_count - 1; } ccons ::=3D NOT NULL onconf(R). = {sql_column_add_nullable_action(pParse, R);} ccons ::=3D 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 =3D require("sqltester") > -test:plan(47) > +test:plan(58) >=20 > --!./tcltestrunner.lua > -- 2004 November 12 > @@ -537,11 +537,13 @@ test:do_catchsql_test( >=20 >=20 > +test:do_catchsql_test( > + "autoinc-11.6", > + [[ > + CREATE TABLE t (i INT, a INT, b INT AUTOINCREMENT, PRIMARY = KEY (a, i)); > + ]], { > + -- > + 1, "Can't create or modify index 'pk_unnamed_T_1' in space = 'T': sequence field must be a part of the index" > + -- > + }) > + > +-- Make sure that AUTOINCREMENT only works for INTEGER field. Test below doesn=E2=80=99t 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); > + ]], { > + -- > + 1, "Can't create or modify index 'pk_unnamed_T_1' in space = 'T': sequence cannot be used with a non-integer key" > + -- > + }) > + > +test:do_catchsql_test( > + "autoinc-11.8", > + [[ > + CREATE TABLE t (i REAL PRIMARY KEY AUTOINCREMENT); > + ]], { > + -- > + 1, "Can't create or modify index 'pk_unnamed_T_1' in space = 'T': sequence cannot be used with a non-integer key" > + -- > + }) > + > +test:do_catchsql_test( > + "autoinc-11.9", > + [[ > + CREATE TABLE t (i BOOLEAN PRIMARY KEY AUTOINCREMENT); > + ]], { > + -- > + 1, "Can't create or modify index 'pk_unnamed_T_1' in space = 'T': sequence cannot be used with a non-integer key" > + -- > + }) > + > +test:do_catchsql_test( > + "autoinc-11.10", > + [[ > + CREATE TABLE t (i SCALAR PRIMARY KEY AUTOINCREMENT); > + ]], { > + -- > + 1, "Can't create or modify index 'pk_unnamed_T_1' in space = 'T': sequence cannot be used with a non-integer key" > + -- > + }) > + > +-- 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)); > + ]], { > + -- > + 1, "Syntax error in CREATE TABLE: statement cannot have more = than one AUTOINCREMENT" > + -- > + }) > + > 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); > ]], { > -- > - 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" > -- > }) >=20 > --=20 > 2.7.4 >=20 >=20