[tarantool-patches] Re: [PATCH v2 2/4] sql: refactor getNewIid() function
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Thu Mar 28 18:11:57 MSK 2019
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 <v.shpilevoy at tarantool.org>
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);
More information about the Tarantool-patches
mailing list