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 9A16F243DF for ; Mon, 14 Jan 2019 09:06:38 -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 MwVsviK9hKKD for ; Mon, 14 Jan 2019 09:06:38 -0500 (EST) 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 4D35E219AF for ; Mon, 14 Jan 2019 09:06:38 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH 6/6] sql: introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PRIMARY KEY References: <5b411f9988366aee0418ed4aeb01243a85cfbd03.1547035184.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <3804c32a-4e68-d876-b811-119ff669852b@tarantool.org> Date: Mon, 14 Jan 2019 17:06:29 +0300 MIME-Version: 1.0 In-Reply-To: <5b411f9988366aee0418ed4aeb01243a85cfbd03.1547035184.git.korablev@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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, Nikita Pettik Thanks for the patch! See 3 comments below. On 09/01/2019 15:13, 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 == 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 > Follow-up #3914 > --- > src/box/sql/build.c | 29 +++++++++++++++++++++-- > src/box/sql/parse.y | 15 ++++++++++++ > test/sql-tap/alter.test.lua | 58 ++++++++++++++++++++++++++++++++++++++++++++- > 3 files changed, 99 insertions(+), 3 deletions(-) > > 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) 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. > +{ > + struct Vdbe *v = sqlite3GetVdbe(parse); > + int tmp_reg = ++parse->nMem; 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. > + sqlite3VdbeAddOp2(v, OP_Integer, space_id, tmp_reg); > + int found_addr = 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 = {} > + format[1] = { name = 'id', type = 'scalar'} > + format[2] = { name = 'f2', type = 'scalar'} > + s = box.schema.create_space('T', {format = 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 = box.space.T.index[0] > + return i.id 3. Why not return box.space.T.index[0].id? > + end, 0) > + > +test:do_catchsql_test( > + "alter-8.2", > + [[ > + ALTER TABLE t ADD CONSTRAINT pk1 PRIMARY KEY("f2"); > + ]], { > + 1, "multiple primary keys are not allowed" > + }) > + > +test:do_catchsql_test( > + "alter-8.3.1", > + [[ > + ALTER TABLE t ADD CONSTRAINT i1 UNIQUE("f2"); > + ]], { > + 0 > + }) > + > +test:do_test( > + "alter-8.3.2", > + function() > + i = box.space.T.index[1] > + return i.id > + end, 1) > + > +test:do_catchsql_test( > + "alter-8.4", > + [[ > + DROP INDEX i1 ON t; > + DROP INDEX pk ON t; > + ]], { > + 0 > +}) > > -- Commented due to #2953 > -- >