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 35B3024AD6 for ; Wed, 16 Jan 2019 15:06:47 -0500 (EST) 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 BBOkcIwRl9Je for ; Wed, 16 Jan 2019 15:06:47 -0500 (EST) 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 E711425DCB for ; Wed, 16 Jan 2019 15:06:46 -0500 (EST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.0 \(3445.100.39\)) Subject: [tarantool-patches] Re: [PATCH 6/6] sql: introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PRIMARY KEY From: "n.pettik" In-Reply-To: <3804c32a-4e68-d876-b811-119ff669852b@tarantool.org> Date: Wed, 16 Jan 2019 23:06:45 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <5b411f9988366aee0418ed4aeb01243a85cfbd03.1547035184.git.korablev@tarantool.org> <3804c32a-4e68-d876-b811-119ff669852b@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 >> diff --git a/src/box/sql/build.c b/src/box/sql/build.c >> index 1d7b6c827..0314be957 100644 >> --- a/src/box/sql/build.c >> +++ b/src/box/sql/build.c >> @@ -2083,6 +2083,19 @@ generate_index_id(struct Parse *parse, = uint32_t space_id, int cursor) >> return iid_reg; >> } >> +static void >> +pk_check_existence(struct Parse *parse, uint32_t space_id, int = cursor) >=20 > 1. It is worth mentioning in a comment that the cursor is opened on = _index > space. Or rename it to _index_cursor. It do not mind if in such a case = you > omit a comment. I=E2=80=99d prefer second option: diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 080914576..cdeb85bd7 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -2089,12 +2089,12 @@ generate_index_id(struct Parse *parse, uint32_t = space_id, int cursor) } =20 static void -pk_check_existence(struct Parse *parse, uint32_t space_id, int cursor) +pk_check_existence(struct Parse *parse, uint32_t space_id, int = _index_cursor) { struct Vdbe *v =3D sqlite3GetVdbe(parse); int tmp_reg =3D ++parse->nMem; sqlite3VdbeAddOp2(v, OP_Integer, space_id, tmp_reg); - int found_addr =3D sqlite3VdbeAddOp4Int(v, OP_NotFound, cursor, = 0, + int found_addr =3D sqlite3VdbeAddOp4Int(v, OP_NotFound, = _index_cursor, 0, >=20 >> +{ >> + struct Vdbe *v =3D sqlite3GetVdbe(parse); >> + int tmp_reg =3D ++parse->nMem; >=20 > 2. Why not to use a truly temp register? As I know, we have facilities = for > it. Parse.nMem are not temporary. Parse.nTempReg - is. I am afraid that they don=E2=80=99t work. I greped through code a bit = and found no real usages of tmp regs. What is more: int sqlite3GetTempReg(Parse * pParse) { if (pParse->nTempReg =3D=3D 0) { return ++pParse->nMem; } return pParse->aTempReg[--pParse->nTempReg]; } It always returns real register since nTempReg =3D=3D 0 and is never incremented. I=E2=80=99ve opened issue: https://github.com/tarantool/tarantool/issues/3942 https://github.com/tarantool/tarantool/issues/3943 >> + sqlite3VdbeAddOp2(v, OP_Integer, space_id, tmp_reg); >> + int found_addr =3D sqlite3VdbeAddOp4Int(v, OP_NotFound, cursor, = 0, >> + tmp_reg, 1); >> + sqlite3VdbeAddOp4(v, OP_Halt, SQLITE_ERROR, = ON_CONFLICT_ACTION_FAIL, 0, >> + "multiple primary keys are not allowed", = P4_STATIC); >> + sqlite3VdbeJumpHere(v, found_addr); >> +} >> + >> /** >> * Add new index to table's indexes list. >> * We follow convention that PK comes first in list. >> diff --git a/test/sql-tap/alter.test.lua = b/test/sql-tap/alter.test.lua >> index 1aad555c0..925753749 100755 >> --- a/test/sql-tap/alter.test.lua >> +++ b/test/sql-tap/alter.test.lua >> @@ -517,6 +517,62 @@ test:do_catchsql_test( >> -- >> }) >> +test:do_test( >> + "alter-8.1.0", >> + function() >> + format =3D {} >> + format[1] =3D { name =3D 'id', type =3D 'scalar'} >> + format[2] =3D { name =3D 'f2', type =3D 'scalar'} >> + s =3D box.schema.create_space('T', {format =3D format}) >> + end, >> + {}) >> + >> +test:do_catchsql_test( >> + "alter-8.1.1", >> + [[ >> + ALTER TABLE t ADD CONSTRAINT pk PRIMARY KEY("id"); >> + ]], { >> + 0 >> + }) >> + >> +test:do_test( >> + "alter-8.1.2", >> + function() >> + i =3D box.space.T.index[0] >> + return i.id >=20 > 3. Why not return box.space.T.index[0].id? =E2=80=A6 diff --git a/test/sql-tap/alter.test.lua b/test/sql-tap/alter.test.lua index 925753749..318b0f68d 100755 --- a/test/sql-tap/alter.test.lua +++ b/test/sql-tap/alter.test.lua @@ -538,8 +538,7 @@ test:do_catchsql_test( test:do_test( "alter-8.1.2", function() - i =3D box.space.T.index[0] - return i.id + return box.space.T.index[0].id end, 0)