* [tarantool-patches] [PATCH v1 1/1] sql: refactor vdbe_emit_open_cursor calls
@ 2018-07-09 12:42 Kirill Shcherbatov
2018-07-10 11:48 ` [tarantool-patches] " Vladislav Shpilevoy
2018-07-10 13:45 ` [tarantool-patches] [PATCH v1 2/2] sql: remove OP_LoadPtr Kirill Shcherbatov
0 siblings, 2 replies; 10+ messages in thread
From: Kirill Shcherbatov @ 2018-07-09 12:42 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Kirill Shcherbatov
Made vdbe_emit_open_cursor calls consistent:
now it uses index id everywhere.
This required to change a way to detect that
VDBE has openned Read cursor to specified table
in vdbe_has_table_read to write result of insert
in temp table if required.
---
https://github.com/tarantool/tarantool/compare/kshch/kshch/vdbe_emit_open_cursor-refactor
over branch
https://github.com/tarantool/tarantool/issues/2847
src/box/sql/analyze.c | 3 +-
src/box/sql/build.c | 4 ++-
src/box/sql/expr.c | 5 ++-
src/box/sql/fkey.c | 3 +-
src/box/sql/insert.c | 95 ++++++++++++++++++++++++++++++---------------------
src/box/sql/select.c | 4 +--
src/box/sql/where.c | 10 ++++--
7 files changed, 75 insertions(+), 49 deletions(-)
diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index 336d146..ff3e735 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -159,8 +159,7 @@ vdbe_emit_stat_space_open(struct Parse *parse, int stat_cursor,
/* Open the sql_stat tables for writing. */
for (uint i = 0; i < lengthof(space_names); ++i) {
uint32_t id = space_ids[i];
- int tnum = SQLITE_PAGENO_FROM_SPACEID_AND_INDEXID(id, 0);
- vdbe_emit_open_cursor(parse, stat_cursor + i, tnum,
+ vdbe_emit_open_cursor(parse, stat_cursor + i, 0,
space_by_id(id));
VdbeComment((v, space_names[i]));
}
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 1c00842..82447dc 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1108,6 +1108,7 @@ vdbe_emit_open_cursor(struct Parse *parse_context, int cursor, int index_id,
struct space *space)
{
assert(space != NULL);
+ assert(index_id == SQLITE_PAGENO_TO_INDEXID(index_id));
struct Vdbe *vdbe = parse_context->pVdbe;
int space_ptr_reg = ++parse_context->nMem;
sqlite3VdbeAddOp4(vdbe, OP_LoadPtr, 0, space_ptr_reg, 0, (void*)space,
@@ -2448,7 +2449,8 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage)
sqlite3VdbeAddOp2(v, OP_Clear, SQLITE_PAGENO_TO_SPACEID(tnum),
0);
struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(tnum));
- vdbe_emit_open_cursor(pParse, iIdx, tnum, space);
+ vdbe_emit_open_cursor(pParse, iIdx, SQLITE_PAGENO_TO_INDEXID(tnum),
+ space);
sqlite3VdbeChangeP5(v, memRootPage >= 0 ? OPFLAG_P2ISREG : 0);
addr1 = sqlite3VdbeAddOp2(v, OP_SorterSort, iSorter, 0);
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 3183e3d..b1650cf 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -2470,8 +2470,11 @@ sqlite3FindInIndex(Parse * pParse, /* Parsing context */
P4_DYNAMIC);
struct space *space =
space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum));
+ uint32_t idx_id =
+ SQLITE_PAGENO_TO_INDEXID(pIdx->
+ tnum);
vdbe_emit_open_cursor(pParse, iTab,
- pIdx->tnum, space);
+ idx_id, space);
VdbeComment((v, "%s", pIdx->zName));
assert(IN_INDEX_INDEX_DESC ==
IN_INDEX_INDEX_ASC + 1);
diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
index 6c75c47..face9cb 100644
--- a/src/box/sql/fkey.c
+++ b/src/box/sql/fkey.c
@@ -441,7 +441,8 @@ fkLookupParent(Parse * pParse, /* Parse context */
int regRec = sqlite3GetTempReg(pParse);
struct space *space =
space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum));
- vdbe_emit_open_cursor(pParse, iCur, pIdx->tnum, space);
+ uint32_t idx_id = SQLITE_PAGENO_TO_INDEXID(pIdx->tnum);
+ vdbe_emit_open_cursor(pParse, iCur, idx_id, space);
for (i = 0; i < nCol; i++) {
sqlite3VdbeAddOp2(v, OP_Copy,
aiCol[i] + 1 + regData,
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 58e159c..77567e7 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -179,41 +179,50 @@ sqlite3TableAffinity(Vdbe * v, Table * pTab, int iReg)
}
}
-/*
- * Return non-zero if the table pTab in database or any of its indices
- * have been opened at any point in the VDBE program. This is used to see if
- * a statement of the form "INSERT INTO <pTab> SELECT ..." can
- * run for the results of the SELECT.
+/**
+ * Return true if the table pTab in database or any of its
+ * indices have been opened at any point in the VDBE program.
+ * This is used to see if a statement of the form
+ * "INSERT INTO <pTab> SELECT ..." can run for the results of the
+ * SELECT.
+ *
+ * @param parser Parse context.
+ * @param table Table AST object.
+ * @retval true or false
*/
-static int
-readsTable(Parse * p, Table * pTab)
+static bool
+vdbe_has_table_read(struct Parse *parser, struct Table *table)
{
- Vdbe *v = sqlite3GetVdbe(p);
- int i;
- int iEnd = sqlite3VdbeCurrentAddr(v);
-
- for (i = 1; i < iEnd; i++) {
- VdbeOp *pOp = sqlite3VdbeGetOp(v, i);
- assert(pOp != 0);
- /* Currently, there is no difference between
- * Read and Write cursors.
+ struct Vdbe *v = sqlite3GetVdbe(parser);
+ int last_instr = sqlite3VdbeCurrentAddr(v);
+ for (int i = 1; i < last_instr; i++) {
+ struct VdbeOp *pOp = sqlite3VdbeGetOp(v, i);
+ assert(pOp != NULL);
+ /*
+ * Currently, there is no difference between Read
+ * and Write cursors.
*/
- if (pOp->opcode == OP_OpenRead ||
- pOp->opcode == OP_OpenWrite) {
- Index *pIndex;
- int tnum = pOp->p2;
- if (tnum == pTab->tnum) {
- return 1;
- }
- for (pIndex = pTab->pIndex; pIndex;
- pIndex = pIndex->pNext) {
- if (tnum == pIndex->tnum) {
- return 1;
- }
+ if (pOp->opcode == OP_OpenRead || pOp->opcode == OP_OpenWrite) {
+ assert(i > 1);
+ struct VdbeOp *space_var_op =
+ sqlite3VdbeGetOp(v, i - 1);
+ assert(space_var_op != NULL);
+ assert(space_var_op->opcode == OP_LoadPtr);
+ struct space *space = space_var_op->p4.space;
+
+ if (space == space_by_id(table->def->id))
+ return true;
+
+ int idx_id = pOp->p2;
+ for (struct Index *pIndex = table->pIndex;
+ pIndex != NULL; pIndex = pIndex->pNext) {
+ if (idx_id ==
+ SQLITE_PAGENO_TO_INDEXID(pIndex->tnum))
+ return true;
}
}
}
- return 0;
+ return false;
}
@@ -526,16 +535,20 @@ sqlite3Insert(Parse * pParse, /* Parser context */
assert(pSelect->pEList);
nColumn = pSelect->pEList->nExpr;
- /* Set useTempTable to TRUE if the result of the SELECT statement
- * should be written into a temporary table (template 4). Set to
- * FALSE if each output row of the SELECT can be written directly into
- * the destination table (template 3).
+ /*
+ * Set useTempTable to TRUE if the result of the
+ * SELECT statement should be written into a
+ * temporary table (template 4). Set to FALSE if
+ * each output row of the SELECT can be written
+ * directly into the destination table
+ * (template 3).
*
- * A temp table must be used if the table being updated is also one
- * of the tables being read by the SELECT statement. Also use a
- * temp table in the case of row triggers.
+ * A temp table must be used if the table being
+ * updated is also one of the tables being read by
+ * the SELECT statement. Also use a temp table in
+ * the case of row triggers.
*/
- if (trigger != NULL || readsTable(pParse, pTab))
+ if (trigger != NULL || vdbe_has_table_read(pParse, pTab))
useTempTable = 1;
if (useTempTable) {
@@ -1934,11 +1947,15 @@ xferOptimization(Parse * pParse, /* Parser context */
assert(pSrcIdx);
struct space *src_space =
space_by_id(SQLITE_PAGENO_TO_SPACEID(pSrcIdx->tnum));
- vdbe_emit_open_cursor(pParse, iSrc, pSrcIdx->tnum, src_space);
+ vdbe_emit_open_cursor(pParse, iSrc,
+ SQLITE_PAGENO_TO_INDEXID(pSrcIdx->tnum),
+ src_space);
VdbeComment((v, "%s", pSrcIdx->zName));
struct space *dest_space =
space_by_id(SQLITE_PAGENO_TO_SPACEID(pDestIdx->tnum));
- vdbe_emit_open_cursor(pParse, iDest, pDestIdx->tnum, dest_space);
+ vdbe_emit_open_cursor(pParse, iDest,
+ SQLITE_PAGENO_TO_INDEXID(pDestIdx->tnum),
+ dest_space);
VdbeComment((v, "%s", pDestIdx->zName));
addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iSrc, 0);
VdbeCoverage(v);
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 52b3fdd..ceb7e34 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -6279,9 +6279,7 @@ sqlite3Select(Parse * pParse, /* The parser context */
* Open the cursor, execute the OP_Count,
* close the cursor.
*/
- vdbe_emit_open_cursor(pParse, cursor,
- space->def->id << 10,
- space);
+ vdbe_emit_open_cursor(pParse, cursor, 0, space);
sqlite3VdbeAddOp2(v, OP_Count, cursor,
sAggInfo.aFunc[0].iMem);
sqlite3VdbeAddOp1(v, OP_Close, cursor);
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index 85143ed..6c07fec 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -4759,10 +4759,16 @@ sqlite3WhereBegin(Parse * pParse, /* The parser context */
assert(iIndexCur >= 0);
if (op) {
if (pIx != NULL) {
+ uint32_t space_id =
+ SQLITE_PAGENO_TO_SPACEID(pIx->
+ tnum);
struct space *space =
- space_by_id(SQLITE_PAGENO_TO_SPACEID(pIx->tnum));
+ space_by_id(space_id);
+ uint32_t idx_id =
+ SQLITE_PAGENO_TO_INDEXID(pIx->
+ tnum);
vdbe_emit_open_cursor(pParse, iIndexCur,
- pIx->tnum, space);
+ idx_id, space);
} else {
vdbe_emit_open_cursor(pParse, iIndexCur,
idx_def->iid,
--
2.7.4
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] sql: refactor vdbe_emit_open_cursor calls
2018-07-09 12:42 [tarantool-patches] [PATCH v1 1/1] sql: refactor vdbe_emit_open_cursor calls Kirill Shcherbatov
@ 2018-07-10 11:48 ` Vladislav Shpilevoy
2018-07-10 13:44 ` Kirill Shcherbatov
2018-07-10 13:45 ` [tarantool-patches] [PATCH v1 2/2] sql: remove OP_LoadPtr Kirill Shcherbatov
1 sibling, 1 reply; 10+ messages in thread
From: Vladislav Shpilevoy @ 2018-07-10 11:48 UTC (permalink / raw)
To: Kirill Shcherbatov, tarantool-patches
Hello. Thanks for the patch!
On 09/07/2018 15:42, Kirill Shcherbatov wrote:
> Made vdbe_emit_open_cursor calls consistent:
> now it uses index id everywhere.
> This required to change a way to detect that
> VDBE has openned Read cursor to specified table
> in vdbe_has_table_read to write result of insert
> in temp table if required.
> ---
Where are the issue and the branch?
> https://github.com/tarantool/tarantool/compare/kshch/kshch/vdbe_emit_open_cursor-refactor
This is not branch and by this link I see message:
"There isn’t anything to compare."
> over branch
> https://github.com/tarantool/tarantool/issues/2847
How is this linked? The issue was found under working with this
issue, but it has been actual before and remains actual after.
See 7 comments below.
And I have pushed my own patch for LoadPtr removal on the top
of your branch. Please, fix my comments below, and fix the patch
I have pushed (most likely it does not pass tests now).
>
> src/box/sql/analyze.c | 3 +-
> src/box/sql/build.c | 4 ++-
> src/box/sql/expr.c | 5 ++-
> src/box/sql/fkey.c | 3 +-
> src/box/sql/insert.c | 95 ++++++++++++++++++++++++++++++---------------------
> src/box/sql/select.c | 4 +--
> src/box/sql/where.c | 10 ++++--
> 7 files changed, 75 insertions(+), 49 deletions(-)
>
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 1c00842..82447dc 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1108,6 +1108,7 @@ vdbe_emit_open_cursor(struct Parse *parse_context, int cursor, int index_id,
> struct space *space)
> {
> assert(space != NULL);
> + assert(index_id == SQLITE_PAGENO_TO_INDEXID(index_id));
1. What the hell? Why?
> struct Vdbe *vdbe = parse_context->pVdbe;
> int space_ptr_reg = ++parse_context->nMem;
> sqlite3VdbeAddOp4(vdbe, OP_LoadPtr, 0, space_ptr_reg, 0, (void*)space,
> addr1 = sqlite3VdbeAddOp2(v, OP_SorterSort, iSorter, 0);
2. vdbe_emit_open_cursor has outdated comment in the header.
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 58e159c..77567e7 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -179,41 +179,50 @@ sqlite3TableAffinity(Vdbe * v, Table * pTab, int iReg)
> }
> }
>
> -/*
> - * Return non-zero if the table pTab in database or any of its indices
> - * have been opened at any point in the VDBE program. This is used to see if
> - * a statement of the form "INSERT INTO <pTab> SELECT ..." can
> - * run for the results of the SELECT.
> +/**
> + * Return true if the table pTab in database or any of its
3. No pTab.
> + * indices have been opened at any point in the VDBE program.
> + * This is used to see if a statement of the form
> + * "INSERT INTO <pTab> SELECT ..." can run for the results of the
> + * SELECT.
> + *
> + * @param parser Parse context.
> + * @param table Table AST object.
> + * @retval true or false
4. Unhelpful comment. Obviously bool is either true or false.
> */
> -static int
> -readsTable(Parse * p, Table * pTab)
> +static bool
> +vdbe_has_table_read(struct Parse *parser, struct Table *table)
5. You can make table be const struct Table *.
> {
> - Vdbe *v = sqlite3GetVdbe(p);
> - int i;
> - int iEnd = sqlite3VdbeCurrentAddr(v);
> -
> - for (i = 1; i < iEnd; i++) {
> - VdbeOp *pOp = sqlite3VdbeGetOp(v, i);
> - assert(pOp != 0);
> - /* Currently, there is no difference between
> - * Read and Write cursors.
> + struct Vdbe *v = sqlite3GetVdbe(parser);
> + int last_instr = sqlite3VdbeCurrentAddr(v);
> + for (int i = 1; i < last_instr; i++) {
> + struct VdbeOp *pOp = sqlite3VdbeGetOp(v, i);
6. When a function is refactored completely, we should use
Tarantool code style for variable names.
> + assert(pOp != NULL);
> + /*
> + * Currently, there is no difference between Read
> + * and Write cursors.
> */
> - if (pOp->opcode == OP_OpenRead ||
> - pOp->opcode == OP_OpenWrite) {
> - Index *pIndex;
> - int tnum = pOp->p2;
> - if (tnum == pTab->tnum) {
> - return 1;
> - }
> - for (pIndex = pTab->pIndex; pIndex;
> - pIndex = pIndex->pNext) {
> - if (tnum == pIndex->tnum) {
> - return 1;
> - }
> + if (pOp->opcode == OP_OpenRead || pOp->opcode == OP_OpenWrite) {
> + assert(i > 1);
> + struct VdbeOp *space_var_op =
> + sqlite3VdbeGetOp(v, i - 1);
> + assert(space_var_op != NULL);
> + assert(space_var_op->opcode == OP_LoadPtr);
> + struct space *space = space_var_op->p4.space;
> +
> + if (space == space_by_id(table->def->id))
7. Why do you need space_by_id()? Why not just space->id == table->def->id?
> + return true;
> +
> + int idx_id = pOp->p2;
> + for (struct Index *pIndex = table->pIndex;
> + pIndex != NULL; pIndex = pIndex->pNext) {
> + if (idx_id ==
> + SQLITE_PAGENO_TO_INDEXID(pIndex->tnum))
> + return true;
> }
> }
> }
> - return 0;
> + return false;
> }
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] sql: refactor vdbe_emit_open_cursor calls
2018-07-10 11:48 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-07-10 13:44 ` Kirill Shcherbatov
2018-07-10 14:59 ` Vladislav Shpilevoy
0 siblings, 1 reply; 10+ messages in thread
From: Kirill Shcherbatov @ 2018-07-10 13:44 UTC (permalink / raw)
To: tarantool-patches, Vladislav Shpilevoy
> Where are the issue and the branch?
There was no issue for this task.
>> over branch
>> https://github.com/tarantool/tarantool/issues/2847
This should be merged later than #2847.
> And I have pushed my own patch for LoadPtr removal on the top
> of your branch. Please, fix my comments below, and fix the patch
> I have pushed (most likely it does not pass tests now).
Ok, thank you.
I'll send working version to this thread as separate message.
> 1. What the hell? Why?
I'd like to prevent invalid(legacy) usage of this function. Maybe it is really redundant.
- assert(index_id == SQLITE_PAGENO_TO_INDEXID(index_id));
> 2. vdbe_emit_open_cursor has outdated comment in the header.
* @param parse_context Parse context.
* @param cursor Number of cursor to be created.
- * @param index_id Encoded index id (encoding is void actually, so
- * pas it as is). In future will be replaced with pointer
- * to struct index.
+ * @param index_id index id. In future will be replaced with
+ * pointer to struct index.
* @retval address of last opcode.
> 3. No pTab.
> 4. Unhelpful comment. Obviously bool is either true or false.
> 5. You can make table be const struct Table *.
/**
* This routine is used to see if a statement of the form
* "INSERT INTO <table> SELECT ..." can run for the results of the
* SELECT.
*
* @param parser Parse context.
* @param table Table AST object.
* @retval true if the table table in database or any of its
* indices have been opened at any point in the VDBE
* program.
* @retval false else.
*/
static bool
vdbe_has_table_read(struct Parse *parser, const struct Table *table)
> 6. When a function is refactored completely, we should use
> Tarantool code style for variable names.
- struct VdbeOp *pOp = sqlite3VdbeGetOp(v, i);
- assert(pOp != NULL);
+ struct VdbeOp *op = sqlite3VdbeGetOp(v, i);
+ assert(op != NULL);
> 7. Why do you need space_by_id()? Why not just space->id == table->def->id?
- if (space == space_by_id(table->def->id))
+ if (space->def->id == table->def->id)
=========================================
Made vdbe_emit_open_cursor calls consistent:
now it uses index id everywhere.
This required to change a way to detect that
VDBE has openned Read cursor to specified table
in vdbe_has_table_read to write result of insert
in temp table if required.
---
src/box/sql/analyze.c | 3 +-
src/box/sql/build.c | 3 +-
src/box/sql/expr.c | 5 ++-
src/box/sql/fkey.c | 3 +-
src/box/sql/insert.c | 96 +++++++++++++++++++++++++++++--------------------
src/box/sql/select.c | 4 +--
src/box/sql/sqliteInt.h | 5 ++-
src/box/sql/where.c | 10 ++++--
8 files changed, 77 insertions(+), 52 deletions(-)
diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index 336d146..ff3e735 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -159,8 +159,7 @@ vdbe_emit_stat_space_open(struct Parse *parse, int stat_cursor,
/* Open the sql_stat tables for writing. */
for (uint i = 0; i < lengthof(space_names); ++i) {
uint32_t id = space_ids[i];
- int tnum = SQLITE_PAGENO_FROM_SPACEID_AND_INDEXID(id, 0);
- vdbe_emit_open_cursor(parse, stat_cursor + i, tnum,
+ vdbe_emit_open_cursor(parse, stat_cursor + i, 0,
space_by_id(id));
VdbeComment((v, space_names[i]));
}
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 1c00842..5f7a35a 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2448,7 +2448,8 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage)
sqlite3VdbeAddOp2(v, OP_Clear, SQLITE_PAGENO_TO_SPACEID(tnum),
0);
struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(tnum));
- vdbe_emit_open_cursor(pParse, iIdx, tnum, space);
+ vdbe_emit_open_cursor(pParse, iIdx, SQLITE_PAGENO_TO_INDEXID(tnum),
+ space);
sqlite3VdbeChangeP5(v, memRootPage >= 0 ? OPFLAG_P2ISREG : 0);
addr1 = sqlite3VdbeAddOp2(v, OP_SorterSort, iSorter, 0);
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 3183e3d..b1650cf 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -2470,8 +2470,11 @@ sqlite3FindInIndex(Parse * pParse, /* Parsing context */
P4_DYNAMIC);
struct space *space =
space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum));
+ uint32_t idx_id =
+ SQLITE_PAGENO_TO_INDEXID(pIdx->
+ tnum);
vdbe_emit_open_cursor(pParse, iTab,
- pIdx->tnum, space);
+ idx_id, space);
VdbeComment((v, "%s", pIdx->zName));
assert(IN_INDEX_INDEX_DESC ==
IN_INDEX_INDEX_ASC + 1);
diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
index 6c75c47..face9cb 100644
--- a/src/box/sql/fkey.c
+++ b/src/box/sql/fkey.c
@@ -441,7 +441,8 @@ fkLookupParent(Parse * pParse, /* Parse context */
int regRec = sqlite3GetTempReg(pParse);
struct space *space =
space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum));
- vdbe_emit_open_cursor(pParse, iCur, pIdx->tnum, space);
+ uint32_t idx_id = SQLITE_PAGENO_TO_INDEXID(pIdx->tnum);
+ vdbe_emit_open_cursor(pParse, iCur, idx_id, space);
for (i = 0; i < nCol; i++) {
sqlite3VdbeAddOp2(v, OP_Copy,
aiCol[i] + 1 + regData,
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 58e159c..ca194e6 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -179,41 +179,51 @@ sqlite3TableAffinity(Vdbe * v, Table * pTab, int iReg)
}
}
-/*
- * Return non-zero if the table pTab in database or any of its indices
- * have been opened at any point in the VDBE program. This is used to see if
- * a statement of the form "INSERT INTO <pTab> SELECT ..." can
- * run for the results of the SELECT.
+/**
+ * This routine is used to see if a statement of the form
+ * "INSERT INTO <table> SELECT ..." can run for the results of the
+ * SELECT.
+ *
+ * @param parser Parse context.
+ * @param table Table AST object.
+ * @retval true if the table table in database or any of its
+ * indices have been opened at any point in the VDBE
+ * program.
+ * @retval false else.
*/
-static int
-readsTable(Parse * p, Table * pTab)
+static bool
+vdbe_has_table_read(struct Parse *parser, const struct Table *table)
{
- Vdbe *v = sqlite3GetVdbe(p);
- int i;
- int iEnd = sqlite3VdbeCurrentAddr(v);
-
- for (i = 1; i < iEnd; i++) {
- VdbeOp *pOp = sqlite3VdbeGetOp(v, i);
- assert(pOp != 0);
- /* Currently, there is no difference between
- * Read and Write cursors.
+ struct Vdbe *v = sqlite3GetVdbe(parser);
+ int last_instr = sqlite3VdbeCurrentAddr(v);
+ for (int i = 1; i < last_instr; i++) {
+ struct VdbeOp *op = sqlite3VdbeGetOp(v, i);
+ assert(op != NULL);
+ /*
+ * Currently, there is no difference between Read
+ * and Write cursors.
*/
- if (pOp->opcode == OP_OpenRead ||
- pOp->opcode == OP_OpenWrite) {
- Index *pIndex;
- int tnum = pOp->p2;
- if (tnum == pTab->tnum) {
- return 1;
- }
- for (pIndex = pTab->pIndex; pIndex;
- pIndex = pIndex->pNext) {
- if (tnum == pIndex->tnum) {
- return 1;
- }
+ if (op->opcode == OP_OpenRead || op->opcode == OP_OpenWrite) {
+ assert(i > 1);
+ struct VdbeOp *space_var_op =
+ sqlite3VdbeGetOp(v, i - 1);
+ assert(space_var_op != NULL);
+ assert(space_var_op->opcode == OP_LoadPtr);
+ struct space *space = space_var_op->p4.space;
+
+ if (space->def->id == table->def->id)
+ return true;
+
+ int idx_id = op->p2;
+ for (struct Index *pIndex = table->pIndex;
+ pIndex != NULL; pIndex = pIndex->pNext) {
+ if (idx_id ==
+ SQLITE_PAGENO_TO_INDEXID(pIndex->tnum))
+ return true;
}
}
}
- return 0;
+ return false;
}
@@ -526,16 +536,20 @@ sqlite3Insert(Parse * pParse, /* Parser context */
assert(pSelect->pEList);
nColumn = pSelect->pEList->nExpr;
- /* Set useTempTable to TRUE if the result of the SELECT statement
- * should be written into a temporary table (template 4). Set to
- * FALSE if each output row of the SELECT can be written directly into
- * the destination table (template 3).
+ /*
+ * Set useTempTable to TRUE if the result of the
+ * SELECT statement should be written into a
+ * temporary table (template 4). Set to FALSE if
+ * each output row of the SELECT can be written
+ * directly into the destination table
+ * (template 3).
*
- * A temp table must be used if the table being updated is also one
- * of the tables being read by the SELECT statement. Also use a
- * temp table in the case of row triggers.
+ * A temp table must be used if the table being
+ * updated is also one of the tables being read by
+ * the SELECT statement. Also use a temp table in
+ * the case of row triggers.
*/
- if (trigger != NULL || readsTable(pParse, pTab))
+ if (trigger != NULL || vdbe_has_table_read(pParse, pTab))
useTempTable = 1;
if (useTempTable) {
@@ -1934,11 +1948,15 @@ xferOptimization(Parse * pParse, /* Parser context */
assert(pSrcIdx);
struct space *src_space =
space_by_id(SQLITE_PAGENO_TO_SPACEID(pSrcIdx->tnum));
- vdbe_emit_open_cursor(pParse, iSrc, pSrcIdx->tnum, src_space);
+ vdbe_emit_open_cursor(pParse, iSrc,
+ SQLITE_PAGENO_TO_INDEXID(pSrcIdx->tnum),
+ src_space);
VdbeComment((v, "%s", pSrcIdx->zName));
struct space *dest_space =
space_by_id(SQLITE_PAGENO_TO_SPACEID(pDestIdx->tnum));
- vdbe_emit_open_cursor(pParse, iDest, pDestIdx->tnum, dest_space);
+ vdbe_emit_open_cursor(pParse, iDest,
+ SQLITE_PAGENO_TO_INDEXID(pDestIdx->tnum),
+ dest_space);
VdbeComment((v, "%s", pDestIdx->zName));
addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iSrc, 0);
VdbeCoverage(v);
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 52b3fdd..ceb7e34 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -6279,9 +6279,7 @@ sqlite3Select(Parse * pParse, /* The parser context */
* Open the cursor, execute the OP_Count,
* close the cursor.
*/
- vdbe_emit_open_cursor(pParse, cursor,
- space->def->id << 10,
- space);
+ vdbe_emit_open_cursor(pParse, cursor, 0, space);
sqlite3VdbeAddOp2(v, OP_Count, cursor,
sAggInfo.aFunc[0].iMem);
sqlite3VdbeAddOp1(v, OP_Close, cursor);
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 380c9c6..6b1afaa 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3583,9 +3583,8 @@ void sqlite3EndTable(Parse *, Token *, Token *, Select *);
*
* @param parse_context Parse context.
* @param cursor Number of cursor to be created.
- * @param index_id Encoded index id (encoding is void actually, so
- * pas it as is). In future will be replaced with pointer
- * to struct index.
+ * @param index_id index id. In future will be replaced with
+ * pointer to struct index.
* @retval address of last opcode.
*/
int
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index 85143ed..6c07fec 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -4759,10 +4759,16 @@ sqlite3WhereBegin(Parse * pParse, /* The parser context */
assert(iIndexCur >= 0);
if (op) {
if (pIx != NULL) {
+ uint32_t space_id =
+ SQLITE_PAGENO_TO_SPACEID(pIx->
+ tnum);
struct space *space =
- space_by_id(SQLITE_PAGENO_TO_SPACEID(pIx->tnum));
+ space_by_id(space_id);
+ uint32_t idx_id =
+ SQLITE_PAGENO_TO_INDEXID(pIx->
+ tnum);
vdbe_emit_open_cursor(pParse, iIndexCur,
- pIx->tnum, space);
+ idx_id, space);
} else {
vdbe_emit_open_cursor(pParse, iIndexCur,
idx_def->iid,
--
2.7.4
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] [PATCH v1 2/2] sql: remove OP_LoadPtr
2018-07-09 12:42 [tarantool-patches] [PATCH v1 1/1] sql: refactor vdbe_emit_open_cursor calls Kirill Shcherbatov
2018-07-10 11:48 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-07-10 13:45 ` Kirill Shcherbatov
2018-07-10 14:59 ` [tarantool-patches] " Vladislav Shpilevoy
1 sibling, 1 reply; 10+ messages in thread
From: Kirill Shcherbatov @ 2018-07-10 13:45 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
This opcode was used when Vdbe had to store key_def in P4 for
OP_OpenRead/Write, so P4 was occupied and OP_LoadPtr was used to
store space pointer in another opcode. But now P4 is free for
OpenRead/Write and space pointer can be stored here.
Alongside, some useless key_def duplications are removed.
---
src/box/sql/analyze.c | 8 ++------
src/box/sql/build.c | 20 ++++++--------------
src/box/sql/delete.c | 16 ++--------------
src/box/sql/insert.c | 18 ++++--------------
src/box/sql/pragma.c | 15 +++++++++++----
src/box/sql/vdbe.c | 41 ++++++-----------------------------------
6 files changed, 31 insertions(+), 87 deletions(-)
diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index ff3e735..d84f1c0 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -794,7 +794,6 @@ analyzeOneTable(Parse * pParse, /* Parser context */
int iTabCur; /* Table cursor */
Vdbe *v; /* The virtual machine being built up */
int i; /* Loop counter */
- int space_ptr_reg = iMem++;
int regStat4 = iMem++; /* Register to hold Stat4Accum object */
int regChng = iMem++; /* Index of changed index field */
int regKey = iMem++; /* Key argument passed to stat_push() */
@@ -893,11 +892,8 @@ analyzeOneTable(Parse * pParse, /* Parser context */
struct space *space =
space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum));
assert(space != NULL);
- sqlite3VdbeAddOp4(v, OP_LoadPtr, 0, space_ptr_reg, 0,
- (void*)space, P4_SPACEPTR);
- sqlite3VdbeAddOp3(v, OP_OpenRead, iIdxCur, pIdx->tnum,
- space_ptr_reg);
- sql_vdbe_set_p4_key_def(pParse, pIdx);
+ sqlite3VdbeAddOp4(v, OP_OpenRead, iIdxCur, pIdx->tnum,
+ 0, (void *) space, P4_SPACEPTR);
VdbeComment((v, "%s", pIdx->zName));
/* Invoke the stat_init() function. The arguments are:
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 5f7a35a..26af0fe 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1108,12 +1108,8 @@ vdbe_emit_open_cursor(struct Parse *parse_context, int cursor, int index_id,
struct space *space)
{
assert(space != NULL);
- struct Vdbe *vdbe = parse_context->pVdbe;
- int space_ptr_reg = ++parse_context->nMem;
- sqlite3VdbeAddOp4(vdbe, OP_LoadPtr, 0, space_ptr_reg, 0, (void*)space,
- P4_SPACEPTR);
- return sqlite3VdbeAddOp3(vdbe, OP_OpenWrite, cursor, index_id,
- space_ptr_reg);
+ return sqlite3VdbeAddOp4(parse_context->pVdbe, OP_OpenWrite, cursor,
+ index_id, 0, (void *) space, P4_SPACEPTR);
}
/*
* Generate code that will increment the schema cookie.
@@ -2961,7 +2957,6 @@ sql_create_index(struct Parse *parse, struct Token *token,
Vdbe *v;
char *zStmt;
int iCursor = parse->nTab++;
- int index_space_ptr_reg = parse->nTab++;
int iSpaceId, iIndexId, iFirstSchemaCol;
v = sqlite3GetVdbe(parse);
@@ -2969,12 +2964,9 @@ sql_create_index(struct Parse *parse, struct Token *token,
goto exit_create_index;
sql_set_multi_write(parse, true);
-
-
- sqlite3VdbeAddOp2(v, OP_SIDtoPtr, BOX_INDEX_ID,
- index_space_ptr_reg);
- sqlite3VdbeAddOp4Int(v, OP_OpenWrite, iCursor, 0,
- index_space_ptr_reg, 6);
+ sqlite3VdbeAddOp4(v, OP_OpenWrite, iCursor, 0, 0,
+ (void *)space_by_id(BOX_INDEX_ID),
+ P4_SPACEPTR);
sqlite3VdbeChangeP5(v, OPFLAG_SEEKEQ);
/*
@@ -3943,7 +3935,7 @@ vdbe_emit_halt_with_presence_test(struct Parse *parser, int space_id,
int cursor = parser->nTab++;
vdbe_emit_open_cursor(parser, cursor, index_id, space_by_id(space_id));
- int name_reg = parser->nMem++;
+ int name_reg = ++parser->nMem;
int label = sqlite3VdbeAddOp4(v, OP_String8, 0, name_reg, 0, name,
P4_DYNAMIC);
sqlite3VdbeAddOp4Int(v, cond_opcode, cursor, label + 3, name_reg, 1);
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 66dc0fc..24122e8 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -335,23 +335,11 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
iAddrOnce = sqlite3VdbeAddOp0(v, OP_Once);
VdbeCoverage(v);
}
-
- int space_ptr_reg = ++parse->nMem;
- sqlite3VdbeAddOp4(v, OP_LoadPtr, 0, space_ptr_reg, 0,
- (void *)space, P4_SPACEPTR);
-
int tnum =
SQLITE_PAGENO_FROM_SPACEID_AND_INDEXID(space_id,
0);
- sqlite3VdbeAddOp3(v, OP_OpenWrite, tab_cursor,
- tnum, space_ptr_reg);
- struct key_def *def = key_def_dup(pk_def);
- if (def == NULL) {
- sqlite3OomFault(parse->db);
- goto delete_from_cleanup;
- }
- sqlite3VdbeAppendP4(v, def, P4_KEYDEF);
-
+ sqlite3VdbeAddOp4(v, OP_OpenWrite, tab_cursor, tnum, 0,
+ (void *) space, P4_SPACEPTR);
VdbeComment((v, "%s", space->index[0]->def->name));
if (one_pass == ONEPASS_MULTI)
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index ca194e6..bb1a225 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -204,13 +204,8 @@ vdbe_has_table_read(struct Parse *parser, const struct Table *table)
* and Write cursors.
*/
if (op->opcode == OP_OpenRead || op->opcode == OP_OpenWrite) {
- assert(i > 1);
- struct VdbeOp *space_var_op =
- sqlite3VdbeGetOp(v, i - 1);
- assert(space_var_op != NULL);
- assert(space_var_op->opcode == OP_LoadPtr);
- struct space *space = space_var_op->p4.space;
-
+ assert(op->p4type == P4_SPACEPTR);
+ struct space *space = op->p4.space;
if (space->def->id == table->def->id)
return true;
@@ -1581,10 +1576,6 @@ sqlite3OpenTableAndIndices(Parse * pParse, /* Parsing context */
*piIdxCur = iBase;
struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pTab->tnum));
assert(space != NULL);
- int space_ptr_reg = ++pParse->nMem;
- sqlite3VdbeAddOp4(v, OP_LoadPtr, 0, space_ptr_reg, 0, (void*)space,
- P4_SPACEPTR);
-
/* One iteration of this cycle adds OpenRead/OpenWrite which
* opens cursor for current index.
*
@@ -1631,9 +1622,8 @@ sqlite3OpenTableAndIndices(Parse * pParse, /* Parsing context */
p5 = 0;
}
if (aToOpen == 0 || aToOpen[i + 1]) {
- sqlite3VdbeAddOp3(v, op, iIdxCur, pIdx->tnum,
- space_ptr_reg);
- sql_vdbe_set_p4_key_def(pParse, pIdx);
+ sqlite3VdbeAddOp4(v, op, iIdxCur, pIdx->tnum, 0,
+ (void *) space, P4_SPACEPTR);
sqlite3VdbeChangeP5(v, p5);
VdbeComment((v, "%s", pIdx->zName));
}
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index 31581b1..eea6a0b 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -685,14 +685,21 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */
pParent,
OP_OpenRead);
} else {
- sqlite3VdbeAddOp3(v,
+ struct space *space =
+ space_cache_find(
+ pIdx->
+ pTable->
+ def->id);
+ assert(space != NULL);
+ sqlite3VdbeAddOp4(v,
OP_OpenRead,
i,
pIdx->
tnum,
- 0);
- sql_vdbe_set_p4_key_def(pParse,
- pIdx);
+ 0,
+ (void *)space,
+ P4_SPACEPTR);
+
}
} else {
k = 0;
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 7a4d376..734d8cc 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1077,19 +1077,6 @@ case OP_Int64: { /* out2 */
break;
}
-/* Opcode: LoadPtr * P2 * P4 *
- * Synopsis: r[P2] = P4
- *
- * P4 is a generic or space pointer. Copy it into register P2.
- */
-case OP_LoadPtr: {
- pOut = out2Prerelease(p, pOp);
- assert(pOp->p4type == P4_PTR || pOp->p4type == P4_SPACEPTR );
- pOut->u.p = pOp->p4.space;
- pOut->flags = MEM_Ptr;
- break;
-}
-
#ifndef SQLITE_OMIT_FLOATING_POINT
/* Opcode: Real * P2 * P4 *
* Synopsis: r[P2]=P4
@@ -3137,26 +3124,15 @@ case OP_SetCookie: {
}
/* Opcode: OpenRead P1 P2 P3 P4 P5
- * Synopsis: index id = P2, space ptr = P3
+ * Synopsis: index id = P2, space ptr = P4
*
- * Open a cursor for a space specified by pointer in P3 and index
+ * Open a cursor for a space specified by pointer in P4 and index
* id in P2. Give the new cursor an identifier of P1. The P1
* values need not be contiguous but all P1 values should be
* small integers. It is an error for P1 to be negative.
- *
- * The P4 value may be a pointer to a key_def structure.
- * If it is a pointer to a key_def structure, then said structure
- * defines the content and collatining sequence of the index
- * being opened. Otherwise, P4 is NULL.
- *
- * If schema has changed since compile time, VDBE ends execution
- * with appropriate error message. The only exception is
- * when P5 is set to OPFLAG_FRESH_PTR, which means that
- * space pointer has been fetched in runtime right before
- * this opcode.
*/
/* Opcode: ReopenIdx P1 P2 P3 P4 P5
- * Synopsis: index id = P2, space ptr = P3
+ * Synopsis: index id = P2, space ptr = P4
*
* The ReopenIdx opcode works exactly like OpenRead except that
* it first checks to see if the cursor on P1 is already open
@@ -3166,7 +3142,7 @@ case OP_SetCookie: {
* The ReopenIdx opcode may only be used with P5 == 0.
*/
/* Opcode: OpenWrite P1 P2 P3 P4 P5
- * Synopsis: index id = P2, space ptr = P3
+ * Synopsis: index id = P2, space ptr = P4
*
* For now, OpenWrite is an alias for OpenRead.
* It exists just due legacy reasons and should be removed:
@@ -3182,9 +3158,7 @@ case OP_ReopenIdx: {
assert(pOp->p5==0 || pOp->p5==OPFLAG_SEEKEQ);
pCur = p->apCsr[pOp->p1];
p2 = pOp->p2;
- pIn3 = &aMem[pOp->p3];
- assert(pIn3->flags & MEM_Ptr);
- if (pCur && pCur->uc.pCursor->space == (struct space *) pIn3->u.p &&
+ if (pCur && pCur->uc.pCursor->space == pOp->p4.space &&
pCur->uc.pCursor->index->def->iid == SQLITE_PAGENO_TO_INDEXID(p2)) {
goto open_cursor_set_hints;
}
@@ -3209,10 +3183,7 @@ case OP_OpenWrite:
goto abort_due_to_error;
}
p2 = pOp->p2;
- pIn3 = &aMem[pOp->p3];
- assert(pIn3->flags & MEM_Ptr);
- struct space *space = ((struct space *) pIn3->u.p);
- assert(space != NULL);
+ struct space *space = pOp->p4.space;
struct index *index = space_index(space, SQLITE_PAGENO_TO_INDEXID(p2));
assert(index != NULL);
/*
--
2.7.4
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] sql: refactor vdbe_emit_open_cursor calls
2018-07-10 13:44 ` Kirill Shcherbatov
@ 2018-07-10 14:59 ` Vladislav Shpilevoy
2018-07-10 15:45 ` Kirill Shcherbatov
0 siblings, 1 reply; 10+ messages in thread
From: Vladislav Shpilevoy @ 2018-07-10 14:59 UTC (permalink / raw)
To: Kirill Shcherbatov, tarantool-patches
>> 2. vdbe_emit_open_cursor has outdated comment in the header.
> * @param parse_context Parse context.
> * @param cursor Number of cursor to be created.
> - * @param index_id Encoded index id (encoding is void actually, so
> - * pas it as is). In future will be replaced with pointer
> - * to struct index.
> + * @param index_id index id. In future will be replaced with
> + * pointer to struct index.
> * @retval address of last opcode.
1. Still out dated. Where is '@param space'?
2. Why vdbe_emit_open_cursor opens the cursor using index_id,
but opcode OP_OpenRead/Write still uses it as tnum?
> =========================================
>
> Made vdbe_emit_open_cursor calls consistent:
> now it uses index id everywhere.
> This required to change a way to detect that
> VDBE has openned Read cursor to specified table
> in vdbe_has_table_read to write result of insert
> in temp table if required.
> ---
> src/box/sql/analyze.c | 3 +-
> src/box/sql/build.c | 3 +-
> src/box/sql/expr.c | 5 ++-
> src/box/sql/fkey.c | 3 +-
> src/box/sql/insert.c | 96 +++++++++++++++++++++++++++++--------------------
> src/box/sql/select.c | 4 +--
> src/box/sql/sqliteInt.h | 5 ++-
> src/box/sql/where.c | 10 ++++--
> 8 files changed, 77 insertions(+), 52 deletions(-)
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH v1 2/2] sql: remove OP_LoadPtr
2018-07-10 13:45 ` [tarantool-patches] [PATCH v1 2/2] sql: remove OP_LoadPtr Kirill Shcherbatov
@ 2018-07-10 14:59 ` Vladislav Shpilevoy
0 siblings, 0 replies; 10+ messages in thread
From: Vladislav Shpilevoy @ 2018-07-10 14:59 UTC (permalink / raw)
To: Kirill Shcherbatov, tarantool-patches
Thanks for finishing the patch! I have force pushed more deletions
on the branch.
On 10/07/2018 16:45, Kirill Shcherbatov wrote:
> From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
>
> This opcode was used when Vdbe had to store key_def in P4 for
> OP_OpenRead/Write, so P4 was occupied and OP_LoadPtr was used to
> store space pointer in another opcode. But now P4 is free for
> OpenRead/Write and space pointer can be stored here.
>
> Alongside, some useless key_def duplications are removed.
> ---
> src/box/sql/analyze.c | 8 ++------
> src/box/sql/build.c | 20 ++++++--------------
> src/box/sql/delete.c | 16 ++--------------
> src/box/sql/insert.c | 18 ++++--------------
> src/box/sql/pragma.c | 15 +++++++++++----
> src/box/sql/vdbe.c | 41 ++++++-----------------------------------
> 6 files changed, 31 insertions(+), 87 deletions(-)
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] sql: refactor vdbe_emit_open_cursor calls
2018-07-10 14:59 ` Vladislav Shpilevoy
@ 2018-07-10 15:45 ` Kirill Shcherbatov
2018-07-10 15:56 ` Vladislav Shpilevoy
0 siblings, 1 reply; 10+ messages in thread
From: Kirill Shcherbatov @ 2018-07-10 15:45 UTC (permalink / raw)
To: tarantool-patches; +Cc: Vladislav Shpilevoy
> 1. Still out dated. Where is '@param space'?
+ * @param space Pointer to space object.
> 2. Why vdbe_emit_open_cursor opens the cursor using index_id,
> but opcode OP_OpenRead/Write still uses it as tnum?
It is much more convenient to do this as a part of next commit.
I temporary put this changes as a commit at head
"sql: uniform OP_Open{Write,Read} ops to use tnum"
diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index d84f1c0..699a4e8 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -891,9 +891,10 @@ analyzeOneTable(Parse * pParse, /* Parser context */
/* Open a read-only cursor on the index being analyzed. */
struct space *space =
space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum));
+ int idx_id = SQLITE_PAGENO_TO_INDEXID(pIdx->tnum);
assert(space != NULL);
- sqlite3VdbeAddOp4(v, OP_OpenRead, iIdxCur, pIdx->tnum,
- 0, (void *) space, P4_SPACEPTR);
+ sqlite3VdbeAddOp4(v, OP_OpenRead, iIdxCur, idx_id, 0,
+ (void *) space, P4_SPACEPTR);
VdbeComment((v, "%s", pIdx->zName));
/* Invoke the stat_init() function. The arguments are:
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 24122e8..ca1e77d 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -335,10 +335,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
iAddrOnce = sqlite3VdbeAddOp0(v, OP_Once);
VdbeCoverage(v);
}
- int tnum =
- SQLITE_PAGENO_FROM_SPACEID_AND_INDEXID(space_id,
- 0);
- sqlite3VdbeAddOp4(v, OP_OpenWrite, tab_cursor, tnum, 0,
+ sqlite3VdbeAddOp4(v, OP_OpenWrite, tab_cursor, 0, 0,
(void *) space, P4_SPACEPTR);
VdbeComment((v, "%s", space->index[0]->def->name));
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index bb1a225..6aede16 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -1622,7 +1622,9 @@ sqlite3OpenTableAndIndices(Parse * pParse, /* Parsing context */
p5 = 0;
}
if (aToOpen == 0 || aToOpen[i + 1]) {
- sqlite3VdbeAddOp4(v, op, iIdxCur, pIdx->tnum, 0,
+ int idx_id =
+ SQLITE_PAGENO_TO_INDEXID(pIdx->tnum);
+ sqlite3VdbeAddOp4(v, op, iIdxCur, idx_id, 0,
(void *) space, P4_SPACEPTR);
sqlite3VdbeChangeP5(v, p5);
VdbeComment((v, "%s", pIdx->zName));
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index 1e8a4c9..8ff5bbc 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -691,9 +691,12 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */
struct space *space =
space_cache_find(pIdx->pTable->
def->id);
+ int idx_id =
+ SQLITE_PAGENO_TO_INDEXID(pIdx->
+ tnum);
assert(space != NULL);
sqlite3VdbeAddOp4(v, OP_OpenRead, i,
- pIdx->tnum,0,
+ idx_id, 0,
(void *) space,
P4_SPACEPTR);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 6d7db13..88579bf 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -3159,9 +3159,8 @@ case OP_ReopenIdx: {
pCur = p->apCsr[pOp->p1];
p2 = pOp->p2;
if (pCur && pCur->uc.pCursor->space == pOp->p4.space &&
- pCur->uc.pCursor->index->def->iid == SQLITE_PAGENO_TO_INDEXID(p2)) {
+ pCur->uc.pCursor->index->def->iid == p2)
goto open_cursor_set_hints;
- }
/* If the cursor is not currently open or is open on a different
* index, then fall through into OP_OpenRead to force a reopen
*/
@@ -3178,7 +3177,7 @@ case OP_OpenWrite:
}
p2 = pOp->p2;
struct space *space = pOp->p4.space;
- struct index *index = space_index(space, SQLITE_PAGENO_TO_INDEXID(p2));
+ struct index *index = space_index(space, p2);
assert(index != NULL);
/*
* Since Tarantool iterator provides the full tuple,
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] sql: refactor vdbe_emit_open_cursor calls
2018-07-10 15:45 ` Kirill Shcherbatov
@ 2018-07-10 15:56 ` Vladislav Shpilevoy
2018-07-10 16:37 ` Kirill Shcherbatov
0 siblings, 1 reply; 10+ messages in thread
From: Vladislav Shpilevoy @ 2018-07-10 15:56 UTC (permalink / raw)
To: Kirill Shcherbatov, tarantool-patches
>> 2. Why vdbe_emit_open_cursor opens the cursor using index_id,
>> but opcode OP_OpenRead/Write still uses it as tnum?
> It is much more convenient to do this as a part of next commit.
> I temporary put this changes as a commit at head
> "sql: uniform OP_Open{Write,Read} ops to use tnum"
Maybe it is more convenient, but it is not more correct. Your
first commit still does not fix the mess with index_id/tnum.
Please, squash it into your first commit.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] sql: refactor vdbe_emit_open_cursor calls
2018-07-10 15:56 ` Vladislav Shpilevoy
@ 2018-07-10 16:37 ` Kirill Shcherbatov
2018-07-10 16:52 ` Vladislav Shpilevoy
0 siblings, 1 reply; 10+ messages in thread
From: Kirill Shcherbatov @ 2018-07-10 16:37 UTC (permalink / raw)
To: tarantool-patches; +Cc: Vladislav Shpilevoy
> Maybe it is more convenient, but it is not more correct. Your
> first commit still does not fix the mess with index_id/tnum.
>
> Please, squash it into your first commit.
Ok, I've done this.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [tarantool-patches] Re: [PATCH v1 1/1] sql: refactor vdbe_emit_open_cursor calls
2018-07-10 16:37 ` Kirill Shcherbatov
@ 2018-07-10 16:52 ` Vladislav Shpilevoy
0 siblings, 0 replies; 10+ messages in thread
From: Vladislav Shpilevoy @ 2018-07-10 16:52 UTC (permalink / raw)
To: Kirill Shcherbatov, tarantool-patches, Nikita Pettik
Thanks for the squash. I have force pushed some comment
style violation fixes.
Nikita, please, do a second review.
On 10/07/2018 19:37, Kirill Shcherbatov wrote:
>> Maybe it is more convenient, but it is not more correct. Your
>> first commit still does not fix the mess with index_id/tnum.
>>
>> Please, squash it into your first commit.
> Ok, I've done this.
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-07-10 16:52 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-09 12:42 [tarantool-patches] [PATCH v1 1/1] sql: refactor vdbe_emit_open_cursor calls Kirill Shcherbatov
2018-07-10 11:48 ` [tarantool-patches] " Vladislav Shpilevoy
2018-07-10 13:44 ` Kirill Shcherbatov
2018-07-10 14:59 ` Vladislav Shpilevoy
2018-07-10 15:45 ` Kirill Shcherbatov
2018-07-10 15:56 ` Vladislav Shpilevoy
2018-07-10 16:37 ` Kirill Shcherbatov
2018-07-10 16:52 ` Vladislav Shpilevoy
2018-07-10 13:45 ` [tarantool-patches] [PATCH v1 2/2] sql: remove OP_LoadPtr Kirill Shcherbatov
2018-07-10 14:59 ` [tarantool-patches] " Vladislav Shpilevoy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox