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 6F3CE2AD86 for ; Thu, 28 Mar 2019 11:12:00 -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 kAHxK2O0CRMQ for ; Thu, 28 Mar 2019 11:12:00 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 28AE72ADF1 for ; Thu, 28 Mar 2019 11:12:00 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 2/4] sql: refactor getNewIid() function References: <2047d49bec832a3c6c639b253fda249225b9c8ea.1553729426.git.korablev@tarantool.org> From: Vladislav Shpilevoy Message-ID: <32bf26dc-79ef-6bee-725a-2e1acf37d297@tarantool.org> Date: Thu, 28 Mar 2019 18:11:57 +0300 MIME-Version: 1.0 In-Reply-To: <2047d49bec832a3c6c639b253fda249225b9c8ea.1553729426.git.korablev@tarantool.org> 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: tarantool-patches@freelists.org, Nikita Pettik Hi! Thanks for the patch! See 4 comments below, review fixes at the end of the email, and on the branch. On 28/03/2019 15:07, Nikita Pettik wrote: > This commit includes no functional changes. Lets simply rewrite > getNewIid() function according to Tarantool codestyle. > > Part of #3914 > --- > src/box/sql/build.c | 72 ++++++++++++++++++++++++++--------------------------- > 1 file changed, 35 insertions(+), 37 deletions(-) > > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 8fb001433..f55f6d800 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -1918,45 +1918,43 @@ sql_drop_foreign_key(struct Parse *parse_context) > sqlVdbeChangeP5(sqlGetVdbe(parse_context), OPFLAG_NCHANGE); > } > > -/* > - * Generate code to determine next free Iid in the space identified by > - * the iSpaceId. Return register number holding the result. > +/** > + * Generate code to determine next free index id in > + * the space identified by the @space_id. 1. According to doxygen docs http://www.doxygen.nl/manual/commands.html there is no such a command as @space_id. But obviously I know what you've implied here. For references to parameters we use '@a' before a parameter. Like this: ... * the space identifier by @a space_id. ... > + * Return register holding the result. > + * > + * Overall VDBE program logic is following: > + * > + * 1 Seek for space id in _index, goto l1 if seeks fails. > + * 2 Goto l2. > + * 3 l1: Halt. > + * 4 l2: Fetch index id from _index record. > */ > static int > -getNewIid(Parse * pParse, int iSpaceId, int iCursor) > +generate_index_id(struct Parse *parse, uint32_t space_id, int cursor) 2. For vdbe methods generating code we use prefix 'vdbe_emit_'. And you use it in the last patch btw. 3. In fact, that function generates an index_id for only secondary indexes. Taking into account these two comments I renamed it to vdbe_emit_new_sec_index_id(). > { > - Vdbe *v = sqlGetVdbe(pParse); > - int iRes = ++pParse->nMem; > - int iKey = ++pParse->nMem; > - int iSeekInst, iGotoInst; > - > - sqlVdbeAddOp2(v, OP_Integer, iSpaceId, iKey); > - iSeekInst = sqlVdbeAddOp4Int(v, OP_SeekLE, iCursor, 0, iKey, 1); > - sqlVdbeAddOp4Int(v, OP_IdxLT, iCursor, 0, iKey, 1); > - > - /* > - * If SeekLE succeeds, the control falls through here, skipping > - * IdxLt. > - * > - * If it fails (no entry with the given key prefix: invalid spaceId) > - * VDBE jumps to the next code block (jump target is IMM, fixed up > - * later with sqlVdbeJumpHere()). > - */ > - iGotoInst = sqlVdbeAddOp0(v, OP_Goto); /* Jump over Halt */ > - > - /* Invalid spaceId detected. Halt now. */ > - sqlVdbeJumpHere(v, iSeekInst); > - sqlVdbeJumpHere(v, iSeekInst + 1); > - sqlVdbeAddOp4(v, > - OP_Halt, SQL_ERROR, ON_CONFLICT_ACTION_FAIL, 0, > - sqlMPrintf(pParse->db, "Invalid space id: %d", > - iSpaceId), P4_DYNAMIC); > - > - /* Fetch iid from the row and ++it. */ > - sqlVdbeJumpHere(v, iGotoInst); > - sqlVdbeAddOp3(v, OP_Column, iCursor, 1, iRes); > - sqlVdbeAddOp2(v, OP_AddImm, iRes, 1); > - return iRes; > + 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, cursor, 0, > + key_reg, 1); 4. Indentation. > + sqlVdbeAddOp4Int(v, OP_IdxLT, cursor, 0, key_reg, 1); > + /* Jump over Halt block. */ > + int goto_succ_addr = sqlVdbeAddOp0(v, OP_Goto); > + /* Invalid space id handling block starts here. */ > + sqlVdbeJumpHere(v, seek_adr); > + sqlVdbeJumpHere(v, seek_adr + 1); > + sqlVdbeAddOp4(v, OP_Halt, SQL_ERROR, ON_CONFLICT_ACTION_FAIL, 0, > + sqlMPrintf(parse->db, "Invalid space id: %d", space_id), > + P4_DYNAMIC); > + Note that I deliberately used @return instead of @retval, because @return has no arguments - it is just a description. ================================================================= commit 9e77985d09e53332d87b323c6ef74b747c418d8e Author: Vladislav Shpilevoy Date: Thu Mar 28 15:53:24 2019 +0300 Review fix diff --git a/src/box/sql/build.c b/src/box/sql/build.c index f55f6d800..b86fcb68d 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -1919,27 +1919,28 @@ sql_drop_foreign_key(struct Parse *parse_context) } /** - * Generate code to determine next free index id in - * the space identified by the @space_id. - * Return register holding the result. - * - * Overall VDBE program logic is following: + * Generate code to determine next free secondary index id in the + * space identified by @a space_id. Overall VDBE program logic is + * following: * * 1 Seek for space id in _index, goto l1 if seeks fails. * 2 Goto l2. * 3 l1: Halt. * 4 l2: Fetch index id from _index record. + * + * @return Register holding a new index id. */ static int -generate_index_id(struct Parse *parse, uint32_t space_id, int cursor) +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, cursor, 0, - key_reg, 1); - sqlVdbeAddOp4Int(v, OP_IdxLT, cursor, 0, key_reg, 1); + 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); /* Invalid space id handling block starts here. */ @@ -1952,7 +1953,7 @@ generate_index_id(struct Parse *parse, uint32_t space_id, int cursor) sqlVdbeJumpHere(v, goto_succ_addr); /* Fetch iid from the row and increment it. */ int iid_reg = ++parse->nMem; - sqlVdbeAddOp3(v, OP_Column, cursor, BOX_INDEX_FIELD_ID, iid_reg); + sqlVdbeAddOp3(v, OP_Column, _index_cursor, BOX_INDEX_FIELD_ID, iid_reg); sqlVdbeAddOp2(v, OP_AddImm, iid_reg, 1); return iid_reg; } @@ -2414,7 +2415,8 @@ sql_create_index(struct Parse *parse) { (void *)space_by_id(BOX_INDEX_ID), P4_SPACEPTR); sqlVdbeChangeP5(vdbe, OPFLAG_SEEKEQ); - int index_id = generate_index_id(parse, def->id, cursor); + int index_id = vdbe_emit_new_sec_index_id(parse, def->id, + cursor); sqlVdbeAddOp1(vdbe, OP_Close, cursor); vdbe_emit_create_index(parse, def, index->def, def->id, index_id); sqlVdbeChangeP5(vdbe, OPFLAG_NCHANGE);