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 E54282B51C for ; Fri, 29 Mar 2019 14:16:33 -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 nRr66FVN_eLg for ; Fri, 29 Mar 2019 14:16:33 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 655132B4FC for ; Fri, 29 Mar 2019 14:16:33 -0400 (EDT) From: "n.pettik" Message-Id: Content-Type: multipart/alternative; boundary="Apple-Mail=_5CE64BDB-88EB-45EA-90ED-8CFADFFAC613" Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH v2 4/4] sql: introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PRIMARY KEY Date: Fri, 29 Mar 2019 21:16:31 +0300 In-Reply-To: <07f3d310-a7d0-e843-38e3-139074584ced@tarantool.org> References: <07f3d310-a7d0-e843-38e3-139074584ced@tarantool.org> 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: Vladislav Shpilevoy --Apple-Mail=_5CE64BDB-88EB-45EA-90ED-8CFADFFAC613 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 > On 28 Mar 2019, at 18:11, Vladislav Shpilevoy = wrote: >=20 > Thanks for the patch! See 5 comments below, review fixes at > the bottom of the email and on the branch. >=20 > On 28/03/2019 15:07, Nikita Pettik wrote: >> Table (aka space) can be created without indexes at least from = Lua-land >> (note that according ANSI SQL table may lack PK). Since there were no >> facilities to create primary key constraint on already existing = table, >> lets extend ADD CONSTRAINT statement with UNIQUE and PRIMARY KEY >> clauses. In this case, UNIQUE works exactly in the same way as CREATE >> UNIQUE INDEX ... statement does. In Tarantool primary index is an = index >> with id =3D=3D 0, so during execution of ADD CONSTRAINT PRIMARY KEY = we check >> that there is no any entries in _index space and create that one. >> Otherwise, error is raised. >>=20 >> Part of #3097 >=20 > 1. Why not 'Closes #3097=E2=80=99? Because ADD CONSTRAINT CHECK is still not yet implemented. >> diff --git a/src/box/sql/build.c b/src/box/sql/build.c >> index 20cc346a0..1f604cfe0 100644 >> --- a/src/box/sql/build.c >> +++ b/src/box/sql/build.c >> @@ -1956,6 +1956,20 @@ generate_index_id(struct Parse *parse, = uint32_t space_id, int cursor) >> return iid_reg; >> } >>=20 >> +static void >=20 > 2. A comment is usually required when a function is not a > one-liner. >=20 >> +vdbe_emit_pk_existence_check(struct Parse *parse, uint32_t space_id, >> + int _index_cursor) >> +{ >> + struct Vdbe *v =3D sqlGetVdbe(parse); >> + int tmp_reg =3D ++parse->nMem; >=20 > 3. Why 'tmp'? As I see, you use a normal register, not temporary. >=20 >> + sqlVdbeAddOp2(v, OP_Integer, space_id, tmp_reg); >> + int found_addr =3D sqlVdbeAddOp4Int(v, OP_NotFound, = _index_cursor, 0, >> + tmp_reg, 1); >> + sqlVdbeAddOp4(v, OP_Halt, SQL_ERROR, ON_CONFLICT_ACTION_FAIL, 0, >> + "multiple primary keys are not allowed", = P4_STATIC); >> + sqlVdbeJumpHere(v, found_addr); >> +} >> + >=20 > 4. I noticed that this function in a nutshell does almost the > same as vdbe_emit_new_index_id, but treats the result differently. > It searches for a record in _index. So I extracted that part into > a separate function vdbe_emit_space_index_search - it positions > a cursor onto the biggest value in _index with a needed space_id > and returns two addresses: first to jump from when a record is > found, and a second to jump from when it is not. IMO it would be > better in encapsulate _index key creation and lookup since it > is used in two places. >=20 >> /** >> * Add new index to table's indexes list. >> * We follow convention that PK comes first in list. >> diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y >> index 0c4603e83..ed5c05436 100644 >> --- a/src/box/sql/parse.y >> +++ b/src/box/sql/parse.y >> @@ -1624,6 +1624,16 @@ cmd ::=3D alter_add_constraint(N) FOREIGN KEY = LP eidlist(FA) RP REFERENCES >> sql_create_foreign_key(pParse); >> } >>=20 >> +cmd ::=3D alter_add_constraint(N) unique_spec(U) LP sortlist(X) RP. = { >> + create_index_def_init(&pParse->create_index_def, N.table_name, = &N.name, X, U, >> + SORT_ORDER_ASC, false); >> + sql_create_index(pParse); >> +} >> + >> +%type unique_spec {int} >=20 > 5. We have a enum for these values. Ok. I see you=E2=80=99ve already fixed all points, so I=E2=80=99m just applying your patch. --Apple-Mail=_5CE64BDB-88EB-45EA-90ED-8CFADFFAC613 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8

On 28 Mar 2019, at 18:11, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:

Thanks for the patch! See 5 = comments below, review fixes at
the bottom of the email and on the branch.

On 28/03/2019 15:07, Nikita = Pettik wrote:
Table = (aka space) can be created without indexes at least from Lua-land
(note that according ANSI SQL table may lack PK). Since there = were no
facilities to create primary key constraint on = already existing table,
lets extend ADD CONSTRAINT = statement with UNIQUE and PRIMARY KEY
clauses. In this = case, UNIQUE works exactly in the same way as CREATE
UNIQUE = INDEX ... statement does.  In Tarantool primary index is an = index
with id =3D=3D 0, so during execution of ADD = CONSTRAINT PRIMARY KEY we check
that there is no any = entries in _index space and create that one.
Otherwise, = error is raised.

Part of #3097

1. Why not 'Closes #3097=E2=80=99?

Because ADD = CONSTRAINT CHECK is still not yet implemented.

diff = --git a/src/box/sql/build.c b/src/box/sql/build.c
index = 20cc346a0..1f604cfe0 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1956,6 +1956,20 = @@ generate_index_id(struct Parse *parse, uint32_t space_id, int = cursor)
return iid_reg;
}

+static void

2. A comment is usually required = when a function is not a
one-liner.

+vdbe_emit_pk_existence_check(struct = Parse *parse, uint32_t space_id,
+      int = _index_cursor)
+{
+ struct = Vdbe *v =3D sqlGetVdbe(parse);
+ int = tmp_reg =3D ++parse->nMem;

3. Why 'tmp'? As I see, you use = a normal register, not temporary.

+ sqlVdbeAddOp2(v, OP_Integer, = space_id, tmp_reg);
+ int found_addr =3D = sqlVdbeAddOp4Int(v, OP_NotFound, _index_cursor, 0,
+   tmp_reg, 1);
+ = sqlVdbeAddOp4(v, OP_Halt, SQL_ERROR, ON_CONFLICT_ACTION_FAIL, = 0,
+       = ;"multiple primary keys are not allowed", P4_STATIC);
+ = sqlVdbeJumpHere(v, found_addr);
+}
+

4. I noticed that this function in a nutshell does almost = the
same as = vdbe_emit_new_index_id, but treats the result differently.
It searches for a record in = _index. So I extracted that part into
a separate function vdbe_emit_space_index_search - it = positions
a cursor onto = the biggest value in _index with a needed space_id
and returns two addresses: first = to jump from when a record is
found, and a second to jump from when it is not. IMO it would = be
better in = encapsulate _index key creation and lookup since it
is used in two places.

/**
 * Add new index to table's indexes list.
 * We follow convention that PK comes first in list.
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 0c4603e83..ed5c05436 100644
--- = a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -1624,6 +1624,16 @@ cmd ::=3D alter_add_constraint(N) = FOREIGN KEY LP eidlist(FA) RP REFERENCES
  sql_create_foreign_key(pParse);
}

+cmd ::=3D alter_add_constraint(N) = unique_spec(U) LP sortlist(X) RP. {
+ =  create_index_def_init(&pParse->create_index_def, = N.table_name, &N.name, X, U,
+ =             &n= bsp;          SORT_ORDER= _ASC, false);
+  sql_create_index(pParse);
+}
+
+%type unique_spec {int}

5. We have a enum for these values.

Ok. I see = you=E2=80=99ve already fixed all points, so I=E2=80=99m
just = applying your patch.


= --Apple-Mail=_5CE64BDB-88EB-45EA-90ED-8CFADFFAC613--