[tarantool-patches] Re: [PATCH v1 1/1] sql: refactor vdbe_emit_open_cursor calls
Kirill Shcherbatov
kshcherbatov at tarantool.org
Tue Jul 10 16:44:59 MSK 2018
> 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
More information about the Tarantool-patches
mailing list