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 5E96E2ADC0 for ; Thu, 28 Mar 2019 11:11: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 DLGI-PAFOFKT for ; Thu, 28 Mar 2019 11:11: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 089E42AD86 for ; Thu, 28 Mar 2019 11:11:56 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 4/4] sql: introduce ALTER TABLE ADD CONSTRAINT UNIQUE/PRIMARY KEY References: From: Vladislav Shpilevoy Message-ID: <07f3d310-a7d0-e843-38e3-139074584ced@tarantool.org> Date: Thu, 28 Mar 2019 18:11:54 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 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: Nikita Pettik , tarantool-patches@freelists.org 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 == 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'? > Follow-up #3914 > > @TarantoolBot document > Title: ALTER TABLE ADD CONSTRAINT UNIQUE/PK > > Currently, tables which lack primary key can take part neither in DDL > nor DQL routines. Attempt to do this leads to error. Such tables > (without PK) may appear as spaces created from Lua NoSQL interface. > To improve NoSQL-SQL interoperability, we are introducing way to add > primary key to already existing table: > > ALTER TABLE ADD CONSTRAINT PRIMARY KEY() > > And alongside with this another one alias to CREATE UNIQUE INDEX: > > ALTER TABLE
ADD CONSTRAINT UNIQUE() > > Note that Tarantool PK constraint should be explicitly added before > any other unique constraints or secondary indexes. Otherwise, error is > raised: "can not add a secondary key before primary". > --- > src/box/sql/build.c | 32 +++++++++++++++++++++++-- > src/box/sql/parse.y | 10 ++++++++ > test/sql-tap/alter.test.lua | 57 +++++++++++++++++++++++++++++++++++++++++++- > test/sql-tap/index1.test.lua | 11 ++++++++- > 4 files changed, 106 insertions(+), 4 deletions(-) > > 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 = sqlGetVdbe(parse); > + int tmp_reg = ++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 = 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 ::= alter_add_constraint(N) FOREIGN KEY LP eidlist(FA) RP REFERENCES > sql_create_foreign_key(pParse); > } > > +cmd ::= 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} 5. We have a enum for these values. > +unique_spec(U) ::= UNIQUE. { U = SQL_INDEX_TYPE_CONSTRAINT_UNIQUE; } > +unique_spec(U) ::= PRIMARY KEY. { U = SQL_INDEX_TYPE_CONSTRAINT_PK; } > + ================================================================ commit cbdb0e2272df7893842a0279ab09d25cdfbddc1e Author: Vladislav Shpilevoy Date: Thu Mar 28 17:38:34 2019 +0300 Review fixes diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 94a559bec..18e438c09 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -1918,6 +1918,38 @@ sql_drop_foreign_key(struct Parse *parse_context) sqlVdbeChangeP5(sqlGetVdbe(parse_context), OPFLAG_NCHANGE); } +/** + * Position @a _index_cursor onto a last record in _index space + * with a specified @a space_id. It corresponds to the latest + * created index with the biggest id. + * @param parser SQL parser. + * @param space_id Space identifier to use as a key for _index. + * @param _index_cursor A cursor, opened on _index system space. + * @param[out] not_found_addr A VDBE address from which a jump + * happens when a record was not found. + * + * @return A VDBE address from which a jump happens when a record + * was found. + */ +static int +vdbe_emit_space_index_search(struct Parse *parser, uint32_t space_id, + int _index_cursor, int *not_found_addr) +{ + struct Vdbe *v = sqlGetVdbe(parser); + int key_reg = ++parser->nMem; + + sqlVdbeAddOp2(v, OP_Integer, space_id, key_reg); + int not_found1 = sqlVdbeAddOp4Int(v, OP_SeekLE, _index_cursor, 0, + key_reg, 1); + int not_found2 = sqlVdbeAddOp4Int(v, OP_IdxLT, _index_cursor, 0, + key_reg, 1); + int found_addr = sqlVdbeAddOp0(v, OP_Goto); + sqlVdbeJumpHere(v, not_found1); + sqlVdbeJumpHere(v, not_found2); + *not_found_addr = sqlVdbeAddOp0(v, OP_Goto); + return found_addr; +} + /** * Generate code to determine next free secondary index id in the * space identified by @a space_id. Overall VDBE program logic is @@ -1935,26 +1967,20 @@ vdbe_emit_new_sec_index_id(struct Parse *parse, uint32_t space_id, int _index_cursor) { struct Vdbe *v = sqlGetVdbe(parse); - int key_reg = ++parse->nMem; - - sqlVdbeAddOp2(v, OP_Integer, space_id, key_reg); - int seek_adr = sqlVdbeAddOp4Int(v, OP_SeekLE, _index_cursor, 0, - key_reg, 1); - sqlVdbeAddOp4Int(v, OP_IdxLT, _index_cursor, 0, key_reg, 1); - /* Jump over Halt block. */ - int goto_succ_addr = sqlVdbeAddOp0(v, OP_Goto); + int not_found_addr, found_addr = + vdbe_emit_space_index_search(parse, space_id, _index_cursor, + ¬_found_addr); /* * Absence of any records in _index for that space is * handled here. */ - sqlVdbeJumpHere(v, seek_adr); - sqlVdbeJumpHere(v, seek_adr + 1); + sqlVdbeJumpHere(v, not_found_addr); sqlVdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR, ON_CONFLICT_ACTION_FAIL, 0, "can not add a secondary key before primary", P4_STATIC); sqlVdbeChangeP5(v, ER_ALTER_SPACE); - sqlVdbeJumpHere(v, goto_succ_addr); + sqlVdbeJumpHere(v, found_addr); /* Fetch iid from the row and increment it. */ int iid_reg = ++parse->nMem; sqlVdbeAddOp3(v, OP_Column, _index_cursor, BOX_INDEX_FIELD_ID, iid_reg); @@ -1962,18 +1988,28 @@ vdbe_emit_new_sec_index_id(struct Parse *parse, uint32_t space_id, return iid_reg; } -static void -vdbe_emit_pk_existence_check(struct Parse *parse, uint32_t space_id, - int _index_cursor) +/** + * Generate code that pushes a primary index id 0, but checks if a + * space with @a space_id has any indexes. If it does, then the + * execution is stopped with an error. + * + * @return Register holding a new index id. + */ +static int +vdbe_emit_new_pk_index_id(struct Parse *parse, uint32_t space_id, + int _index_cursor) { struct Vdbe *v = sqlGetVdbe(parse); - int tmp_reg = ++parse->nMem; - sqlVdbeAddOp2(v, OP_Integer, space_id, tmp_reg); - int found_addr = sqlVdbeAddOp4Int(v, OP_NotFound, _index_cursor, 0, - tmp_reg, 1); + int not_found_addr, found_addr = + vdbe_emit_space_index_search(parse, space_id, _index_cursor, + ¬_found_addr); + sqlVdbeJumpHere(v, found_addr); sqlVdbeAddOp4(v, OP_Halt, SQL_ERROR, ON_CONFLICT_ACTION_FAIL, 0, "multiple primary keys are not allowed", P4_STATIC); - sqlVdbeJumpHere(v, found_addr); + sqlVdbeJumpHere(v, not_found_addr); + int index_id = parse->nMem++; + sqlVdbeAddOp2(v, OP_Integer, 0, index_id); + return index_id; } /** @@ -2441,9 +2477,8 @@ sql_create_index(struct Parse *parse) { * we can immediately halt execution of VDBE. */ if (idx_type == SQL_INDEX_TYPE_CONSTRAINT_PK) { - vdbe_emit_pk_existence_check(parse, def->id, cursor); - index_id = parse->nMem++; - sqlVdbeAddOp2(vdbe, OP_Integer, 0, index_id); + index_id = vdbe_emit_new_pk_index_id(parse, def->id, + cursor); } else { index_id = vdbe_emit_new_sec_index_id(parse, def->id, cursor); diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index ed5c05436..53718a158 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -1630,7 +1630,7 @@ cmd ::= alter_add_constraint(N) unique_spec(U) LP sortlist(X) RP. { sql_create_index(pParse); } -%type unique_spec {int} +%type unique_spec {enum sql_index_type} unique_spec(U) ::= UNIQUE. { U = SQL_INDEX_TYPE_CONSTRAINT_UNIQUE; } unique_spec(U) ::= PRIMARY KEY. { U = SQL_INDEX_TYPE_CONSTRAINT_PK; }