* [tarantool-patches] [PATCH 0/3] sql: implement point where for DELETE stmts
@ 2018-06-01 15:16 Kirill Yukhin
2018-06-01 15:16 ` [tarantool-patches] [PATCH 1/3] sql: fetch primary index for affinity only Kirill Yukhin
` (3 more replies)
0 siblings, 4 replies; 19+ messages in thread
From: Kirill Yukhin @ 2018-06-01 15:16 UTC (permalink / raw)
To: v.shpilevoy; +Cc: tarantool-patches, Kirill Yukhin
This patches consists of two preparational patches and the
main patch which enables DELETE's from spaces created w/ Lua.
Issue: https://github.com/tarantool/tarantool/issues/3235
Branch: https://github.com/tarantool/tarantool/tree/kyukhin/gh-3235-delete-with-where
Kirill Yukhin (3):
sql: fetch primary index for affinity only
sql: remove expressions from SQL indexes
sql: implement point where for DELETE stmts
src/box/field_def.c | 1 +
src/box/sql.c | 4 +-
src/box/sql/build.c | 63 ++++++-----
src/box/sql/delete.c | 103 +++++++++++------
src/box/sql/expr.c | 12 +-
src/box/sql/insert.c | 105 +++++++++---------
src/box/sql/sqliteInt.h | 63 +++++++++--
src/box/sql/where.c | 251 +++++++++++++++++++++++++++++++-----------
src/box/sql/whereInt.h | 2 +
src/box/sql/wherecode.c | 128 +++++++++++++++------
src/box/sql/whereexpr.c | 36 +-----
test/sql-tap/delete1.test.lua | 24 +++-
12 files changed, 527 insertions(+), 265 deletions(-)
--
2.16.2
^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] [PATCH 1/3] sql: fetch primary index for affinity only
2018-06-01 15:16 [tarantool-patches] [PATCH 0/3] sql: implement point where for DELETE stmts Kirill Yukhin
@ 2018-06-01 15:16 ` Kirill Yukhin
2018-06-01 18:00 ` [tarantool-patches] " Vladislav Shpilevoy
2018-06-01 15:16 ` [tarantool-patches] [PATCH 2/3] sql: remove expressions from SQL indexes Kirill Yukhin
` (2 subsequent siblings)
3 siblings, 1 reply; 19+ messages in thread
From: Kirill Yukhin @ 2018-06-01 15:16 UTC (permalink / raw)
To: v.shpilevoy; +Cc: tarantool-patches, Kirill Yukhin
This small patch removes usages of primary index throughout
code sql_table_delete_from, limiting use to fetching of
affinities only. We cannot use space_def here, since primary
index might contain calculated columns.
Part of #3235
---
src/box/sql/delete.c | 54 ++++++++++++++++++++++++++++++++++++----------------
1 file changed, 38 insertions(+), 16 deletions(-)
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index ddad54b..28713c8 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -183,6 +183,16 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
struct NameContext nc;
memset(&nc, 0, sizeof(nc));
nc.pParse = parse;
+ if (tab_list->a[0].pTab == NULL) {
+ struct Table *t = malloc(sizeof(struct Table));
+ if (t == NULL) {
+ sqlite3OomFault(parse->db);
+ goto delete_from_cleanup;
+ }
+ assert(space != NULL);
+ t->def = space->def;
+ tab_list->a[0].pTab = t;
+ }
nc.pSrcList = tab_list;
if (sqlite3ResolveExprNames(&nc, where))
goto delete_from_cleanup;
@@ -196,7 +206,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
* is held in ephemeral table, there is no PK for
* it, so columns should be loaded manually.
*/
- struct Index *pk = NULL;
+ struct key_def *pk_def = NULL;
int reg_pk = parse->nMem + 1;
int pk_len;
int eph_cursor = parse->nTab++;
@@ -207,13 +217,17 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
sqlite3VdbeAddOp2(v, OP_OpenTEphemeral,
eph_cursor, pk_len);
} else {
- pk = sqlite3PrimaryKeyIndex(table);
- assert(pk != NULL);
- pk_len = index_column_count(pk);
+ assert(space->index_count > 0);
+ pk_def = key_def_dup(space->index[0]->def->key_def);
+ if(pk_def == NULL) {
+ sqlite3OomFault(parse->db);
+ goto delete_from_cleanup;
+ }
+ pk_len = pk_def->part_count;
parse->nMem += pk_len;
- sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, eph_cursor,
- pk_len);
- sql_vdbe_set_p4_key_def(parse, pk);
+ sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, eph_cursor,
+ pk_len, 0,
+ (char *)pk_def, P4_KEYDEF);
}
/* Construct a query to find the primary key for
@@ -252,11 +266,9 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
/* Extract the primary key for the current row */
if (!is_view) {
for (int i = 0; i < pk_len; i++) {
- assert(pk->aiColumn[i] >= 0);
sqlite3ExprCodeGetColumnOfTable(v, table->def,
tab_cursor,
- pk->
- aiColumn[i],
+ pk_def->parts[i].fieldno,
reg_pk + i);
}
} else {
@@ -287,10 +299,14 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
* key.
*/
key_len = 0;
- const char *zAff = is_view ? NULL :
- sqlite3IndexAffinityStr(parse->db, pk);
+ const char *aff = NULL;
+ struct Index *pk = sqlite3PrimaryKeyIndex(table);
+ if (pk != NULL) {
+ aff = is_view ? NULL :
+ sqlite3IndexAffinityStr(parse->db, pk);
+ }
sqlite3VdbeAddOp4(v, OP_MakeRecord, reg_pk, pk_len,
- reg_key, zAff, pk_len);
+ reg_key, aff, pk_len);
/* Set flag to save memory allocating one
* by malloc.
*/
@@ -325,8 +341,14 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
(void *)space, P4_SPACEPTR);
sqlite3VdbeAddOp3(v, OP_OpenWrite, tab_cursor,
table->tnum, space_ptr_reg);
- sql_vdbe_set_p4_key_def(parse, pk);
- VdbeComment((v, "%s", pk->zName));
+ struct key_def *def = key_def_dup(pk_def);
+ if (def == NULL) {
+ sqlite3OomFault(parse->db);
+ goto delete_from_cleanup;
+ }
+ sqlite3VdbeAppendP4(v, def, P4_KEYDEF);
+
+ VdbeComment((v, "%s", space->index[0]->def->name));
if (one_pass == ONEPASS_MULTI)
sqlite3VdbeJumpHere(v, iAddrOnce);
@@ -339,7 +361,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
if (one_pass != ONEPASS_OFF) {
/* OP_Found will use an unpacked key. */
assert(key_len == pk_len);
- assert(pk != NULL || table->pSelect != NULL);
+ assert(pk_def != NULL || table->pSelect != NULL);
sqlite3VdbeAddOp4Int(v, OP_NotFound, tab_cursor,
addr_bypass, reg_key, key_len);
--
2.16.2
^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] [PATCH 2/3] sql: remove expressions from SQL indexes
2018-06-01 15:16 [tarantool-patches] [PATCH 0/3] sql: implement point where for DELETE stmts Kirill Yukhin
2018-06-01 15:16 ` [tarantool-patches] [PATCH 1/3] sql: fetch primary index for affinity only Kirill Yukhin
@ 2018-06-01 15:16 ` Kirill Yukhin
2018-06-01 18:00 ` [tarantool-patches] " Vladislav Shpilevoy
2018-06-01 15:16 ` [tarantool-patches] [PATCH 3/3] sql: implement point where for DELETE stmts Kirill Yukhin
2018-06-14 12:41 ` [tarantool-patches] Re: [PATCH 0/3] " Kirill Yukhin
3 siblings, 1 reply; 19+ messages in thread
From: Kirill Yukhin @ 2018-06-01 15:16 UTC (permalink / raw)
To: v.shpilevoy; +Cc: tarantool-patches, Kirill Yukhin
Legacy SQL FE was able to create indexes w/ expressions.
Tarantool will employ diofferenct scheme to implement
functional indexes, thus code handling it in SQL FE is
dead and redundant. Remove it.
Part of #3235
---
src/box/sql/build.c | 22 ++++++---------
src/box/sql/delete.c | 3 +--
src/box/sql/expr.c | 12 ++-------
src/box/sql/insert.c | 72 +++++++++++++++----------------------------------
src/box/sql/sqliteInt.h | 6 -----
src/box/sql/where.c | 27 ++-----------------
src/box/sql/wherecode.c | 13 +++------
src/box/sql/whereexpr.c | 36 +++----------------------
8 files changed, 42 insertions(+), 149 deletions(-)
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 28e4d7a..65bba1f 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -252,7 +252,6 @@ static void
freeIndex(sqlite3 * db, Index * p)
{
sql_expr_delete(db, p->pPartIdxWhere, false);
- sql_expr_list_delete(db, p->aColExpr);
sqlite3DbFree(db, p->zColAff);
sqlite3DbFree(db, p);
}
@@ -3042,8 +3041,7 @@ sql_create_index(struct Parse *parse, struct Token *token,
/* Analyze the list of expressions that form the terms of the index and
* report any errors. In the common case where the expression is exactly
- * a table column, store that column in aiColumn[]. For general expressions,
- * populate pIndex->aColExpr and store XN_EXPR (-2) in aiColumn[].
+ * a table column, store that column in aiColumn[].
*
* TODO: Issue a warning if two or more columns of the index are identical.
* TODO: Issue a warning if the table primary key is used as part of the
@@ -3938,17 +3936,13 @@ sqlite3UniqueConstraint(Parse * pParse, /* Parsing context */
Table *pTab = pIdx->pTable;
sqlite3StrAccumInit(&errMsg, pParse->db, 0, 0, 200);
- if (pIdx->aColExpr) {
- sqlite3XPrintf(&errMsg, "index '%q'", pIdx->zName);
- } else {
- for (j = 0; j < pIdx->nColumn; j++) {
- char *zCol;
- assert(pIdx->aiColumn[j] >= 0);
- zCol = pTab->def->fields[pIdx->aiColumn[j]].name;
- if (j)
- sqlite3StrAccumAppend(&errMsg, ", ", 2);
- sqlite3XPrintf(&errMsg, "%s.%s", pTab->def->name, zCol);
- }
+ for (j = 0; j < pIdx->nColumn; j++) {
+ char *zCol;
+ assert(pIdx->aiColumn[j] >= 0);
+ zCol = pTab->def->fields[pIdx->aiColumn[j]].name;
+ if (j)
+ sqlite3StrAccumAppend(&errMsg, ", ", 2);
+ sqlite3XPrintf(&errMsg, "%s.%s", pTab->def->name, zCol);
}
zErr = sqlite3StrAccumFinish(&errMsg);
sqlite3HaltConstraint(pParse,
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 28713c8..2b59130 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -564,8 +564,7 @@ sql_generate_index_key(struct Parse *parse, struct Index *index, int cursor,
prev->pPartIdxWhere != NULL))
prev = NULL;
for (int j = 0; j < col_cnt; j++) {
- if (prev != NULL && prev->aiColumn[j] == index->aiColumn[j]
- && prev->aiColumn[j] != XN_EXPR) {
+ if (prev != NULL && prev->aiColumn[j] == index->aiColumn[j]) {
/*
* This column was already computed by the
* previous index.
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 8866f6f..a69d38b 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -3484,16 +3484,8 @@ sqlite3ExprCodeLoadIndexColumn(Parse * pParse, /* The parsing context */
)
{
i16 iTabCol = pIdx->aiColumn[iIdxCol];
- if (iTabCol == XN_EXPR) {
- assert(pIdx->aColExpr);
- assert(pIdx->aColExpr->nExpr > iIdxCol);
- pParse->iSelfTab = iTabCur;
- sqlite3ExprCodeCopy(pParse, pIdx->aColExpr->a[iIdxCol].pExpr,
- regOut);
- } else {
- sqlite3ExprCodeGetColumnOfTable(pParse->pVdbe, pIdx->pTable->def,
- iTabCur, iTabCol, regOut);
- }
+ sqlite3ExprCodeGetColumnOfTable(pParse->pVdbe, pIdx->pTable->def,
+ iTabCur, iTabCol, regOut);
}
void
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 59c61c7..3dbf855 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -98,21 +98,9 @@ sqlite3IndexAffinityStr(sqlite3 * db, Index * pIdx)
}
for (n = 0; n < nColumn; n++) {
i16 x = pIdx->aiColumn[n];
- if (x >= 0) {
- char affinity = pIdx->pTable->
- def->fields[x].affinity;
- pIdx->zColAff[n] = affinity;
- } else {
- char aff;
- assert(x == XN_EXPR);
- assert(pIdx->aColExpr != 0);
- aff =
- sqlite3ExprAffinity(pIdx->aColExpr->a[n].
- pExpr);
- if (aff == 0)
- aff = AFFINITY_BLOB;
- pIdx->zColAff[n] = aff;
- }
+ char affinity = pIdx->pTable->
+ def->fields[x].affinity;
+ pIdx->zColAff[n] = affinity;
}
pIdx->zColAff[n] = 0;
}
@@ -1256,34 +1244,24 @@ sqlite3GenerateConstraintChecks(Parse * pParse, /* The parser context */
for (i = 0; i < nIdxCol; i++) {
int iField = pIdx->aiColumn[i];
int x;
- if (iField == XN_EXPR) {
- pParse->ckBase = regNewData + 1;
- sqlite3ExprCodeCopy(pParse,
- pIdx->aColExpr->a[i].pExpr,
- regIdx + i);
- pParse->ckBase = 0;
- VdbeComment((v, "%s column %d", pIdx->zName,
- i));
- } else {
- /* OP_SCopy copies value in separate register,
- * which later will be used by OP_NoConflict.
- * But OP_NoConflict is necessary only in cases
- * when bytecode is needed for proper UNIQUE
- * constraint handling.
- */
- if (uniqueByteCodeNeeded) {
- if (iField == pTab->iPKey)
- x = regNewData;
- else
- x = iField + regNewData + 1;
-
- assert(iField >= 0);
- sqlite3VdbeAddOp2(v, OP_SCopy,
- x, regIdx + i);
- VdbeComment((v, "%s",
- pTab->def->fields[
- iField].name));
- }
+ /* OP_SCopy copies value in separate register,
+ * which later will be used by OP_NoConflict.
+ * But OP_NoConflict is necessary only in cases
+ * when bytecode is needed for proper UNIQUE
+ * constraint handling.
+ */
+ if (uniqueByteCodeNeeded) {
+ if (iField == pTab->iPKey)
+ x = regNewData;
+ else
+ x = iField + regNewData + 1;
+
+ assert(iField >= 0);
+ sqlite3VdbeAddOp2(v, OP_SCopy,
+ x, regIdx + i);
+ VdbeComment((v, "%s",
+ pTab->def->fields[
+ iField].name));
}
}
@@ -1688,14 +1666,6 @@ xferCompatibleIndex(Index * pDest, Index * pSrc)
if (pSrc->aiColumn[i] != pDest->aiColumn[i]) {
return 0; /* Different columns indexed */
}
- if (pSrc->aiColumn[i] == XN_EXPR) {
- assert(pSrc->aColExpr != 0 && pDest->aColExpr != 0);
- if (sqlite3ExprCompare(pSrc->aColExpr->a[i].pExpr,
- pDest->aColExpr->a[i].pExpr,
- -1) != 0) {
- return 0; /* Different expressions in the index */
- }
- }
if (sql_index_column_sort_order(pSrc, i) !=
sql_index_column_sort_order(pDest, i)) {
return 0; /* Different sort orders */
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 01351a1..943fda9 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2116,7 +2116,6 @@ struct Index {
/** Array of collation identifiers. */
uint32_t *coll_id_array;
Expr *pPartIdxWhere; /* WHERE clause for partial indices */
- ExprList *aColExpr; /* Column expressions */
int tnum; /* DB Page containing root of this index */
u16 nColumn; /* Number of columns stored in the index */
u8 onError; /* ON_CONFLICT_ACTION_ABORT, _IGNORE, _REPLACE,
@@ -2161,11 +2160,6 @@ index_field_tuple_est(struct Index *idx, uint32_t field);
#define IsUniqueIndex(X) (((X)->idxType == SQLITE_IDXTYPE_UNIQUE) || \
((X)->idxType == SQLITE_IDXTYPE_PRIMARYKEY))
-/* The Index.aiColumn[] values are normally positive integer. But
- * there are some negative values that have special meaning:
- */
-#define XN_EXPR (-2) /* Indexed column is an expression */
-
#ifdef DEFAULT_TUPLE_COUNT
#undef DEFAULT_TUPLE_COUNT
#endif
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index e791647..d312587 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -265,11 +265,6 @@ whereScanNext(WhereScan * pScan)
for (pTerm = pWC->a + k; k < pWC->nTerm; k++, pTerm++) {
if (pTerm->leftCursor == iCur
&& pTerm->u.leftColumn == iColumn
- && (iColumn != XN_EXPR
- || sqlite3ExprCompare(pTerm->pExpr->
- pLeft,
- pScan->pIdxExpr,
- iCur) == 0)
&& (pScan->iEquiv <= 1
|| !ExprHasProperty(pTerm->pExpr,
EP_FromJoin))
@@ -377,9 +372,7 @@ whereScanInit(WhereScan * pScan, /* The WhereScan object being initialized */
if (pIdx) {
int j = iColumn;
iColumn = pIdx->aiColumn[j];
- if (iColumn == XN_EXPR) {
- pScan->pIdxExpr = pIdx->aColExpr->a[j].pExpr;
- } else if (iColumn >= 0) {
+ if (iColumn >= 0) {
char affinity =
pIdx->pTable->def->fields[iColumn].affinity;
pScan->idxaff = affinity;
@@ -387,8 +380,6 @@ whereScanInit(WhereScan * pScan, /* The WhereScan object being initialized */
pScan->coll = sql_index_collation(pIdx, j, &id);
pScan->is_column_seen = true;
}
- } else if (iColumn == XN_EXPR) {
- return 0;
}
pScan->opMask = opMask;
pScan->k = 0;
@@ -2713,7 +2704,6 @@ indexMightHelpWithOrderBy(WhereLoopBuilder * pBuilder,
Index * pIndex, int iCursor)
{
ExprList *pOB;
- ExprList *aColExpr;
int ii, jj;
int nIdxCol = index_column_count(pIndex);
if (index_is_unordered(pIndex))
@@ -2729,16 +2719,6 @@ indexMightHelpWithOrderBy(WhereLoopBuilder * pBuilder,
if (pExpr->iColumn == pIndex->aiColumn[jj])
return 1;
}
- } else if ((aColExpr = pIndex->aColExpr) != 0) {
- for (jj = 0; jj < nIdxCol; jj++) {
- if (pIndex->aiColumn[jj] != XN_EXPR)
- continue;
- if (sqlite3ExprCompare
- (pExpr, aColExpr->a[jj].pExpr,
- iCursor) == 0) {
- return 1;
- }
- }
}
}
return 0;
@@ -3429,10 +3409,7 @@ wherePathSatisfiesOrderBy(WhereInfo * pWInfo, /* The WHERE clause */
if (pOBExpr->iColumn != iColumn)
continue;
} else {
- if (sqlite3ExprCompare(pOBExpr,
- pIndex->aColExpr->a[j].pExpr, iCur)) {
- continue;
- }
+ continue;
}
if (iColumn >= 0) {
bool is_found;
diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index 09b2671..bf2a2a2 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -48,8 +48,6 @@ static const char *
explainIndexColumnName(Index * pIdx, int i)
{
i = pIdx->aiColumn[i];
- if (i == XN_EXPR)
- return "<expr>";
return pIdx->pTable->def->fields[i].name;
}
@@ -719,7 +717,6 @@ codeAllEqualityTerms(Parse * pParse, /* Parsing context */
for (j = 0; j < nSkip; j++) {
sqlite3VdbeAddOp3(v, OP_Column, iIdxCur,
pIdx->aiColumn[j], regBase + j);
- testcase(pIdx->aiColumn[j] == XN_EXPR);
VdbeComment((v, "%s", explainIndexColumnName(pIdx, j)));
}
}
@@ -1259,8 +1256,8 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about t
* FYI: entries in an index are ordered as follows:
* NULL, ... NULL, min_value, ...
*/
- if ((j >= 0 && pIdx->pTable->def->fields[j].is_nullable)
- || j == XN_EXPR) {
+ if (j >= 0
+ && pIdx->pTable->def->fields[j].is_nullable) {
assert(pLoop->nSkip == 0);
bSeekPastNull = 1;
nExtraReg = 1;
@@ -1305,11 +1302,9 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about t
#endif
if (pRangeStart == 0) {
j = pIdx->aiColumn[nEq];
- if ((j >= 0 &&
- pIdx->pTable->def->fields[j].is_nullable)||
- j == XN_EXPR) {
+ if (j >= 0 &&
+ pIdx->pTable->def->fields[j].is_nullable)
bSeekPastNull = 1;
- }
}
}
assert(pRangeEnd == 0
diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
index aa6d452..e5d6b44 100644
--- a/src/box/sql/whereexpr.c
+++ b/src/box/sql/whereexpr.c
@@ -887,26 +887,19 @@ exprSelectUsage(WhereMaskSet * pMaskSet, Select * pS)
* in any index. Return TRUE (1) if pExpr is an indexed term and return
* FALSE (0) if not. If TRUE is returned, also set *piCur to the cursor
* number of the table that is indexed and *piColumn to the column number
- * of the column that is indexed, or XN_EXPR (-2) if an expression is being
- * indexed.
+ * of the column that is indexed.
*
* If pExpr is a TK_COLUMN column reference, then this routine always returns
* true even if that particular column is not indexed, because the column
* might be added to an automatic index later.
*/
static int
-exprMightBeIndexed(SrcList * pFrom, /* The FROM clause */
- int op, /* The specific comparison operator */
- Bitmask mPrereq, /* Bitmask of FROM clause terms referenced by pExpr */
+exprMightBeIndexed(int op, /* The specific comparison operator */
Expr * pExpr, /* An operand of a comparison operator */
int *piCur, /* Write the referenced table cursor number here */
int *piColumn /* Write the referenced table column number here */
)
{
- Index *pIdx;
- int i;
- int iCur;
-
/* If this expression is a vector to the left or right of a
* inequality constraint (>, <, >= or <=), perform the processing
* on the first element of the vector.
@@ -923,27 +916,6 @@ exprMightBeIndexed(SrcList * pFrom, /* The FROM clause */
*piColumn = pExpr->iColumn;
return 1;
}
- if (mPrereq == 0)
- return 0; /* No table references */
- if ((mPrereq & (mPrereq - 1)) != 0)
- return 0; /* Refs more than one table */
- for (i = 0; mPrereq > 1; i++, mPrereq >>= 1) {
- }
- iCur = pFrom->a[i].iCursor;
- for (pIdx = pFrom->a[i].pTab->pIndex; pIdx; pIdx = pIdx->pNext) {
- if (pIdx->aColExpr == 0)
- continue;
- for (i = 0; i < pIdx->nColumn; i++) {
- if (pIdx->aiColumn[i] != XN_EXPR)
- continue;
- if (sqlite3ExprCompare
- (pExpr, pIdx->aColExpr->a[i].pExpr, iCur) == 0) {
- *piCur = iCur;
- *piColumn = XN_EXPR;
- return 1;
- }
- }
- }
return 0;
}
@@ -1038,13 +1010,13 @@ exprAnalyze(SrcList * pSrc, /* the FROM clause */
}
if (exprMightBeIndexed
- (pSrc, op, prereqLeft, pLeft, &iCur, &iColumn)) {
+ (op, pLeft, &iCur, &iColumn)) {
pTerm->leftCursor = iCur;
pTerm->u.leftColumn = iColumn;
pTerm->eOperator = operatorMask(op) & opMask;
}
if (pRight
- && exprMightBeIndexed(pSrc, op, pTerm->prereqRight, pRight,
+ && exprMightBeIndexed(op, pRight,
&iCur, &iColumn)
) {
WhereTerm *pNew;
--
2.16.2
^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] [PATCH 3/3] sql: implement point where for DELETE stmts
2018-06-01 15:16 [tarantool-patches] [PATCH 0/3] sql: implement point where for DELETE stmts Kirill Yukhin
2018-06-01 15:16 ` [tarantool-patches] [PATCH 1/3] sql: fetch primary index for affinity only Kirill Yukhin
2018-06-01 15:16 ` [tarantool-patches] [PATCH 2/3] sql: remove expressions from SQL indexes Kirill Yukhin
@ 2018-06-01 15:16 ` Kirill Yukhin
2018-06-01 18:00 ` [tarantool-patches] " Vladislav Shpilevoy
2018-06-14 12:41 ` [tarantool-patches] Re: [PATCH 0/3] " Kirill Yukhin
3 siblings, 1 reply; 19+ messages in thread
From: Kirill Yukhin @ 2018-06-01 15:16 UTC (permalink / raw)
To: v.shpilevoy; +Cc: tarantool-patches, Kirill Yukhin
This patch implements support of SQL's DELETE statemets
which a accompanied by point WHERE point constraints.
This patch doesn't support any kinds of nested selects
or JOINs.
Part of #3235
---
src/box/field_def.c | 1 +
src/box/sql.c | 4 +-
src/box/sql/build.c | 41 +++++---
src/box/sql/delete.c | 48 ++++++---
src/box/sql/insert.c | 33 +++++++
src/box/sql/sqliteInt.h | 57 ++++++++++-
src/box/sql/where.c | 224 +++++++++++++++++++++++++++++++++++-------
src/box/sql/whereInt.h | 2 +
src/box/sql/wherecode.c | 115 +++++++++++++++++-----
test/sql-tap/delete1.test.lua | 24 +++--
10 files changed, 448 insertions(+), 101 deletions(-)
diff --git a/src/box/field_def.c b/src/box/field_def.c
index 4d39d03..8dbead6 100644
--- a/src/box/field_def.c
+++ b/src/box/field_def.c
@@ -95,6 +95,7 @@ const struct opt_def field_def_reg[] = {
nullable_action, NULL),
OPT_DEF("collation", OPT_UINT32, struct field_def, coll_id),
OPT_DEF("default", OPT_STRPTR, struct field_def, default_value),
+ OPT_DEF("affinity", OPT_UINT32, struct field_def, affinity),
OPT_END,
};
diff --git a/src/box/sql.c b/src/box/sql.c
index 7379cb4..b1d346e 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1453,7 +1453,7 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
uint32_t cid = def->fields[i].coll_id;
struct field_def *field = &def->fields[i];
const char *default_str = field->default_value;
- int base_len = 4;
+ int base_len = 5;
if (cid != COLL_NONE)
base_len += 1;
if (default_str != NULL)
@@ -1474,6 +1474,8 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
assert(def->fields[i].is_nullable ==
action_is_nullable(def->fields[i].nullable_action));
p = enc->encode_str(p, t, strlen(t));
+ p = enc->encode_str(p, "affinity", 8);
+ p = enc->encode_uint(p, def->fields[i].affinity);
p = enc->encode_str(p, "is_nullable", 11);
p = enc->encode_bool(p, def->fields[i].is_nullable);
p = enc->encode_str(p, "nullable_action", 15);
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 65bba1f..c88ad30 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1182,9 +1182,14 @@ bool
space_is_view(Table *table) {
assert(table != NULL);
uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(table->tnum);
- struct space *space = space_by_id(space_id);
- assert(space != NULL);
- return space->def->opts.is_view;
+ if (space_id > 0) {
+ struct space *space = space_by_id(space_id);
+ assert(space != NULL);
+ return space->def->opts.is_view;
+ } else {
+ assert(table->def != NULL);
+ return table->def->opts.is_view;
+ }
}
struct ExprList *
@@ -1197,23 +1202,13 @@ space_checks_expr_list(uint32_t space_id)
return space->def->opts.checks;
}
-/**
- * Create cursor which will be positioned to the space/index.
- * It makes space lookup and loads pointer to it into register,
- * which is passes to OP_OpenWrite as an argument.
- *
- * @param parse_context Parse context.
- * @param cursor Number of cursor to be created.
- * @param entity_id Encoded space and index ids.
- * @retval address of last opcode.
- */
int
-emit_open_cursor(Parse *parse_context, int cursor, int entity_id)
+emit_open_cursor(struct Parse *parse_context, int cursor, int entity_id)
{
assert(entity_id > 0);
struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(entity_id));
assert(space != NULL);
- Vdbe *vdbe = parse_context->pVdbe;
+ 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);
@@ -1221,6 +1216,22 @@ emit_open_cursor(Parse *parse_context, int cursor, int entity_id)
space_ptr_reg);
}
+int
+sql_emit_open_cursor(struct Parse *parse, int cursor, int index_id, struct space *space)
+{
+ assert(space != NULL);
+ Vdbe *vdbe = parse->pVdbe;
+ int space_ptr_reg = ++parse->nMem;
+ sqlite3VdbeAddOp4(vdbe, OP_LoadPtr, 0, space_ptr_reg, 0, (void*)space,
+ P4_SPACEPTR);
+ struct key_def *def = key_def_dup(space->index[index_id]->def->key_def);
+ if (def == NULL)
+ return 0;
+ return sqlite3VdbeAddOp4(vdbe, OP_OpenWrite, cursor, index_id,
+ space_ptr_reg,
+ (char*)def,
+ P4_KEYDEF);
+}
/*
* Generate code that will increment the schema cookie.
*
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 2b59130..de4e0c1 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -184,7 +184,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
memset(&nc, 0, sizeof(nc));
nc.pParse = parse;
if (tab_list->a[0].pTab == NULL) {
- struct Table *t = malloc(sizeof(struct Table));
+ struct Table *t = calloc(sizeof(struct Table), 1);
if (t == NULL) {
sqlite3OomFault(parse->db);
goto delete_from_cleanup;
@@ -266,7 +266,12 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
/* Extract the primary key for the current row */
if (!is_view) {
for (int i = 0; i < pk_len; i++) {
- sqlite3ExprCodeGetColumnOfTable(v, table->def,
+ struct space_def *def;
+ if (table != NULL)
+ def = table->def;
+ else
+ def = space->def;
+ sqlite3ExprCodeGetColumnOfTable(v, def,
tab_cursor,
pk_def->parts[i].fieldno,
reg_pk + i);
@@ -339,8 +344,18 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
int space_ptr_reg = ++parse->nMem;
sqlite3VdbeAddOp4(v, OP_LoadPtr, 0, space_ptr_reg, 0,
(void *)space, P4_SPACEPTR);
+
+ int tnum;
+ if (table != NULL) {
+ tnum = table->tnum;
+ }
+ else {
+ /* index id is 0 for PK. */
+ tnum = SQLITE_PAGENO_FROM_SPACEID_AND_INDEXID(space->def->id,
+ 0);
+ }
sqlite3VdbeAddOp3(v, OP_OpenWrite, tab_cursor,
- table->tnum, space_ptr_reg);
+ tnum, space_ptr_reg);
struct key_def *def = key_def_dup(pk_def);
if (def == NULL) {
sqlite3OomFault(parse->db);
@@ -446,7 +461,8 @@ sql_generate_row_delete(struct Parse *parse, struct Table *table,
/* If there are any triggers to fire, allocate a range of registers to
* use for the old.* references in the triggers.
*/
- if (sqlite3FkRequired(table, NULL) || trigger_list != NULL) {
+ if (table != NULL &&
+ (sqlite3FkRequired(table, NULL) || trigger_list != NULL)) {
/* Mask of OLD.* columns in use */
/* TODO: Could use temporary registers here. */
uint32_t mask =
@@ -505,7 +521,7 @@ sql_generate_row_delete(struct Parse *parse, struct Table *table,
* of the DELETE statement is to fire the INSTEAD OF
* triggers).
*/
- if (table->pSelect == NULL) {
+ if (table == NULL || table->pSelect == NULL) {
uint8_t p5 = 0;
sqlite3VdbeAddOp2(v, OP_Delete, cursor,
(need_update_count ? OPFLAG_NCHANGE : 0));
@@ -520,16 +536,20 @@ sql_generate_row_delete(struct Parse *parse, struct Table *table,
sqlite3VdbeChangeP5(v, p5);
}
- /* Do any ON CASCADE, SET NULL or SET DEFAULT operations
- * required to handle rows (possibly in other tables) that
- * refer via a foreign key to the row just deleted.
- */
- sqlite3FkActions(parse, table, 0, first_old_reg, 0);
+ if (table != NULL) {
+ /* Do any ON CASCADE, SET NULL or SET DEFAULT
+ * operations required to handle rows (possibly
+ * in other tables) that refer via a foreign
+ * key to the row just deleted.
+ */
- /* Invoke AFTER DELETE trigger programs. */
- sqlite3CodeRowTrigger(parse, trigger_list,
- TK_DELETE, 0, TRIGGER_AFTER, table, first_old_reg,
- onconf, label);
+ sqlite3FkActions(parse, table, 0, first_old_reg, 0);
+
+ /* Invoke AFTER DELETE trigger programs. */
+ sqlite3CodeRowTrigger(parse, trigger_list,
+ TK_DELETE, 0, TRIGGER_AFTER, table,
+ first_old_reg, onconf, label);
+ }
/* Jump here if the row had already been deleted before
* any BEFORE trigger programs were invoked. Or if a trigger program
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 3dbf855..8d0ab3b 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -108,6 +108,39 @@ sqlite3IndexAffinityStr(sqlite3 * db, Index * pIdx)
return pIdx->zColAff;
}
+char *
+sql_index_affinity_str(struct sqlite3 * db, struct index_def *def)
+{
+ char *aff;
+ /* The first time a column affinity string for a particular index is
+ * required, it is allocated and populated here. It is then stored as
+ * a member of the Index structure for subsequent use.
+ *
+ * The column affinity string will eventually be deleted by
+ * sqliteDeleteIndex() when the Index structure itself is cleaned
+ * up.
+ */
+ int nColumn = def->key_def->part_count;
+ aff = (char *)sqlite3DbMallocRaw(0, nColumn + 1);
+ if (aff == NULL) {
+ sqlite3OomFault(db);
+ return 0;
+ }
+ int i;
+ struct space *space = space_cache_find(def->space_id);
+ assert(space != NULL);
+
+ for (i = 0; i < nColumn; i++) {
+ uint32_t x = def->key_def->parts[i].fieldno;
+ aff[i] = space->def->fields[x].affinity;
+ if (aff[i] == AFFINITY_UNDEFINED)
+ aff[i] = 'A';
+ }
+ aff[i] = 0;
+
+ return aff;
+}
+
/*
* Compute the affinity string for table pTab, if it has not already been
* computed. As an optimization, omit trailing AFFINITY_BLOB affinities.
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 943fda9..6ea956d 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2536,6 +2536,8 @@ struct SrcList {
char *zName; /* Name of the table */
char *zAlias; /* The "B" part of a "A AS B" phrase. zName is the "A" */
Table *pTab; /* An SQL table corresponding to zName */
+ /* A temporary hack: need to store eph. space. */
+ struct space *space;
Select *pSelect; /* A SELECT statement used in place of a table name */
int addrFillSub; /* Address of subroutine to manifest a subquery */
int regReturn; /* Register holding return address of addrFillSub */
@@ -3552,8 +3554,36 @@ enum sort_order
sql_index_column_sort_order(Index *idx, uint32_t column);
void sqlite3EndTable(Parse *, Token *, Token *, Select *);
+
+/**
+ * DEPRECATED. All calls to be replaced w/ sql_emit_open_cursor.
+ * Create cursor which will be positioned to the space/index.
+ * It makes space lookup and loads pointer to it into register,
+ * which is passes to OP_OpenWrite as an argument.
+ *
+ * @param parse Parse context.
+ * @param cursor Number of cursor to be created.
+ * @param entity_id Encoded space and index ids.
+ * @retval address of last opcode.
+ */
+int
+emit_open_cursor(struct Parse *parse, int cursor, int entity_id);
+
+/**
+ * Create cursor which will be positioned to the space/index.
+ * It makes space lookup and loads pointer to it into register,
+ * which is passes to OP_OpenWrite as an argument.
+ *
+ * @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.
+ * @retval address of last opcode.
+ */
int
-emit_open_cursor(Parse *, int, int);
+sql_emit_open_cursor(struct Parse *parse, int cursor, int index_id,
+ struct space *space);
int sqlite3ParseUri(const char *, const char *, unsigned int *,
sqlite3_vfs **, char **, char **);
@@ -4085,6 +4115,31 @@ int sqlite3VarintLen(u64 v);
#define putVarint sqlite3PutVarint
const char *sqlite3IndexAffinityStr(sqlite3 *, Index *);
+
+/**
+ * Return a pointer to the column affinity string associated with index
+ * pIdx. A column affinity string has one character for each column in
+ * the table, according to the affinity of the column:
+ *
+ * Character Column affinity
+ * ------------------------------
+ * 'A' BLOB
+ * 'B' TEXT
+ * 'C' NUMERIC
+ * 'D' INTEGER
+ * 'F' REAL
+ *
+ * Memory for the buffer containing the column index affinity string
+ * is managed along with the rest of the Index structure. It will be
+ * released when sqlite3DeleteIndex() is called.
+ *
+ * @param db Database handle.
+ * @param def index_def where from affinity to be extracted.
+ * @retval Affinity string.
+ */
+char *
+sql_index_affinity_str(struct sqlite3 *db, struct index_def *def);
+
void sqlite3TableAffinity(Vdbe *, Table *, int);
char sqlite3CompareAffinity(Expr * pExpr, char aff2);
int sqlite3IndexAffinityOk(Expr * pExpr, char idx_affinity);
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index d312587..14cb23d 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -371,7 +371,10 @@ whereScanInit(WhereScan * pScan, /* The WhereScan object being initialized */
pScan->is_column_seen = false;
if (pIdx) {
int j = iColumn;
- iColumn = pIdx->aiColumn[j];
+ if (j >= pIdx->nColumn)
+ iColumn = -1;
+ else
+ iColumn = pIdx->aiColumn[j];
if (iColumn >= 0) {
char affinity =
pIdx->pTable->def->fields[iColumn].affinity;
@@ -390,6 +393,34 @@ whereScanInit(WhereScan * pScan, /* The WhereScan object being initialized */
return whereScanNext(pScan);
}
+static WhereTerm *
+sql_where_scan_init(struct WhereScan *scan, struct WhereClause *clause,
+ int cursor, int column, uint32_t op_mask,
+ struct space_def *space_def, struct key_def *key_def)
+{
+ scan->pOrigWC = scan->pWC = clause;
+ scan->pIdxExpr = NULL;
+ scan->idxaff = 0;
+ scan->coll = NULL;
+ scan->is_column_seen = false;
+ if (key_def != NULL) {
+ int j = column;
+ column = key_def->parts[j].fieldno;
+ if (column >= 0) {
+ scan->idxaff = space_def->fields[column].affinity;
+ scan->coll = key_def->parts[column].coll;
+ scan->is_column_seen = true;
+ }
+ }
+ scan->opMask = op_mask;
+ scan->k = 0;
+ scan->aiCur[0] = cursor;
+ scan->aiColumn[0] = column;
+ scan->nEquiv = 1;
+ scan->iEquiv = 1;
+ return whereScanNext(scan);
+}
+
/*
* Search for a term in the WHERE clause that is of the form "X <op> <expr>"
* where X is a reference to the iColumn of table iCur or of index pIdx
@@ -441,6 +472,36 @@ sqlite3WhereFindTerm(WhereClause * pWC, /* The WHERE clause to be searched */
return pResult;
}
+WhereTerm *
+sql_where_find_term(WhereClause * pWC, /* The WHERE clause to be searched */
+ int iCur, /* Cursor number of LHS */
+ int iColumn, /* Column number of LHS */
+ Bitmask notReady, /* RHS must not overlap with this mask */
+ u32 op, /* Mask of WO_xx values describing operator */
+ struct space_def *space_def,
+ struct key_def *key_def) /* Must be compatible with this index, if not NULL */
+{
+ WhereTerm *pResult = 0;
+ WhereTerm *p;
+ WhereScan scan;
+
+ p = sql_where_scan_init(&scan, pWC, iCur, iColumn, op, space_def,
+ key_def);
+ op &= WO_EQ;
+ while (p) {
+ if ((p->prereqRight & notReady) == 0) {
+ if (p->prereqRight == 0 && (p->eOperator & op) != 0) {
+ testcase(p->eOperator & WO_IS);
+ return p;
+ }
+ if (pResult == 0)
+ pResult = p;
+ }
+ p = whereScanNext(&scan);
+ }
+ return pResult;
+}
+
/*
* This function searches pList for an entry that matches the iCol-th column
* of index pIdx.
@@ -1703,6 +1764,7 @@ whereLoopInit(WhereLoop * p)
p->nLTerm = 0;
p->nLSlot = ArraySize(p->aLTermSpace);
p->wsFlags = 0;
+ p->index_def = NULL;
}
/*
@@ -3366,7 +3428,7 @@ wherePathSatisfiesOrderBy(WhereInfo * pWInfo, /* The WHERE clause */
/* Get the column number in the table (iColumn) and sort order
* (revIdx) for the j-th column of the index.
*/
- if (pIndex) {
+ if (pIndex && j < pIndex->nColumn) {
iColumn = pIndex->aiColumn[j];
revIdx = sql_index_column_sort_order(pIndex,
j);
@@ -4046,7 +4108,7 @@ wherePathSolver(WhereInfo * pWInfo, LogEst nRowEst)
* general-purpose query planner.
*/
static int
-whereShortCut(WhereLoopBuilder * pBuilder)
+sql_where_shortcut(struct WhereLoopBuilder *builder)
{
WhereInfo *pWInfo;
struct SrcList_item *pItem;
@@ -4055,20 +4117,20 @@ whereShortCut(WhereLoopBuilder * pBuilder)
WhereLoop *pLoop;
int iCur;
int j;
- Table *pTab;
- Index *pIdx;
- pWInfo = pBuilder->pWInfo;
+ pWInfo = builder->pWInfo;
if (pWInfo->wctrlFlags & WHERE_OR_SUBCLAUSE)
return 0;
assert(pWInfo->pTabList->nSrc >= 1);
pItem = pWInfo->pTabList->a;
- pTab = pItem->pTab;
+ struct space_def *space_def = pItem->pTab->def;
+ assert(space_def != NULL);
+
if (pItem->fg.isIndexedBy)
return 0;
iCur = pItem->iCursor;
pWC = &pWInfo->sWC;
- pLoop = pBuilder->pNew;
+ pLoop = builder->pNew;
pLoop->wsFlags = 0;
pLoop->nSkip = 0;
pTerm = sqlite3WhereFindTerm(pWC, iCur, -1, 0, WO_EQ, 0);
@@ -4080,34 +4142,74 @@ whereShortCut(WhereLoopBuilder * pBuilder)
/* TUNING: Cost of a PK lookup is 10 */
pLoop->rRun = 33; /* 33==sqlite3LogEst(10) */
} else {
- for (pIdx = pTab->pIndex; pIdx; pIdx = pIdx->pNext) {
+ struct space *space = space_cache_find(space_def->id);
+ if (space != NULL) {
+ for (uint32_t i = 0; i < space->index_count; ++i) {
+ struct index_def *idx_def;
+ idx_def = space->index[i]->def;
+ int opMask;
+ int nIdxCol = idx_def->key_def->part_count;
+ assert(pLoop->aLTermSpace == pLoop->aLTerm);
+ if (!idx_def->opts.is_unique
+ /* || pIdx->pPartIdxWhere != 0 */
+ || nIdxCol > ArraySize(pLoop->aLTermSpace)
+ )
+ continue;
+ opMask = WO_EQ;
+ for (j = 0; j < nIdxCol; j++) {
+ pTerm = sql_where_find_term(pWC, iCur,
+ j, 0,
+ opMask,
+ space_def,
+ idx_def->
+ key_def);
+ if (pTerm == 0)
+ break;
+ testcase(pTerm->eOperator & WO_IS);
+ pLoop->aLTerm[j] = pTerm;
+ }
+ if (j != nIdxCol)
+ continue;
+ pLoop->wsFlags = WHERE_COLUMN_EQ |
+ WHERE_ONEROW | WHERE_INDEXED |
+ WHERE_IDX_ONLY;
+ pLoop->nLTerm = j;
+ pLoop->nEq = j;
+ pLoop->pIndex = NULL;
+ pLoop->index_def = idx_def;
+ /* TUNING: Cost of a unique index lookup is 15 */
+ pLoop->rRun = 39; /* 39==sqlite3LogEst(15) */
+ break;
+ }
+ } else {
+ /* Space is ephemeral. */
+ assert(space_def->id == 0);
int opMask;
- int nIdxCol = index_column_count(pIdx);
+ int nIdxCol = space_def->field_count;
assert(pLoop->aLTermSpace == pLoop->aLTerm);
- if (!index_is_unique(pIdx)
- || pIdx->pPartIdxWhere != 0
- || nIdxCol > ArraySize(pLoop->aLTermSpace)
- )
- continue;
+ if ( nIdxCol > ArraySize(pLoop->aLTermSpace))
+ return 0;
opMask = WO_EQ;
for (j = 0; j < nIdxCol; j++) {
- pTerm =
- sqlite3WhereFindTerm(pWC, iCur, j, 0,
- opMask, pIdx);
- if (pTerm == 0)
+ pTerm = sql_where_find_term(pWC, iCur,
+ j, 0,
+ opMask,
+ space_def,
+ NULL);
+ if (pTerm == NULL)
break;
pLoop->aLTerm[j] = pTerm;
}
if (j != nIdxCol)
- continue;
+ return 0;
pLoop->wsFlags = WHERE_COLUMN_EQ | WHERE_ONEROW |
WHERE_INDEXED | WHERE_IDX_ONLY;
pLoop->nLTerm = j;
pLoop->nEq = j;
- pLoop->pIndex = pIdx;
+ pLoop->pIndex = NULL;
+ pLoop->index_def = NULL;
/* TUNING: Cost of a unique index lookup is 15 */
pLoop->rRun = 39; /* 39==sqlite3LogEst(15) */
- break;
}
}
if (pLoop->wsFlags) {
@@ -4420,7 +4522,7 @@ sqlite3WhereBegin(Parse * pParse, /* The parser context */
}
#endif
- if (nTabList != 1 || whereShortCut(&sWLB) == 0) {
+ if (nTabList != 1 || sql_where_shortcut(&sWLB) == 0) {
rc = whereLoopAddAll(&sWLB);
if (rc)
goto whereBeginError;
@@ -4583,13 +4685,31 @@ sqlite3WhereBegin(Parse * pParse, /* The parser context */
}
if (pLoop->wsFlags & WHERE_INDEXED) {
Index *pIx = pLoop->pIndex;
+ struct index_def *idx_def = pLoop->index_def;
+ struct space *space = space_cache_find(pTabItem->pTab->def->id);
int iIndexCur;
int op = OP_OpenRead;
/* iAuxArg is always set if to a positive value if ONEPASS is possible */
assert(iAuxArg != 0
|| (pWInfo->
wctrlFlags & WHERE_ONEPASS_DESIRED) == 0);
- if (IsPrimaryKeyIndex(pIx)
+ /* Check if index is primary. Either of
+ * points should be true:
+ * 1. struct Index is non-NULL and is
+ * primary
+ * 2. idx_def is non-NULL and it is
+ * primary
+ * 3. (goal of this comment) both pIx and
+ * idx_def are NULL in which case it is
+ * ephemeral table, but not in Tnt sense.
+ * It is something w/ defined space_def
+ * and nothing else. Skip such loops.
+ */
+ if (idx_def == NULL && pIx == NULL) continue;
+ bool is_primary = (pIx != NULL && IsPrimaryKeyIndex(pIx)) ||
+ (idx_def != NULL && (idx_def->iid == 0)) |
+ (idx_def == NULL && pIx == NULL);
+ if (is_primary
&& (wctrlFlags & WHERE_OR_SUBCLAUSE) != 0) {
/* This is one term of an OR-optimization using
* the PRIMARY KEY. No need for a separate index
@@ -4597,13 +4717,31 @@ sqlite3WhereBegin(Parse * pParse, /* The parser context */
iIndexCur = pLevel->iTabCur;
op = 0;
} else if (pWInfo->eOnePass != ONEPASS_OFF) {
- Index *pJ = pTabItem->pTab->pIndex;
- iIndexCur = iAuxArg;
- assert(wctrlFlags & WHERE_ONEPASS_DESIRED);
- while (ALWAYS(pJ) && pJ != pIx) {
- iIndexCur++;
- pJ = pJ->pNext;
+ if (pIx != NULL) {
+ Index *pJ = pTabItem->pTab->pIndex;
+ iIndexCur = iAuxArg;
+ assert(wctrlFlags &
+ WHERE_ONEPASS_DESIRED);
+ while (ALWAYS(pJ) && pJ != pIx) {
+ iIndexCur++;
+ pJ = pJ->pNext;
+ }
+ } else {
+ if (space != NULL) {
+ for(uint32_t i = 0;
+ i < space->index_count;
+ ++i) {
+ if (space->index[i]->def ==
+ idx_def) {
+ iIndexCur = iAuxArg + i;
+ break;
+ }
+ }
+ } else {
+ iIndexCur = iAuxArg;
+ }
}
+ assert(wctrlFlags & WHERE_ONEPASS_DESIRED);
op = OP_OpenWrite;
pWInfo->aiCurOnePass[1] = iIndexCur;
} else if (iAuxArg
@@ -4614,11 +4752,17 @@ sqlite3WhereBegin(Parse * pParse, /* The parser context */
iIndexCur = pParse->nTab++;
}
pLevel->iIdxCur = iIndexCur;
- assert(pIx->pSchema == pTab->pSchema);
assert(iIndexCur >= 0);
if (op) {
- emit_open_cursor(pParse, iIndexCur, pIx->tnum);
- sql_vdbe_set_p4_key_def(pParse, pIx);
+ if (pIx != NULL) {
+ emit_open_cursor(pParse, iIndexCur,
+ pIx->tnum);
+ sql_vdbe_set_p4_key_def(pParse, pIx);
+ } else {
+ sql_emit_open_cursor(pParse, iIndexCur,
+ idx_def->iid,
+ space);
+ }
if ((pLoop->wsFlags & WHERE_CONSTRAINT) != 0
&& (pLoop->
wsFlags & (WHERE_COLUMN_RANGE |
@@ -4627,7 +4771,10 @@ sqlite3WhereBegin(Parse * pParse, /* The parser context */
wctrlFlags & WHERE_ORDERBY_MIN) == 0) {
sqlite3VdbeChangeP5(v, OPFLAG_SEEKEQ); /* Hint to COMDB2 */
}
- VdbeComment((v, "%s", pIx->zName));
+ if (pIx != NULL)
+ VdbeComment((v, "%s", pIx->zName));
+ else
+ VdbeComment((v, "%s", idx_def->name));
#ifdef SQLITE_ENABLE_COLUMN_USED_MASK
{
u64 colUsed = 0;
@@ -4809,7 +4956,6 @@ sqlite3WhereEnd(WhereInfo * pWInfo)
for (i = 0, pLevel = pWInfo->a; i < pWInfo->nLevel; i++, pLevel++) {
int k, last;
VdbeOp *pOp;
- Index *pIdx = 0;
struct SrcList_item *pTabItem = &pTabList->a[pLevel->iFrom];
Table *pTab MAYBE_UNUSED = pTabItem->pTab;
assert(pTab != 0);
@@ -4836,12 +4982,15 @@ sqlite3WhereEnd(WhereInfo * pWInfo)
* that reference the table and converts them into opcodes that
* reference the index.
*/
+ Index *pIdx = NULL;
+ struct index_def *def = NULL;
if (pLoop->wsFlags & (WHERE_INDEXED | WHERE_IDX_ONLY)) {
pIdx = pLoop->pIndex;
+ def = pLoop->index_def;
} else if (pLoop->wsFlags & WHERE_MULTI_OR) {
pIdx = pLevel->u.pCovidx;
}
- if (pIdx && !db->mallocFailed) {
+ if ((pIdx != NULL || def != NULL) && !db->mallocFailed) {
last = sqlite3VdbeCurrentAddr(v);
k = pLevel->addrBody;
pOp = sqlite3VdbeGetOp(v, k);
@@ -4850,7 +4999,8 @@ sqlite3WhereEnd(WhereInfo * pWInfo)
continue;
if (pOp->opcode == OP_Column) {
int x = pOp->p2;
- assert(pIdx->pTable == pTab);
+ assert(pIdx == NULL ||
+ pIdx->pTable == pTab);
if (x >= 0) {
pOp->p2 = x;
pOp->p1 = pLevel->iIdxCur;
diff --git a/src/box/sql/whereInt.h b/src/box/sql/whereInt.h
index 1303365..548cbcb 100644
--- a/src/box/sql/whereInt.h
+++ b/src/box/sql/whereInt.h
@@ -141,6 +141,8 @@ struct WhereLoop {
u16 nBtm; /* Size of BTM vector */
u16 nTop; /* Size of TOP vector */
Index *pIndex; /* Index used, or NULL */
+ /** Index definition, if there's no pIndex. */
+ struct index_def *index_def;
u32 wsFlags; /* WHERE_* flags describing the plan */
u16 nLTerm; /* Number of entries in aLTerm[] */
u16 nSkip; /* Number of NULL aLTerm[] entries */
diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index bf2a2a2..1ce1db0 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -38,6 +38,7 @@
* that actually generate the bulk of the WHERE loop code. The original where.c
* file retains the code that does query planning and analysis.
*/
+#include "box/schema.h"
#include "sqliteInt.h"
#include "whereInt.h"
@@ -62,6 +63,7 @@ explainIndexColumnName(Index * pIdx, int i)
static void
explainAppendTerm(StrAccum * pStr, /* The text expression being built */
Index * pIdx, /* Index to read column names from */
+ struct index_def *def,
int nTerm, /* Number of terms */
int iTerm, /* Zero-based index of first term. */
int bAnd, /* Non-zero to append " AND " */
@@ -78,9 +80,16 @@ explainAppendTerm(StrAccum * pStr, /* The text expression being built */
for (i = 0; i < nTerm; i++) {
if (i)
sqlite3StrAccumAppend(pStr, ",", 1);
- sqlite3StrAccumAppendAll(pStr,
- explainIndexColumnName(pIdx,
- iTerm + i));
+ const char *name;
+ if (pIdx != NULL) {
+ name = explainIndexColumnName(pIdx, iTerm + i);
+ } else {
+ assert(def != NULL);
+ struct space *space = space_cache_find(def->space_id);
+ assert(space != NULL);
+ name = space->def->fields[i].name;
+ }
+ sqlite3StrAccumAppendAll(pStr, name);
}
if (nTerm > 1)
sqlite3StrAccumAppend(pStr, ")", 1);
@@ -116,16 +125,26 @@ static void
explainIndexRange(StrAccum * pStr, WhereLoop * pLoop)
{
Index *pIndex = pLoop->pIndex;
+ struct index_def *def = pLoop->index_def;
u16 nEq = pLoop->nEq;
u16 nSkip = pLoop->nSkip;
int i, j;
+ assert(pIndex != NULL || def != NULL);
+
if (nEq == 0
&& (pLoop->wsFlags & (WHERE_BTM_LIMIT | WHERE_TOP_LIMIT)) == 0)
return;
sqlite3StrAccumAppend(pStr, " (", 2);
for (i = 0; i < nEq; i++) {
- const char *z = explainIndexColumnName(pIndex, i);
+ const char *z;
+ if (pIndex != NULL) {
+ z = explainIndexColumnName(pIndex, i);
+ } else {
+ struct space *space = space_cache_find(def->space_id);
+ assert(space != NULL);
+ z = space->def->fields[i].name;
+ }
if (i)
sqlite3StrAccumAppend(pStr, " AND ", 5);
sqlite3XPrintf(pStr, i >= nSkip ? "%s=?" : "ANY(%s)", z);
@@ -133,11 +152,11 @@ explainIndexRange(StrAccum * pStr, WhereLoop * pLoop)
j = i;
if (pLoop->wsFlags & WHERE_BTM_LIMIT) {
- explainAppendTerm(pStr, pIndex, pLoop->nBtm, j, i, ">");
+ explainAppendTerm(pStr, pIndex, def, pLoop->nBtm, j, i, ">");
i = 1;
}
if (pLoop->wsFlags & WHERE_TOP_LIMIT) {
- explainAppendTerm(pStr, pIndex, pLoop->nTop, j, i, "<");
+ explainAppendTerm(pStr, pIndex, def, pLoop->nTop, j, i, "<");
}
sqlite3StrAccumAppend(pStr, ")", 1);
}
@@ -199,13 +218,15 @@ sqlite3WhereExplainOneScan(Parse * pParse, /* Parse context */
}
if ((flags & WHERE_IPK) == 0) {
const char *zFmt = 0;
- Index *pIdx;
+ Index *pIdx = pLoop->pIndex;
+ struct index_def *idx_def = pLoop->index_def;
+ if (pIdx == NULL && idx_def == NULL) return 0;
- assert(pLoop->pIndex != 0);
- pIdx = pLoop->pIndex;
+ assert(pIdx != NULL || idx_def != NULL);
assert(!(flags & WHERE_AUTO_INDEX)
|| (flags & WHERE_IDX_ONLY));
- if (IsPrimaryKeyIndex(pIdx)) {
+ if ((pIdx != NULL && IsPrimaryKeyIndex(pIdx)) ||
+ (idx_def != NULL && idx_def->iid == 0)) {
if (isSearch) {
zFmt = "PRIMARY KEY";
}
@@ -220,7 +241,12 @@ sqlite3WhereExplainOneScan(Parse * pParse, /* Parse context */
}
if (zFmt) {
sqlite3StrAccumAppend(&str, " USING ", 7);
- sqlite3XPrintf(&str, zFmt, pIdx->zName);
+ if (pIdx != NULL)
+ sqlite3XPrintf(&str, zFmt, pIdx->zName);
+ else if (idx_def != NULL)
+ sqlite3XPrintf(&str, zFmt, idx_def->name);
+ else
+ sqlite3XPrintf(&str, zFmt, "EPHEMERAL INDEX");
explainIndexRange(&str, pLoop);
}
} else if ((flags & WHERE_IPK) != 0
@@ -675,20 +701,19 @@ codeAllEqualityTerms(Parse * pParse, /* Parsing context */
u16 nEq; /* The number of == or IN constraints to code */
u16 nSkip; /* Number of left-most columns to skip */
Vdbe *v = pParse->pVdbe; /* The vm under construction */
- Index *pIdx; /* The index being used for this loop */
WhereTerm *pTerm; /* A single constraint term */
WhereLoop *pLoop; /* The WhereLoop object */
int j; /* Loop counter */
int regBase; /* Base register */
int nReg; /* Number of registers to allocate */
- char *zAff; /* Affinity string to return */
/* This module is only called on query plans that use an index. */
pLoop = pLevel->pWLoop;
nEq = pLoop->nEq;
nSkip = pLoop->nSkip;
- pIdx = pLoop->pIndex;
- assert(pIdx != 0);
+ struct Index *pIdx = pLoop->pIndex;
+ struct index_def *idx_def = pLoop->index_def;
+ assert(pIdx != NULL || idx_def != NULL);
/* Figure out how many memory cells we will need then allocate them.
*/
@@ -696,9 +721,13 @@ codeAllEqualityTerms(Parse * pParse, /* Parsing context */
nReg = pLoop->nEq + nExtraReg;
pParse->nMem += nReg;
- zAff =
- sqlite3DbStrDup(pParse->db,
- sqlite3IndexAffinityStr(pParse->db, pIdx));
+ char *zAff;
+ if (pIdx != NULL) {
+ zAff = sqlite3DbStrDup(pParse->db,
+ sqlite3IndexAffinityStr(pParse->db, pIdx));
+ } else {
+ zAff = sql_index_affinity_str(pParse->db, idx_def);
+ }
assert(zAff != 0 || pParse->db->mallocFailed);
if (nSkip) {
@@ -1214,7 +1243,6 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about t
int endEq; /* True if range end uses ==, >= or <= */
int start_constraints; /* Start of range is constrained */
int nConstraint; /* Number of constraint terms */
- Index *pIdx; /* The index we will be using */
int iIdxCur; /* The VDBE cursor for the index */
int nExtraReg = 0; /* Number of extra registers needed */
int op; /* Instruction opcode */
@@ -1227,7 +1255,9 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about t
* to integer type, used for IPK.
*/
- pIdx = pLoop->pIndex;
+ struct Index *pIdx = pLoop->pIndex;
+ struct index_def *idx_def = pLoop->index_def;
+ assert(pIdx != NULL || idx_def != NULL);
iIdxCur = pLevel->iIdxCur;
assert(nEq >= pLoop->nSkip);
@@ -1242,7 +1272,11 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about t
assert(pWInfo->pOrderBy == 0
|| pWInfo->pOrderBy->nExpr == 1
|| (pWInfo->wctrlFlags & WHERE_ORDERBY_MIN) == 0);
- int nIdxCol = index_column_count(pIdx);
+ int nIdxCol;
+ if (pIdx != NULL)
+ nIdxCol = index_column_count(pIdx);
+ else
+ nIdxCol = idx_def->key_def->part_count;
if ((pWInfo->wctrlFlags & WHERE_ORDERBY_MIN) != 0
&& pWInfo->nOBSat > 0 && (nIdxCol > nEq)) {
j = pIdx->aiColumn[nEq];
@@ -1379,11 +1413,34 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about t
startEq = 0;
start_constraints = 1;
}
- struct Index *pk = sqlite3PrimaryKeyIndex(pIdx->pTable);
- assert(pk);
- int nPkCol = index_column_count(pk);
- char affinity =
- pIdx->pTable->def->fields[pk->aiColumn[0]].affinity;
+ struct Index *pk = NULL;
+ struct index_def *idx_pk = NULL;
+ char affinity;
+ if (pIdx == NULL) {
+ struct space *space = space_cache_find(idx_def->space_id);
+ assert(space != NULL);
+ idx_pk = space->index[0]->def;
+ int fieldno = idx_pk->key_def->parts[0].fieldno;
+ affinity = space->def->fields[fieldno].affinity;
+ if (affinity == AFFINITY_UNDEFINED) {
+ if (idx_pk->key_def->part_count == 1 &&
+ space->def->fields[fieldno].type ==
+ FIELD_TYPE_INTEGER)
+ affinity = AFFINITY_INTEGER;
+ else
+ affinity = AFFINITY_BLOB;
+ }
+ } else {
+ pk = sqlite3PrimaryKeyIndex(pIdx->pTable);
+ affinity =
+ pIdx->pTable->def->fields[pk->aiColumn[0]].affinity;
+ }
+
+ int nPkCol;
+ if (pk != NULL)
+ nPkCol = index_column_count(pk);
+ else
+ nPkCol = idx_pk->key_def->part_count;
if (nPkCol == 1 && affinity == AFFINITY_INTEGER) {
/* Right now INTEGER PRIMARY KEY is the only option to
* get Tarantool's INTEGER column type. Need special handling
@@ -1392,7 +1449,11 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo, /* Complete information about t
*/
int limit = pRangeStart == NULL ? nEq : nEq + 1;
for (int i = 0; i < limit; i++) {
- if (pIdx->aiColumn[i] == pk->aiColumn[0]) {
+ if ((pIdx != NULL && pIdx->aiColumn[i] ==
+ pk->aiColumn[0]) ||
+ (idx_pk != NULL &&
+ idx_def->key_def->parts[i].fieldno ==
+ idx_pk->key_def->parts[0].fieldno)) {
/* Here: we know for sure that table has INTEGER
PRIMARY KEY, single column, and Index we're
trying to use for scan contains this column. */
diff --git a/test/sql-tap/delete1.test.lua b/test/sql-tap/delete1.test.lua
index 810ca8a..d6d4762 100755
--- a/test/sql-tap/delete1.test.lua
+++ b/test/sql-tap/delete1.test.lua
@@ -1,6 +1,6 @@
#!/usr/bin/env tarantool
test = require("sqltester")
-test:plan(9)
+test:plan(10)
--!./tcltestrunner.lua
-- ["set","testdir",[["file","dirname",["argv0"]]]]
@@ -132,18 +132,30 @@ test:do_test(
})
-- Tests for data dictionary integration.
-s = box.schema.create_space('t')
-i = s:create_index('i', {parts={1, 'unsigned'}})
+format = {}
+format[1] = {name = 'id', type = 'scalar'}
+format[2] = {name = 'f', type = 'scalar'}
+s = box.schema.create_space('t', {format = format})
+i = s:create_index('i', {parts = {1, 'scalar'}})
+
test:do_test(
"delete1-6.0",
function()
- s:replace({1})
- s:replace({2})
- s:replace({3})
+ s:replace({1, 4})
+ s:replace({2, 5})
+ s:replace({3, 6})
return s:count()
end,
3)
+test:do_test(
+ "delete1-6.1.1",
+ function()
+ box.sql.execute([[delete from "t" where "id"=2]])
+ return s:count()
+ end,
+ 2)
+
test:do_test(
"delete1-6.1",
function()
--
2.16.2
^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 1/3] sql: fetch primary index for affinity only
2018-06-01 15:16 ` [tarantool-patches] [PATCH 1/3] sql: fetch primary index for affinity only Kirill Yukhin
@ 2018-06-01 18:00 ` Vladislav Shpilevoy
2018-06-07 12:03 ` Kirill Yukhin
0 siblings, 1 reply; 19+ messages in thread
From: Vladislav Shpilevoy @ 2018-06-01 18:00 UTC (permalink / raw)
To: Kirill Yukhin; +Cc: tarantool-patches
Hello. Thanks for the patch! See 2 comments below.
On 01/06/2018 18:16, Kirill Yukhin wrote:
> This small patch removes usages of primary index throughout
> code sql_table_delete_from, limiting use to fetching of
> affinities only. We cannot use space_def here, since primary
> index might contain calculated columns.
1. What are calculated columns?
>
> Part of #3235
> ---
> src/box/sql/delete.c | 54 ++++++++++++++++++++++++++++++++++++----------------
> 1 file changed, 38 insertions(+), 16 deletions(-)
>
> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> index ddad54b..28713c8 100644
> --- a/src/box/sql/delete.c
> +++ b/src/box/sql/delete.c
> @@ -183,6 +183,16 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
> struct NameContext nc;
> memset(&nc, 0, sizeof(nc));
> nc.pParse = parse;
> + if (tab_list->a[0].pTab == NULL) {
> + struct Table *t = malloc(sizeof(struct Table));
> + if (t == NULL) {
> + sqlite3OomFault(parse->db);
> + goto delete_from_cleanup;
> + }
> + assert(space != NULL);
> + t->def = space->def;
> + tab_list->a[0].pTab = t;
2. Why can not you declare struct Table on the stack at the top of this
function, and here use its address? Can this pTab be used out of
sql_table_delete_from()? And I do not see where do you delete this
table.
> + }
> nc.pSrcList = tab_list;
> if (sqlite3ResolveExprNames(&nc, where))
> goto delete_from_cleanup;
^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 2/3] sql: remove expressions from SQL indexes
2018-06-01 15:16 ` [tarantool-patches] [PATCH 2/3] sql: remove expressions from SQL indexes Kirill Yukhin
@ 2018-06-01 18:00 ` Vladislav Shpilevoy
0 siblings, 0 replies; 19+ messages in thread
From: Vladislav Shpilevoy @ 2018-06-01 18:00 UTC (permalink / raw)
To: tarantool-patches, Kirill Yukhin
Thanks for the patch! See 2 comments below.
On 01/06/2018 18:16, Kirill Yukhin wrote:
> Legacy SQL FE was able to create indexes w/ expressions.
> Tarantool will employ diofferenct scheme to implement
1. diofferenct?
> functional indexes, thus code handling it in SQL FE is
> dead and redundant. Remove it.
>
> Part of #3235
2. I pushed my review fixes. Please, consider them as squash if ok.
> ---
> src/box/sql/build.c | 22 ++++++---------
> src/box/sql/delete.c | 3 +--
> src/box/sql/expr.c | 12 ++-------
> src/box/sql/insert.c | 72 +++++++++++++++----------------------------------
> src/box/sql/sqliteInt.h | 6 -----
> src/box/sql/where.c | 27 ++-----------------
> src/box/sql/wherecode.c | 13 +++------
> src/box/sql/whereexpr.c | 36 +++----------------------
> 8 files changed, 42 insertions(+), 149 deletions(-)
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 3/3] sql: implement point where for DELETE stmts
2018-06-01 15:16 ` [tarantool-patches] [PATCH 3/3] sql: implement point where for DELETE stmts Kirill Yukhin
@ 2018-06-01 18:00 ` Vladislav Shpilevoy
2018-06-18 2:11 ` n.pettik
0 siblings, 1 reply; 19+ messages in thread
From: Vladislav Shpilevoy @ 2018-06-01 18:00 UTC (permalink / raw)
To: Kirill Yukhin; +Cc: tarantool-patches
Thanks for the patch! See 17 comments below. I have not finished
the review. I continue, when you would fix this relatively major
major comments.
On 01/06/2018 18:16, Kirill Yukhin wrote:
> This patch implements support of SQL's DELETE statemets
> which a accompanied by point WHERE point constraints.
> This patch doesn't support any kinds of nested selects
> or JOINs.
>
> Part of #3235
> ---
> src/box/field_def.c | 1 +
> src/box/sql.c | 4 +-
> src/box/sql/build.c | 41 +++++---
> src/box/sql/delete.c | 48 ++++++---
> src/box/sql/insert.c | 33 +++++++
> src/box/sql/sqliteInt.h | 57 ++++++++++-
> src/box/sql/where.c | 224 +++++++++++++++++++++++++++++++++++-------
> src/box/sql/whereInt.h | 2 +
> src/box/sql/wherecode.c | 115 +++++++++++++++++-----
> test/sql-tap/delete1.test.lua | 24 +++--
> 10 files changed, 448 insertions(+), 101 deletions(-)
>
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 65bba1f..c88ad30 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1182,9 +1182,14 @@ bool
> space_is_view(Table *table) {
> assert(table != NULL);
> uint32_t space_id = SQLITE_PAGENO_TO_SPACEID(table->tnum);
> - struct space *space = space_by_id(space_id);
> - assert(space != NULL);
> - return space->def->opts.is_view;
> + if (space_id > 0) {
> + struct space *space = space_by_id(space_id);
> + assert(space != NULL);
> + return space->def->opts.is_view;
> + } else {
> + assert(table->def != NULL);
> + return table->def->opts.is_view;
> + }
1. I have remove space_is_view in a separate commit. Please, consider and
perhaps squash.
> }
>
> struct ExprList *
> @@ -1221,6 +1216,22 @@ emit_open_cursor(Parse *parse_context, int cursor, int entity_id)
> space_ptr_reg);
> }
>
> +int
> +sql_emit_open_cursor(struct Parse *parse, int cursor, int index_id, struct space *space)
2. Few lines above I see emit_open_cursor. Please, try to remove one of them.
At the worst case you can just inline emit_open_cursor in the single place of
its usage, and rename sql_emit_open_cursor to emit_open_cursor.
> +{
> + assert(space != NULL);
> + Vdbe *vdbe = parse->pVdbe;
> + int space_ptr_reg = ++parse->nMem;
> + sqlite3VdbeAddOp4(vdbe, OP_LoadPtr, 0, space_ptr_reg, 0, (void*)space,
> + P4_SPACEPTR);
> + struct key_def *def = key_def_dup(space->index[index_id]->def->key_def);
> + if (def == NULL)
> + return 0;
> + return sqlite3VdbeAddOp4(vdbe, OP_OpenWrite, cursor, index_id,
> + space_ptr_reg,
> + (char*)def,
> + P4_KEYDEF);
3. Looks like the arguments can fit in 2 lines instead of 4.
> +}
> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> index 2b59130..de4e0c1 100644
> --- a/src/box/sql/delete.c
> +++ b/src/box/sql/delete.c
> @@ -184,7 +184,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
> memset(&nc, 0, sizeof(nc));
> nc.pParse = parse;
> if (tab_list->a[0].pTab == NULL) {
> - struct Table *t = malloc(sizeof(struct Table));
> + struct Table *t = calloc(sizeof(struct Table), 1);
4. It must the part of the previous patch.
> if (t == NULL) {
> sqlite3OomFault(parse->db);
> goto delete_from_cleanup;
> @@ -266,7 +266,12 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
> /* Extract the primary key for the current row */
> if (!is_view) {
> for (int i = 0; i < pk_len; i++) {
> - sqlite3ExprCodeGetColumnOfTable(v, table->def,
> + struct space_def *def;
> + if (table != NULL)
> + def = table->def;
> + else
> + def = space->def;
5. Why not always space->def?
> + sqlite3ExprCodeGetColumnOfTable(v, def,
> tab_cursor,
> pk_def->parts[i].fieldno,
> reg_pk + i);
> @@ -339,8 +344,18 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
> int space_ptr_reg = ++parse->nMem;
> sqlite3VdbeAddOp4(v, OP_LoadPtr, 0, space_ptr_reg, 0,
> (void *)space, P4_SPACEPTR);
> +
> + int tnum;
> + if (table != NULL) {
> + tnum = table->tnum;
> + }
> + else {
> + /* index id is 0 for PK. */
> + tnum = SQLITE_PAGENO_FROM_SPACEID_AND_INDEXID(space->def->id,
> + 0);
> + }
6. Why not always the second version?
> sqlite3VdbeAddOp3(v, OP_OpenWrite, tab_cursor,
> - table->tnum, space_ptr_reg);
> + tnum, space_ptr_reg);
> struct key_def *def = key_def_dup(pk_def);
> if (def == NULL) {
> sqlite3OomFault(parse->db);
> @@ -505,7 +521,7 @@ sql_generate_row_delete(struct Parse *parse, struct Table *table,
> * of the DELETE statement is to fire the INSTEAD OF
> * triggers).
> */
> - if (table->pSelect == NULL) {
> + if (table == NULL || table->pSelect == NULL) {
7. Is the same as space->def->opts.is_view.
> uint8_t p5 = 0;
> sqlite3VdbeAddOp2(v, OP_Delete, cursor,
> (need_update_count ? OPFLAG_NCHANGE : 0));
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 3dbf855..8d0ab3b 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -108,6 +108,39 @@ sqlite3IndexAffinityStr(sqlite3 * db, Index * pIdx)
> return pIdx->zColAff;
> }
>
> +char *
> +sql_index_affinity_str(struct sqlite3 * db, struct index_def *def)
> +{
> + char *aff;
> + /* The first time a column affinity string for a particular index is
> + * required, it is allocated and populated here. It is then stored as
> + * a member of the Index structure for subsequent use.
8. No, it is not populated.
> + *
> + * The column affinity string will eventually be deleted by
> + * sqliteDeleteIndex() when the Index structure itself is cleaned
> + * up.
> + */
> + int nColumn = def->key_def->part_count;
> + aff = (char *)sqlite3DbMallocRaw(0, nColumn + 1);
9. Why do you need to declare 'char *aff;' separately above?
> + if (aff == NULL) {
> + sqlite3OomFault(db);
> + return 0;
> + }
> + int i;
10. Can be part of 'for'.
> + struct space *space = space_cache_find(def->space_id);
> + assert(space != NULL);
> +
> + for (i = 0; i < nColumn; i++) {
> + uint32_t x = def->key_def->parts[i].fieldno;
> + aff[i] = space->def->fields[x].affinity;
> + if (aff[i] == AFFINITY_UNDEFINED)
> + aff[i] = 'A';
> + }
> + aff[i] = 0;
> +
> + return aff;
> +}
> +
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 943fda9..6ea956d 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -2536,6 +2536,8 @@ struct SrcList {
> char *zName; /* Name of the table */
> char *zAlias; /* The "B" part of a "A AS B" phrase. zName is the "A" */
> Table *pTab; /* An SQL table corresponding to zName */
> + /* A temporary hack: need to store eph. space. */
> + struct space *space;
11. ??? It is unused.
> Select *pSelect; /* A SELECT statement used in place of a table name */
> int addrFillSub; /* Address of subroutine to manifest a subquery */
> int regReturn; /* Register holding return address of addrFillSub */
> diff --git a/src/box/sql/where.c b/src/box/sql/where.c
> index d312587..14cb23d 100644
> --- a/src/box/sql/where.c
> +++ b/src/box/sql/where.c
> @@ -371,7 +371,10 @@ whereScanInit(WhereScan * pScan, /* The WhereScan object being initialized */
> pScan->is_column_seen = false;
> if (pIdx) {
> int j = iColumn;
> - iColumn = pIdx->aiColumn[j];
> + if (j >= pIdx->nColumn)
> + iColumn = -1;
> + else
> + iColumn = pIdx->aiColumn[j];
12. How can j can be > nColumn?
> if (iColumn >= 0) {
> char affinity =
> pIdx->pTable->def->fields[iColumn].affinity;
> @@ -441,6 +472,36 @@ sqlite3WhereFindTerm(WhereClause * pWC, /* The WHERE clause to be searched */
> return pResult;
> }
>
> +WhereTerm *
> +sql_where_find_term(WhereClause * pWC, /* The WHERE clause to be searched */
> + int iCur, /* Cursor number of LHS */
> + int iColumn, /* Column number of LHS */
> + Bitmask notReady, /* RHS must not overlap with this mask */
> + u32 op, /* Mask of WO_xx values describing operator */
> + struct space_def *space_def,
> + struct key_def *key_def) /* Must be compatible with this index, if not NULL */
> +{
> + WhereTerm *pResult = 0;
> + WhereTerm *p;
> + WhereScan scan;
> +
> + p = sql_where_scan_init(&scan, pWC, iCur, iColumn, op, space_def,
> + key_def);
13. Why do you need to declare 'WhereTerm *p;' above? Please, make the new functions
obey Tarantool code style. I will not mention it again below.
> + op &= WO_EQ;
> + while (p) {
> + if ((p->prereqRight & notReady) == 0) {
> + if (p->prereqRight == 0 && (p->eOperator & op) != 0) {
> + testcase(p->eOperator & WO_IS);
> + return p;
> + }
> + if (pResult == 0)
> + pResult = p;
> + }
> + p = whereScanNext(&scan);
> + }
> + return pResult;
> +}
> +
> /*
> * This function searches pList for an entry that matches the iCol-th column
> * of index pIdx.
> @@ -1703,6 +1764,7 @@ whereLoopInit(WhereLoop * p)
> p->nLTerm = 0;
> p->nLSlot = ArraySize(p->aLTermSpace);
> p->wsFlags = 0;
> + p->index_def = NULL;
14. Why for non-existing struct Table you have created dummy one, but for
non-existing struct Index you can not do the same? You can add
struct index_def to struct Index for this, it is not? Anyways index_def
is going to be part of struct Index.
> }
>
> /*
> @@ -3366,7 +3428,7 @@ wherePathSatisfiesOrderBy(WhereInfo * pWInfo, /* The WHERE clause */
> /* Get the column number in the table (iColumn) and sort order
> * (revIdx) for the j-th column of the index.
> */
> - if (pIndex) {
> + if (pIndex && j < pIndex->nColumn) {
15. pIndex != NULL
16. How j can be > pIndex->nColumn?
> iColumn = pIndex->aiColumn[j];
> revIdx = sql_index_column_sort_order(pIndex,
> j);
> @@ -4080,34 +4142,74 @@ whereShortCut(WhereLoopBuilder * pBuilder)
> /* TUNING: Cost of a PK lookup is 10 */
> pLoop->rRun = 33; /* 33==sqlite3LogEst(10) */
> } else {
> - for (pIdx = pTab->pIndex; pIdx; pIdx = pIdx->pNext) {
> + struct space *space = space_cache_find(space_def->id);
> + if (space != NULL) {
> + for (uint32_t i = 0; i < space->index_count; ++i) {
> + struct index_def *idx_def;
> + idx_def = space->index[i]->def;
> + int opMask;
> + int nIdxCol = idx_def->key_def->part_count;
> + assert(pLoop->aLTermSpace == pLoop->aLTerm);
> + if (!idx_def->opts.is_unique
> + /* || pIdx->pPartIdxWhere != 0 */
> + || nIdxCol > ArraySize(pLoop->aLTermSpace)
> + )
> + continue;
> + opMask = WO_EQ;
> + for (j = 0; j < nIdxCol; j++) {
> + pTerm = sql_where_find_term(pWC, iCur,
> + j, 0,
> + opMask,
> + space_def,
> + idx_def->
> + key_def);
> + if (pTerm == 0)
> + break;
> + testcase(pTerm->eOperator & WO_IS);
> + pLoop->aLTerm[j] = pTerm;
> + }
> + if (j != nIdxCol)
> + continue;
> + pLoop->wsFlags = WHERE_COLUMN_EQ |
> + WHERE_ONEROW | WHERE_INDEXED |
> + WHERE_IDX_ONLY;
> + pLoop->nLTerm = j;
> + pLoop->nEq = j;
> + pLoop->pIndex = NULL;
> + pLoop->index_def = idx_def;
> + /* TUNING: Cost of a unique index lookup is 15 */
> + pLoop->rRun = 39; /* 39==sqlite3LogEst(15) */
> + break;
17. Too much copypaste. Can you please reduce duplicating?
> + }
> + } else {
> + /* Space is ephemeral. */
> + assert(space_def->id == 0);
> int opMask;
> - int nIdxCol = index_column_count(pIdx);
> + int nIdxCol = space_def->field_count;
> assert(pLoop->aLTermSpace == pLoop->aLTerm);
> - if (!index_is_unique(pIdx)
> - || pIdx->pPartIdxWhere != 0
> - || nIdxCol > ArraySize(pLoop->aLTermSpace)
> - )
> - continue;
> + if ( nIdxCol > ArraySize(pLoop->aLTermSpace))
> + return 0;
> opMask = WO_EQ;
> for (j = 0; j < nIdxCol; j++) {
> - pTerm =
> - sqlite3WhereFindTerm(pWC, iCur, j, 0,
> - opMask, pIdx);
> - if (pTerm == 0)
> + pTerm = sql_where_find_term(pWC, iCur,
> + j, 0,
> + opMask,
> + space_def,
> + NULL);
> + if (pTerm == NULL)
> break;
> pLoop->aLTerm[j] = pTerm;
> }
> if (j != nIdxCol)
> - continue;
> + return 0;
> pLoop->wsFlags = WHERE_COLUMN_EQ | WHERE_ONEROW |
> WHERE_INDEXED | WHERE_IDX_ONLY;
> pLoop->nLTerm = j;
> pLoop->nEq = j;
> - pLoop->pIndex = pIdx;
> + pLoop->pIndex = NULL;
> + pLoop->index_def = NULL;
> /* TUNING: Cost of a unique index lookup is 15 */
> pLoop->rRun = 39; /* 39==sqlite3LogEst(15) */
> - break;
> }
> }
> if (pLoop->wsFlags) {
^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 1/3] sql: fetch primary index for affinity only
2018-06-01 18:00 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-06-07 12:03 ` Kirill Yukhin
2018-06-07 15:01 ` Vladislav Shpilevoy
0 siblings, 1 reply; 19+ messages in thread
From: Kirill Yukhin @ 2018-06-07 12:03 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
Hello Vlad,
My answers are inlined. Updated patch is in the bottom.
On 01 июн 21:00, Vladislav Shpilevoy wrote:
> Hello. Thanks for the patch! See 2 comments below.
>
> On 01/06/2018 18:16, Kirill Yukhin wrote:
> > This small patch removes usages of primary index throughout
> > code sql_table_delete_from, limiting use to fetching of
> > affinities only. We cannot use space_def here, since primary
> > index might contain calculated columns.
> 1. What are calculated columns?
A.k.a. computed columns, those which ultimately are expressions.
> > Part of #3235
> > ---
> > src/box/sql/delete.c | 54 ++++++++++++++++++++++++++++++++++++----------------
> > 1 file changed, 38 insertions(+), 16 deletions(-)
> >
> > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> > index ddad54b..28713c8 100644
> > --- a/src/box/sql/delete.c
> > +++ b/src/box/sql/delete.c
> > @@ -183,6 +183,16 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
> > struct NameContext nc;
> > memset(&nc, 0, sizeof(nc));
> > nc.pParse = parse;
> > + if (tab_list->a[0].pTab == NULL) {
> > + struct Table *t = malloc(sizeof(struct Table));
> > + if (t == NULL) {
> > + sqlite3OomFault(parse->db);
> > + goto delete_from_cleanup;
> > + }
> > + assert(space != NULL);
> > + t->def = space->def;
> > + tab_list->a[0].pTab = t;
>
> 2. Why can not you declare struct Table on the stack at the top of this
> function, and here use its address? Can this pTab be used out of
> sql_table_delete_from()? And I do not see where do you delete this
> table.
Relaced w/ stack variable.
--
Regards, Kirill Yukhin
commit 6ef9616b71c6e253336c012442b90c4d2ebb2c55
Author: Kirill Yukhin <kyukhin@tarantool.org>
Date: Wed May 23 15:31:37 2018 +0300
sql: fetch primary index for affinity only
This small patch removes usages of primary index throughout
code sql_table_delete_from, limiting use to fetching of
affinities only. We cannot use space_def here, since primary
index might contain calculated columns.
Part of #3235
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index ddad54b..a4a8da6 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -76,6 +76,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
struct Expr *where)
{
struct sqlite3 *db = parse->db;
+ struct Table loc_table;
if (parse->nErr || db->mallocFailed)
goto delete_from_cleanup;
@@ -183,6 +184,12 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
struct NameContext nc;
memset(&nc, 0, sizeof(nc));
nc.pParse = parse;
+ if (tab_list->a[0].pTab == NULL) {
+ assert(space != NULL);
+ memset(&loc_table, 0, sizeof(struct Table));
+ loc_table.def = space->def;
+ tab_list->a[0].pTab = &loc_table;
+ }
nc.pSrcList = tab_list;
if (sqlite3ResolveExprNames(&nc, where))
goto delete_from_cleanup;
@@ -196,7 +203,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
* is held in ephemeral table, there is no PK for
* it, so columns should be loaded manually.
*/
- struct Index *pk = NULL;
+ struct key_def *pk_def = NULL;
int reg_pk = parse->nMem + 1;
int pk_len;
int eph_cursor = parse->nTab++;
@@ -207,13 +214,17 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
sqlite3VdbeAddOp2(v, OP_OpenTEphemeral,
eph_cursor, pk_len);
} else {
- pk = sqlite3PrimaryKeyIndex(table);
- assert(pk != NULL);
- pk_len = index_column_count(pk);
+ assert(space->index_count > 0);
+ pk_def = key_def_dup(space->index[0]->def->key_def);
+ if(pk_def == NULL) {
+ sqlite3OomFault(parse->db);
+ goto delete_from_cleanup;
+ }
+ pk_len = pk_def->part_count;
parse->nMem += pk_len;
- sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, eph_cursor,
- pk_len);
- sql_vdbe_set_p4_key_def(parse, pk);
+ sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, eph_cursor,
+ pk_len, 0,
+ (char *)pk_def, P4_KEYDEF);
}
/* Construct a query to find the primary key for
@@ -252,11 +263,9 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
/* Extract the primary key for the current row */
if (!is_view) {
for (int i = 0; i < pk_len; i++) {
- assert(pk->aiColumn[i] >= 0);
sqlite3ExprCodeGetColumnOfTable(v, table->def,
tab_cursor,
- pk->
- aiColumn[i],
+ pk_def->parts[i].fieldno,
reg_pk + i);
}
} else {
@@ -287,10 +296,14 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
* key.
*/
key_len = 0;
- const char *zAff = is_view ? NULL :
- sqlite3IndexAffinityStr(parse->db, pk);
+ const char *aff = NULL;
+ struct Index *pk = sqlite3PrimaryKeyIndex(table);
+ if (pk != NULL) {
+ aff = is_view ? NULL :
+ sqlite3IndexAffinityStr(parse->db, pk);
+ }
sqlite3VdbeAddOp4(v, OP_MakeRecord, reg_pk, pk_len,
- reg_key, zAff, pk_len);
+ reg_key, aff, pk_len);
/* Set flag to save memory allocating one
* by malloc.
*/
@@ -325,8 +338,14 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
(void *)space, P4_SPACEPTR);
sqlite3VdbeAddOp3(v, OP_OpenWrite, tab_cursor,
table->tnum, space_ptr_reg);
- sql_vdbe_set_p4_key_def(parse, pk);
- VdbeComment((v, "%s", pk->zName));
+ struct key_def *def = key_def_dup(pk_def);
+ if (def == NULL) {
+ sqlite3OomFault(parse->db);
+ goto delete_from_cleanup;
+ }
+ sqlite3VdbeAppendP4(v, def, P4_KEYDEF);
+
+ VdbeComment((v, "%s", space->index[0]->def->name));
if (one_pass == ONEPASS_MULTI)
sqlite3VdbeJumpHere(v, iAddrOnce);
@@ -339,7 +358,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
if (one_pass != ONEPASS_OFF) {
/* OP_Found will use an unpacked key. */
assert(key_len == pk_len);
- assert(pk != NULL || table->pSelect != NULL);
+ assert(pk_def != NULL || table->pSelect != NULL);
sqlite3VdbeAddOp4Int(v, OP_NotFound, tab_cursor,
addr_bypass, reg_key, key_len);
^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 1/3] sql: fetch primary index for affinity only
2018-06-07 12:03 ` Kirill Yukhin
@ 2018-06-07 15:01 ` Vladislav Shpilevoy
2018-06-08 8:25 ` Kirill Yukhin
0 siblings, 1 reply; 19+ messages in thread
From: Vladislav Shpilevoy @ 2018-06-07 15:01 UTC (permalink / raw)
To: tarantool-patches, Kirill Yukhin
Hello. Thanks for the fixes! See one minor comment below.
And I do not see the new commit on the branch. Branch still has the
old one + 2 my review fixes commits.
> --
> Regards, Kirill Yukhin
>
> commit 6ef9616b71c6e253336c012442b90c4d2ebb2c55
> Author: Kirill Yukhin <kyukhin@tarantool.org>
> Date: Wed May 23 15:31:37 2018 +0300
>
> sql: fetch primary index for affinity only
>
> This small patch removes usages of primary index throughout
> code sql_table_delete_from, limiting use to fetching of
> affinities only. We cannot use space_def here, since primary
> index might contain calculated columns.
>
> Part of #3235
>
> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> index ddad54b..a4a8da6 100644
> --- a/src/box/sql/delete.c
> +++ b/src/box/sql/delete.c
> @@ -325,8 +338,14 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
> (void *)space, P4_SPACEPTR);
> sqlite3VdbeAddOp3(v, OP_OpenWrite, tab_cursor,
> table->tnum, space_ptr_reg);
> - sql_vdbe_set_p4_key_def(parse, pk);
> - VdbeComment((v, "%s", pk->zName));
> + struct key_def *def = key_def_dup(pk_def);
> + if (def == NULL) {
> + sqlite3OomFault(parse->db);
> + goto delete_from_cleanup;
> + }
> + sqlite3VdbeAppendP4(v, def, P4_KEYDEF);
Why could not you at first dup key_def, and then just use AddOp4? Instead of
AddOp3 + dup + AppendP4.
> +
> + VdbeComment((v, "%s", space->index[0]->def->name));
>
> if (one_pass == ONEPASS_MULTI)
> sqlite3VdbeJumpHere(v, iAddrOnce);
^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 1/3] sql: fetch primary index for affinity only
2018-06-07 15:01 ` Vladislav Shpilevoy
@ 2018-06-08 8:25 ` Kirill Yukhin
0 siblings, 0 replies; 19+ messages in thread
From: Kirill Yukhin @ 2018-06-08 8:25 UTC (permalink / raw)
To: Vladislav Shpilevoy; +Cc: tarantool-patches
Hello Vlad,
Updated patch in the bottom.
On 07 июн 18:01, Vladislav Shpilevoy wrote:
> Hello. Thanks for the fixes! See one minor comment below.
>
> And I do not see the new commit on the branch. Branch still has the
> old one + 2 my review fixes commits.
Pushed.
> > --
> > Regards, Kirill Yukhin
> >
> > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> > index ddad54b..a4a8da6 100644
> > --- a/src/box/sql/delete.c
> > +++ b/src/box/sql/delete.c
> > @@ -325,8 +338,14 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
> > (void *)space, P4_SPACEPTR);
> > sqlite3VdbeAddOp3(v, OP_OpenWrite, tab_cursor,
> > table->tnum, space_ptr_reg);
> > - sql_vdbe_set_p4_key_def(parse, pk);
> > - VdbeComment((v, "%s", pk->zName));
> > + struct key_def *def = key_def_dup(pk_def);
> > + if (def == NULL) {
> > + sqlite3OomFault(parse->db);
> > + goto delete_from_cleanup;
> > + }
> > + sqlite3VdbeAppendP4(v, def, P4_KEYDEF);
> Why could not you at first dup key_def, and then just use AddOp4? Instead of
> AddOp3 + dup + AppendP4.
Done.
--
Regards, Kirill Yukhin
commit 34e707406484a144e538c05677e1e6ea5a819fd3
Author: Kirill Yukhin <kyukhin@tarantool.org>
Date: Wed May 23 15:31:37 2018 +0300
sql: fetch primary index for affinity only
This small patch removes usages of primary index throughout
code sql_table_delete_from, limiting use to fetching of
affinities only. We cannot use space_def here, since primary
index might contain calculated columns.
Part of #3235
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index ddad54b..eff4da1 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -76,6 +76,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
struct Expr *where)
{
struct sqlite3 *db = parse->db;
+ struct Table loc_table;
if (parse->nErr || db->mallocFailed)
goto delete_from_cleanup;
@@ -183,6 +184,12 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
struct NameContext nc;
memset(&nc, 0, sizeof(nc));
nc.pParse = parse;
+ if (tab_list->a[0].pTab == NULL) {
+ assert(space != NULL);
+ memset(&loc_table, 0, sizeof(struct Table));
+ loc_table.def = space->def;
+ tab_list->a[0].pTab = &loc_table;
+ }
nc.pSrcList = tab_list;
if (sqlite3ResolveExprNames(&nc, where))
goto delete_from_cleanup;
@@ -196,7 +203,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
* is held in ephemeral table, there is no PK for
* it, so columns should be loaded manually.
*/
- struct Index *pk = NULL;
+ struct key_def *pk_def = NULL;
int reg_pk = parse->nMem + 1;
int pk_len;
int eph_cursor = parse->nTab++;
@@ -207,13 +214,17 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
sqlite3VdbeAddOp2(v, OP_OpenTEphemeral,
eph_cursor, pk_len);
} else {
- pk = sqlite3PrimaryKeyIndex(table);
- assert(pk != NULL);
- pk_len = index_column_count(pk);
+ assert(space->index_count > 0);
+ pk_def = key_def_dup(space->index[0]->def->key_def);
+ if(pk_def == NULL) {
+ sqlite3OomFault(parse->db);
+ goto delete_from_cleanup;
+ }
+ pk_len = pk_def->part_count;
parse->nMem += pk_len;
- sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, eph_cursor,
- pk_len);
- sql_vdbe_set_p4_key_def(parse, pk);
+ sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, eph_cursor,
+ pk_len, 0,
+ (char *)pk_def, P4_KEYDEF);
}
/* Construct a query to find the primary key for
@@ -252,11 +263,9 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
/* Extract the primary key for the current row */
if (!is_view) {
for (int i = 0; i < pk_len; i++) {
- assert(pk->aiColumn[i] >= 0);
sqlite3ExprCodeGetColumnOfTable(v, table->def,
tab_cursor,
- pk->
- aiColumn[i],
+ pk_def->parts[i].fieldno,
reg_pk + i);
}
} else {
@@ -287,10 +296,14 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
* key.
*/
key_len = 0;
- const char *zAff = is_view ? NULL :
- sqlite3IndexAffinityStr(parse->db, pk);
+ const char *aff = NULL;
+ struct Index *pk = sqlite3PrimaryKeyIndex(table);
+ if (pk != NULL) {
+ aff = is_view ? NULL :
+ sqlite3IndexAffinityStr(parse->db, pk);
+ }
sqlite3VdbeAddOp4(v, OP_MakeRecord, reg_pk, pk_len,
- reg_key, zAff, pk_len);
+ reg_key, aff, pk_len);
/* Set flag to save memory allocating one
* by malloc.
*/
@@ -323,10 +336,16 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
int space_ptr_reg = ++parse->nMem;
sqlite3VdbeAddOp4(v, OP_LoadPtr, 0, space_ptr_reg, 0,
(void *)space, P4_SPACEPTR);
- sqlite3VdbeAddOp3(v, OP_OpenWrite, tab_cursor,
- table->tnum, space_ptr_reg);
- sql_vdbe_set_p4_key_def(parse, pk);
- VdbeComment((v, "%s", pk->zName));
+ struct key_def *def = key_def_dup(pk_def);
+ if (def == NULL) {
+ sqlite3OomFault(parse->db);
+ goto delete_from_cleanup;
+ }
+ sqlite3VdbeAddOp4(v, OP_OpenWrite, tab_cursor,
+ table->tnum, space_ptr_reg,
+ (void *)def, P4_KEYDEF);
+
+ VdbeComment((v, "%s", space->index[0]->def->name));
if (one_pass == ONEPASS_MULTI)
sqlite3VdbeJumpHere(v, iAddrOnce);
@@ -339,7 +358,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
if (one_pass != ONEPASS_OFF) {
/* OP_Found will use an unpacked key. */
assert(key_len == pk_len);
- assert(pk != NULL || table->pSelect != NULL);
+ assert(pk_def != NULL || table->pSelect != NULL);
sqlite3VdbeAddOp4Int(v, OP_NotFound, tab_cursor,
addr_bypass, reg_key, key_len);
^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 0/3] sql: implement point where for DELETE stmts
2018-06-01 15:16 [tarantool-patches] [PATCH 0/3] sql: implement point where for DELETE stmts Kirill Yukhin
` (2 preceding siblings ...)
2018-06-01 15:16 ` [tarantool-patches] [PATCH 3/3] sql: implement point where for DELETE stmts Kirill Yukhin
@ 2018-06-14 12:41 ` Kirill Yukhin
3 siblings, 0 replies; 19+ messages in thread
From: Kirill Yukhin @ 2018-06-14 12:41 UTC (permalink / raw)
To: v.shpilevoy; +Cc: tarantool-patches
Hello Vlad,
On 01 июн 18:16, Kirill Yukhin wrote:
> This patches consists of two preparational patches and the
> main patch which enables DELETE's from spaces created w/ Lua.
Thanks for your inputs and even patches. I've squashed your
review changes (w/ minor fixes) into main patch-set and committed
them to 2.0 branch.
--
Regards, Kirill Yukhin
^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 3/3] sql: implement point where for DELETE stmts
2018-06-01 18:00 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-06-18 2:11 ` n.pettik
2018-06-18 10:44 ` Vladislav Shpilevoy
0 siblings, 1 reply; 19+ messages in thread
From: n.pettik @ 2018-06-18 2:11 UTC (permalink / raw)
To: tarantool-patches; +Cc: Kirill Yukhin, Vladislav Shpilevoy
For some reason this patch got to the trunk without review fixes.
Raw version of this patch contains several bugs, so they led to test fails
(sql-tap/analyze6.test.lua and few sql-tap/tkt*).
I suggest my own fixes. The whole patch is at the end of letter.
Branch: https://github.com/tarantool/tarantool/commits/np/gh-3463-review-fixed-for-point-delete
Issue: https://github.com/tarantool/tarantool/issues/3463
>
>> }
>> struct ExprList *
>> @@ -1221,6 +1216,22 @@ emit_open_cursor(Parse *parse_context, int cursor, int entity_id)
>> space_ptr_reg);
>> }
>> +int
>> +sql_emit_open_cursor(struct Parse *parse, int cursor, int index_id, struct space *space)
>
> 2. Few lines above I see emit_open_cursor. Please, try to remove one of them.
> At the worst case you can just inline emit_open_cursor in the single place of
> its usage, and rename sql_emit_open_cursor to emit_open_cursor.
Ok, I just replaced all calls of emit_open_cursor() with sql_emit_open_cursor().
The only difference is in doing space lookup outside emit_open_cursor().
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 552578048..8c83a797e 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1181,35 +1181,17 @@ space_checks_expr_list(uint32_t space_id)
}
int
-emit_open_cursor(struct Parse *parse_context, int cursor, int entity_id)
+sql_emit_open_cursor(struct Parse *parse_context, int cursor, int index_id,
+ struct space *space)
{
- assert(entity_id > 0);
- struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(entity_id));
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, entity_id,
+ return sqlite3VdbeAddOp3(vdbe, OP_OpenWrite, cursor, index_id,
space_ptr_reg);
}
-
-int
-sql_emit_open_cursor(struct Parse *parse, int cursor, int index_id, struct space *space)
-{
- assert(space != NULL);
- Vdbe *vdbe = parse->pVdbe;
- int space_ptr_reg = ++parse->nMem;
- sqlite3VdbeAddOp4(vdbe, OP_LoadPtr, 0, space_ptr_reg, 0, (void*)space,
- P4_SPACEPTR);
- struct key_def *def = key_def_dup(space->index[index_id]->def->key_def);
- if (def == NULL)
- return 0;
- return sqlite3VdbeAddOp4(vdbe, OP_OpenWrite, cursor, index_id,
- space_ptr_reg,
- (char*)def,
- P4_KEYDEF);
-}
>
>> +{
>> + assert(space != NULL);
>> + Vdbe *vdbe = parse->pVdbe;
>> + int space_ptr_reg = ++parse->nMem;
>> + sqlite3VdbeAddOp4(vdbe, OP_LoadPtr, 0, space_ptr_reg, 0, (void*)space,
>> + P4_SPACEPTR);
>> + struct key_def *def = key_def_dup(space->index[index_id]->def->key_def);
>> + if (def == NULL)
>> + return 0;
>> + return sqlite3VdbeAddOp4(vdbe, OP_OpenWrite, cursor, index_id,
>> + space_ptr_reg,
>> + (char*)def,
>> + P4_KEYDEF);
>
> 3. Looks like the arguments can fit in 2 lines instead of 4.
Fixed. See code above.
>
>> +}
>> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
>> index 2b59130..de4e0c1 100644
>> --- a/src/box/sql/delete.c
>> +++ b/src/box/sql/delete.c
>> @@ -184,7 +184,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>> memset(&nc, 0, sizeof(nc));
>> nc.pParse = parse;
>> if (tab_list->a[0].pTab == NULL) {
>> - struct Table *t = malloc(sizeof(struct Table));
>> + struct Table *t = calloc(sizeof(struct Table), 1);
>
> 4. It must the part of the previous patch.
Refactored this way:
+++ b/src/box/sql/delete.c
@@ -88,7 +88,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
* instead of just a Table* parameter.
*/
struct Table *table = NULL;
- struct space *space;
+ struct space *space = NULL;
uint32_t space_id;
/* Figure out if we have any triggers and if the table
* being deleted from is a view.
@@ -104,17 +104,28 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
strlen(tab_name));
if (space_id == BOX_ID_NIL)
goto delete_from_cleanup;
+ /*
+ * In this case space has been created via Lua
+ * facilities, so there is no corresponding entry
+ * in table hash. Thus, lets create simple
+ * wrapper around space_def to support interface.
+ */
+ space = space_by_id(space_id);
+ tab_list->a[0].pTab = sql_table_construct_from_space(space);
+ if (tab_list->a[0].pTab == NULL)
+ goto delete_from_cleanup;
} else {
table = sql_list_lookup_table(parse, tab_list);
if (table == NULL)
goto delete_from_cleanup;
space_id = SQLITE_PAGENO_TO_SPACEID(table->tnum);
+ space = space_by_id(space_id);
+ assert(space != NULL);
@@ -183,16 +194,6 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
struct NameContext nc;
memset(&nc, 0, sizeof(nc));
nc.pParse = parse;
- if (tab_list->a[0].pTab == NULL) {
- struct Table *t = calloc(sizeof(struct Table), 1);
- if (t == NULL) {
- sqlite3OomFault(parse->db);
- goto delete_from_cleanup;
- }
- assert(space != NULL);
- t->def = space->def;
- tab_list->a[0].pTab = t;
- }
+++ b/src/box/sql/resolve.c
@@ -39,6 +39,9 @@
#include <stdlib.h>
#include <string.h>
+#include "box/box.h"
+#include "box/schema.h"
+
/*
* Walk the expression tree pExpr and increase the aggregate function
* depth (the Expr.op2 field) by N on every TK_AGG_FUNCTION node.
@@ -1432,6 +1435,21 @@ resolveSelectStep(Walker * pWalker, Select * p)
return WRC_Prune;
}
+struct Table *
+sql_table_construct_from_space(const struct space *space)
+{
+ assert(space != NULL);
+ struct Table *table = calloc(1, sizeof(*table));
+ if (table == NULL) {
+ diag_set(OutOfMemory, sizeof(table), "calloc", "table");
+ return NULL;
+ }
+ table->def = space_def_dup(space->def);
+ if (table->def == NULL)
+ return NULL;
+ return table;
+}
+
+/**
+ * Create wrapper around space_def of the given space.
+ * This routine is required to support.
+ * Both table and space def are allocated on heap.
+ *
+ * @param space Space to be prototype.
+ * @retval New instance of struct Table allocated on malloc,
+ * or NULL in case of OOM.
+ */
+struct Table *
+sql_table_construct_from_space(const struct space *space);
>
>> if (t == NULL) {
>> sqlite3OomFault(parse->db);
>> goto delete_from_cleanup;
>> @@ -266,7 +266,12 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>> /* Extract the primary key for the current row */
>> if (!is_view) {
>> for (int i = 0; i < pk_len; i++) {
>> - sqlite3ExprCodeGetColumnOfTable(v, table->def,
>> + struct space_def *def;
>> + if (table != NULL)
>> + def = table->def;
>> + else
>> + def = space->def;
>
> 5. Why not always space->def?
Idk, indeed lets always use space->def:
@@ -266,11 +267,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
/* Extract the primary key for the current row */
if (!is_view) {
for (int i = 0; i < pk_len; i++) {
- struct space_def *def;
- if (table != NULL)
- def = table->def;
- else
- def = space->def;
+ struct space_def *def = space->def;
>
>> + sqlite3ExprCodeGetColumnOfTable(v, def,
>> tab_cursor,
>> pk_def->parts[i].fieldno,
>> reg_pk + i);
>> @@ -339,8 +344,18 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>> int space_ptr_reg = ++parse->nMem;
>> sqlite3VdbeAddOp4(v, OP_LoadPtr, 0, space_ptr_reg, 0,
>> (void *)space, P4_SPACEPTR);
>> +
>> + int tnum;
>> + if (table != NULL) {
>> + tnum = table->tnum;
>> + }
>> + else {
>> + /* index id is 0 for PK. */
>> + tnum = SQLITE_PAGENO_FROM_SPACEID_AND_INDEXID(space->def->id,
>> + 0);
>> + }
>
> 6. Why not always the second version?
Idk as well, lets always use conversion from space->def->id.
@@ -342,15 +339,9 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
sqlite3VdbeAddOp4(v, OP_LoadPtr, 0, space_ptr_reg, 0,
(void *)space, P4_SPACEPTR);
- int tnum;
- if (table != NULL) {
- tnum = table->tnum;
- }
- else {
- /* index id is 0 for PK. */
- tnum = SQLITE_PAGENO_FROM_SPACEID_AND_INDEXID(space->def->id,
- 0);
- }
+ int tnum =
+ SQLITE_PAGENO_FROM_SPACEID_AND_INDEXID(space->def->id,
+ 0);
>
>> sqlite3VdbeAddOp3(v, OP_OpenWrite, tab_cursor,
>> - table->tnum, space_ptr_reg);
>> + tnum, space_ptr_reg);
>> struct key_def *def = key_def_dup(pk_def);
>> if (def == NULL) {
>> sqlite3OomFault(parse->db);
>> @@ -505,7 +521,7 @@ sql_generate_row_delete(struct Parse *parse, struct Table *table,
>> * of the DELETE statement is to fire the INSTEAD OF
>> * triggers).
>> */
>> - if (table->pSelect == NULL) {
>> + if (table == NULL || table->pSelect == NULL) {
>
> 7. Is the same as space->def->opts.is_view.
Ok:
@@ -518,7 +509,7 @@ sql_generate_row_delete(struct Parse *parse, struct Table *table,
* of the DELETE statement is to fire the INSTEAD OF
* triggers).
*/
- if (table == NULL || table->pSelect == NULL) {
+ if (table == NULL || !table->def->opts.is_view) {
>
>> uint8_t p5 = 0;
>> sqlite3VdbeAddOp2(v, OP_Delete, cursor,
>> (need_update_count ? OPFLAG_NCHANGE : 0));
>> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
>> index 3dbf855..8d0ab3b 100644
>> --- a/src/box/sql/insert.c
>> +++ b/src/box/sql/insert.c
>> @@ -108,6 +108,39 @@ sqlite3IndexAffinityStr(sqlite3 * db, Index * pIdx)
>> return pIdx->zColAff;
>> }
>> +char *
>> +sql_index_affinity_str(struct sqlite3 * db, struct index_def *def)
>> +{
>> + char *aff;
>> + /* The first time a column affinity string for a particular index is
>> + * required, it is allocated and populated here. It is then stored as
>> + * a member of the Index structure for subsequent use.
>
> 8. No, it is not populated.
Deleted the whole comment - it doesn’t seem to be relevant.
And slightly refactored function:
@@ -107,32 +107,22 @@ sqlite3IndexAffinityStr(sqlite3 *db, Index *index)
char *
sql_index_affinity_str(struct sqlite3 * db, struct index_def *def)
{
- char *aff;
- /* The first time a column affinity string for a particular index is
- * required, it is allocated and populated here. It is then stored as
- * a member of the Index structure for subsequent use.
- *
- * The column affinity string will eventually be deleted by
- * sqliteDeleteIndex() when the Index structure itself is cleaned
- * up.
- */
- int nColumn = def->key_def->part_count;
- aff = (char *)sqlite3DbMallocRaw(0, nColumn + 1);
+ uint32_t column_count = def->key_def->part_count;
+ char *aff = (char *)sqlite3DbMallocRaw(0, column_count + 1);
if (aff == NULL) {
sqlite3OomFault(db);
- return 0;
+ return NULL;
}
- int i;
- struct space *space = space_cache_find(def->space_id);
+ struct space *space = space_by_id(def->space_id);
assert(space != NULL);
- for (i = 0; i < nColumn; i++) {
+ for (uint32_t i = 0; i < column_count; i++) {
uint32_t x = def->key_def->parts[i].fieldno;
aff[i] = space->def->fields[x].affinity;
if (aff[i] == AFFINITY_UNDEFINED)
aff[i] = 'A';
}
- aff[i] = 0;
+ aff[column_count] = '\0';
return aff;
}
>
>> + *
>> + * The column affinity string will eventually be deleted by
>> + * sqliteDeleteIndex() when the Index structure itself is cleaned
>> + * up.
>> + */
>> + int nColumn = def->key_def->part_count;
>> + aff = (char *)sqlite3DbMallocRaw(0, nColumn + 1);
>
> 9. Why do you need to declare 'char *aff;' separately above?
Fixed, see diff above.
>
>> + if (aff == NULL) {
>> + sqlite3OomFault(db);
>> + return 0;
>> + }
>> + int i;
>
> 10. Can be part of 'for’.
Fixed, see diff above.
>
>> + struct space *space = space_cache_find(def->space_id);
>> + assert(space != NULL);
>> +
>> + for (i = 0; i < nColumn; i++) {
>> + uint32_t x = def->key_def->parts[i].fieldno;
>> + aff[i] = space->def->fields[x].affinity;
>> + if (aff[i] == AFFINITY_UNDEFINED)
>> + aff[i] = 'A';
>> + }
>> + aff[i] = 0;
>> +
>> + return aff;
>> +}
>> +
>> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
>> index 943fda9..6ea956d 100644
>> --- a/src/box/sql/sqliteInt.h
>> +++ b/src/box/sql/sqliteInt.h
>> @@ -2536,6 +2536,8 @@ struct SrcList {
>> char *zName; /* Name of the table */
>> char *zAlias; /* The "B" part of a "A AS B" phrase. zName is the "A" */
>> Table *pTab; /* An SQL table corresponding to zName */
>> + /* A temporary hack: need to store eph. space. */
>> + struct space *space;
>
> 11. ??? It is unused.
Indeed, removed:
+++ b/src/box/sql/sqliteInt.h
@@ -2536,8 +2536,6 @@ struct SrcList {
char *zName; /* Name of the table */
char *zAlias; /* The "B" part of a "A AS B" phrase. zName is the "A" */
Table *pTab; /* An SQL table corresponding to zName */
- /* A temporary hack: need to store eph. space. */
- struct space *space;
>
>> Select *pSelect; /* A SELECT statement used in place of a table name */
>> int addrFillSub; /* Address of subroutine to manifest a subquery */
>> int regReturn; /* Register holding return address of addrFillSub */
>> diff --git a/src/box/sql/where.c b/src/box/sql/where.c
>> index d312587..14cb23d 100644
>> --- a/src/box/sql/where.c
>> +++ b/src/box/sql/where.c
>> @@ -371,7 +371,10 @@ whereScanInit(WhereScan * pScan, /* The WhereScan object being initialized */
>> pScan->is_column_seen = false;
>> if (pIdx) {
>> int j = iColumn;
>> - iColumn = pIdx->aiColumn[j];
>> + if (j >= pIdx->nColumn)
>> + iColumn = -1;
>> + else
>> + iColumn = pIdx->aiColumn[j];
>
> 12. How can j can be > nColumn?
No way:
@@ -371,10 +372,7 @@ whereScanInit(WhereScan * pScan, /* The WhereScan object being initialized */
pScan->is_column_seen = false;
if (pIdx) {
int j = iColumn;
- if (j >= pIdx->nColumn)
- iColumn = -1;
- else
- iColumn = pIdx->aiColumn[j];
+ iColumn = pIdx->aiColumn[j];
>
>> if (iColumn >= 0) {
>> char affinity =
>> pIdx->pTable->def->fields[iColumn].affinity;
>> @@ -441,6 +472,36 @@ sqlite3WhereFindTerm(WhereClause * pWC, /* The WHERE clause to be searched */
>> return pResult;
>> }
>> +WhereTerm *
>> +sql_where_find_term(WhereClause * pWC, /* The WHERE clause to be searched */
>> + int iCur, /* Cursor number of LHS */
>> + int iColumn, /* Column number of LHS */
>> + Bitmask notReady, /* RHS must not overlap with this mask */
>> + u32 op, /* Mask of WO_xx values describing operator */
>> + struct space_def *space_def,
>> + struct key_def *key_def) /* Must be compatible with this index, if not NULL */
>> +{
>> + WhereTerm *pResult = 0;
>> + WhereTerm *p;
>> + WhereScan scan;
>> +
>> + p = sql_where_scan_init(&scan, pWC, iCur, iColumn, op, space_def,
>> + key_def);
>
> 13. Why do you need to declare 'WhereTerm *p;' above? Please, make the new functions
> obey Tarantool code style. I will not mention it again below.
Ok, I have refactored this function. See diff at the end of letter.
>
>> + op &= WO_EQ;
>> + while (p) {
>> + if ((p->prereqRight & notReady) == 0) {
>> + if (p->prereqRight == 0 && (p->eOperator & op) != 0) {
>> + testcase(p->eOperator & WO_IS);
>> + return p;
>> + }
>> + if (pResult == 0)
>> + pResult = p;
>> + }
>> + p = whereScanNext(&scan);
>> + }
>> + return pResult;
>> +}
>> +
>> /*
>> * This function searches pList for an entry that matches the iCol-th column
>> * of index pIdx.
>> @@ -1703,6 +1764,7 @@ whereLoopInit(WhereLoop * p)
>> p->nLTerm = 0;
>> p->nLSlot = ArraySize(p->aLTermSpace);
>> p->wsFlags = 0;
>> + p->index_def = NULL;
>
> 14. Why for non-existing struct Table you have created dummy one, but for
> non-existing struct Index you can not do the same? You can add
> struct index_def to struct Index for this, it is not? Anyways index_def
> is going to be part of struct Index.
AFAIK Ivan works on this issue, lets wait for his patch.
>
>> }
>> /*
>> @@ -3366,7 +3428,7 @@ wherePathSatisfiesOrderBy(WhereInfo * pWInfo, /* The WHERE clause */
>> /* Get the column number in the table (iColumn) and sort order
>> * (revIdx) for the j-th column of the index.
>> */
>> - if (pIndex) {
>> + if (pIndex && j < pIndex->nColumn) {
>
> 15. pIndex != NULL
>
> 16. How j can be > pIndex->nColumn?
@@ -2668,7 +2650,8 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage)
- if (pIndex && j < pIndex->nColumn) {
+ if (pIndex != NULL) {
>
>> iColumn = pIndex->aiColumn[j];
>> revIdx = sql_index_column_sort_order(pIndex,
>> j);
>> @@ -4080,34 +4142,74 @@ whereShortCut(WhereLoopBuilder * pBuilder)
>> /* TUNING: Cost of a PK lookup is 10 */
>> pLoop->rRun = 33; /* 33==sqlite3LogEst(10) */
>> } else {
>> - for (pIdx = pTab->pIndex; pIdx; pIdx = pIdx->pNext) {
>> + struct space *space = space_cache_find(space_def->id);
>> + if (space != NULL) {
>> + for (uint32_t i = 0; i < space->index_count; ++i) {
>> + struct index_def *idx_def;
>> + idx_def = space->index[i]->def;
>> + int opMask;
>> + int nIdxCol = idx_def->key_def->part_count;
>> + assert(pLoop->aLTermSpace == pLoop->aLTerm);
>> + if (!idx_def->opts.is_unique
>> + /* || pIdx->pPartIdxWhere != 0 */
>> + || nIdxCol > ArraySize(pLoop->aLTermSpace)
>> + )
>> + continue;
>> + opMask = WO_EQ;
>> + for (j = 0; j < nIdxCol; j++) {
>> + pTerm = sql_where_find_term(pWC, iCur,
>> + j, 0,
>> + opMask,
>> + space_def,
>> + idx_def->
>> + key_def);
>> + if (pTerm == 0)
>> + break;
>> + testcase(pTerm->eOperator & WO_IS);
>> + pLoop->aLTerm[j] = pTerm;
>> + }
>> + if (j != nIdxCol)
>> + continue;
>> + pLoop->wsFlags = WHERE_COLUMN_EQ |
>> + WHERE_ONEROW | WHERE_INDEXED |
>> + WHERE_IDX_ONLY;
>> + pLoop->nLTerm = j;
>> + pLoop->nEq = j;
>> + pLoop->pIndex = NULL;
>> + pLoop->index_def = idx_def;
>> + /* TUNING: Cost of a unique index lookup is 15 */
>> + pLoop->rRun = 39; /* 39==sqlite3LogEst(15) */
>> + break;
>
> 17. Too much copypaste. Can you please reduce duplicating?
See patch below.
=======================================================================
From 9ced467a2f33ce02ac48f875db06d3eec7d5561d Mon Sep 17 00:00:00 2001
From: Nikita Pettik <korablev@tarantool.org>
Date: Mon, 18 Jun 2018 00:38:55 +0300
Subject: [PATCH] sql: review fixes for a40287051
---
src/box/sql/analyze.c | 6 +-
src/box/sql/build.c | 27 +----
src/box/sql/delete.c | 45 +++----
src/box/sql/expr.c | 9 +-
src/box/sql/fkey.c | 7 +-
src/box/sql/insert.c | 36 +++---
src/box/sql/resolve.c | 18 +++
src/box/sql/select.c | 5 +-
src/box/sql/sqliteInt.h | 42 ++++---
src/box/sql/where.c | 309 +++++++++++++++++++++++++-----------------------
src/box/sql/wherecode.c | 5 +-
11 files changed, 256 insertions(+), 253 deletions(-)
diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index 9509645c0..28d68a55b 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -176,9 +176,9 @@ openStatTable(Parse * pParse, /* Parsing context */
/* Open the sql_stat[134] tables for writing. */
for (i = 0; aTable[i]; i++) {
- int addr = emit_open_cursor(pParse, iStatCur + i, aRoot[i]);
- v->aOp[addr].p4.key_def = NULL;
- v->aOp[addr].p4type = P4_KEYDEF;
+ struct space *space =
+ space_by_id(SQLITE_PAGENO_TO_SPACEID(aRoot[i]));
+ sql_emit_open_cursor(pParse, iStatCur + i, aRoot[i], space);
sqlite3VdbeChangeP5(v, aCreateTbl[i]);
VdbeComment((v, aTable[i]));
}
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 552578048..8c83a797e 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1181,35 +1181,17 @@ space_checks_expr_list(uint32_t space_id)
}
int
-emit_open_cursor(struct Parse *parse_context, int cursor, int entity_id)
+sql_emit_open_cursor(struct Parse *parse_context, int cursor, int index_id,
+ struct space *space)
{
- assert(entity_id > 0);
- struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(entity_id));
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, entity_id,
+ return sqlite3VdbeAddOp3(vdbe, OP_OpenWrite, cursor, index_id,
space_ptr_reg);
}
-
-int
-sql_emit_open_cursor(struct Parse *parse, int cursor, int index_id, struct space *space)
-{
- assert(space != NULL);
- Vdbe *vdbe = parse->pVdbe;
- int space_ptr_reg = ++parse->nMem;
- sqlite3VdbeAddOp4(vdbe, OP_LoadPtr, 0, space_ptr_reg, 0, (void*)space,
- P4_SPACEPTR);
- struct key_def *def = key_def_dup(space->index[index_id]->def->key_def);
- if (def == NULL)
- return 0;
- return sqlite3VdbeAddOp4(vdbe, OP_OpenWrite, cursor, index_id,
- space_ptr_reg,
- (char*)def,
- P4_KEYDEF);
-}
/*
* Generate code that will increment the schema cookie.
*
@@ -2668,7 +2650,8 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage)
if (memRootPage < 0)
sqlite3VdbeAddOp2(v, OP_Clear, SQLITE_PAGENO_TO_SPACEID(tnum),
0);
- emit_open_cursor(pParse, iIdx, tnum);
+ struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(tnum));
+ sql_emit_open_cursor(pParse, iIdx, tnum, space);
sqlite3VdbeChangeP5(v,
OPFLAG_BULKCSR | ((memRootPage >= 0) ?
OPFLAG_P2ISREG : 0));
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 23e2bcc49..1beacec48 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -88,7 +88,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
* instead of just a Table* parameter.
*/
struct Table *table = NULL;
- struct space *space;
+ struct space *space = NULL;
uint32_t space_id;
/* Figure out if we have any triggers and if the table
* being deleted from is a view.
@@ -104,17 +104,28 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
strlen(tab_name));
if (space_id == BOX_ID_NIL)
goto delete_from_cleanup;
+ /*
+ * In this case space has been created via Lua
+ * facilities, so there is no corresponding entry
+ * in table hash. Thus, lets create simple
+ * wrapper around space_def to support interface.
+ */
+ space = space_by_id(space_id);
+ tab_list->a[0].pTab = sql_table_construct_from_space(space);
+ if (tab_list->a[0].pTab == NULL)
+ goto delete_from_cleanup;
} else {
table = sql_list_lookup_table(parse, tab_list);
if (table == NULL)
goto delete_from_cleanup;
space_id = SQLITE_PAGENO_TO_SPACEID(table->tnum);
+ space = space_by_id(space_id);
+ assert(space != NULL);
trigger_list =sqlite3TriggersExist(table, TK_DELETE,
NULL, NULL);
is_complex = trigger_list != NULL ||
sqlite3FkRequired(table, NULL);
}
- space = space_by_id(space_id);
assert(space != NULL);
bool is_view = space->def->opts.is_view;
@@ -183,16 +194,6 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
struct NameContext nc;
memset(&nc, 0, sizeof(nc));
nc.pParse = parse;
- if (tab_list->a[0].pTab == NULL) {
- struct Table *t = calloc(sizeof(struct Table), 1);
- if (t == NULL) {
- sqlite3OomFault(parse->db);
- goto delete_from_cleanup;
- }
- assert(space != NULL);
- t->def = space->def;
- tab_list->a[0].pTab = t;
- }
nc.pSrcList = tab_list;
if (sqlite3ResolveExprNames(&nc, where))
goto delete_from_cleanup;
@@ -266,11 +267,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
/* Extract the primary key for the current row */
if (!is_view) {
for (int i = 0; i < pk_len; i++) {
- struct space_def *def;
- if (table != NULL)
- def = table->def;
- else
- def = space->def;
+ struct space_def *def = space->def;
sqlite3ExprCodeGetColumnOfTable(v, def,
tab_cursor,
pk_def->parts[i].fieldno,
@@ -342,15 +339,9 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
sqlite3VdbeAddOp4(v, OP_LoadPtr, 0, space_ptr_reg, 0,
(void *)space, P4_SPACEPTR);
- int tnum;
- if (table != NULL) {
- tnum = table->tnum;
- }
- else {
- /* index id is 0 for PK. */
- tnum = SQLITE_PAGENO_FROM_SPACEID_AND_INDEXID(space->def->id,
- 0);
- }
+ int tnum =
+ SQLITE_PAGENO_FROM_SPACEID_AND_INDEXID(space->def->id,
+ 0);
sqlite3VdbeAddOp3(v, OP_OpenWrite, tab_cursor,
tnum, space_ptr_reg);
struct key_def *def = key_def_dup(pk_def);
@@ -518,7 +509,7 @@ sql_generate_row_delete(struct Parse *parse, struct Table *table,
* of the DELETE statement is to fire the INSTEAD OF
* triggers).
*/
- if (table == NULL || table->pSelect == NULL) {
+ if (table == NULL || !table->def->opts.is_view) {
uint8_t p5 = 0;
sqlite3VdbeAddOp2(v, OP_Delete, cursor,
(need_update_count ? OPFLAG_NCHANGE : 0));
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index a69d38bd0..1a5e130d0 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -36,6 +36,8 @@
#include "box/coll_id_cache.h"
#include "coll.h"
#include "sqliteInt.h"
+#include "tarantoolInt.h"
+#include "box/schema.h"
#include "box/session.h"
/* Forward declarations */
@@ -2485,9 +2487,10 @@ sqlite3FindInIndex(Parse * pParse, /* Parsing context */
"USING INDEX %s FOR IN-OPERATOR",
pIdx->zName),
P4_DYNAMIC);
- emit_open_cursor(pParse, iTab,
- pIdx->tnum);
- sql_vdbe_set_p4_key_def(pParse, pIdx);
+ struct space *space =
+ space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum));
+ sql_emit_open_cursor(pParse, iTab,
+ pIdx->tnum, 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 6fd29200c..74cf51302 100644
--- a/src/box/sql/fkey.c
+++ b/src/box/sql/fkey.c
@@ -35,6 +35,7 @@
*/
#include "coll.h"
#include "sqliteInt.h"
+#include "box/schema.h"
#include "box/session.h"
#include "tarantoolInt.h"
@@ -439,9 +440,9 @@ fkLookupParent(Parse * pParse, /* Parse context */
int nCol = pFKey->nCol;
int regTemp = sqlite3GetTempRange(pParse, nCol);
int regRec = sqlite3GetTempReg(pParse);
-
- emit_open_cursor(pParse, iCur, pIdx->tnum);
- sql_vdbe_set_p4_key_def(pParse, pIdx);
+ struct space *space =
+ space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum));
+ sql_emit_open_cursor(pParse, iCur, pIdx->tnum, 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 7aba83529..0a0b3fc21 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -54,8 +54,8 @@ sqlite3OpenTable(Parse * pParse, /* Generate code into this VDBE */
Index *pPk = sqlite3PrimaryKeyIndex(pTab);
assert(pPk != 0);
assert(pPk->tnum == pTab->tnum);
- emit_open_cursor(pParse, iCur, pPk->tnum);
- sql_vdbe_set_p4_key_def(pParse, pPk);
+ struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pPk->tnum));
+ sql_emit_open_cursor(pParse, iCur, pPk->tnum, space);
VdbeComment((v, "%s", pTab->def->name));
}
@@ -107,32 +107,22 @@ sqlite3IndexAffinityStr(sqlite3 *db, Index *index)
char *
sql_index_affinity_str(struct sqlite3 * db, struct index_def *def)
{
- char *aff;
- /* The first time a column affinity string for a particular index is
- * required, it is allocated and populated here. It is then stored as
- * a member of the Index structure for subsequent use.
- *
- * The column affinity string will eventually be deleted by
- * sqliteDeleteIndex() when the Index structure itself is cleaned
- * up.
- */
- int nColumn = def->key_def->part_count;
- aff = (char *)sqlite3DbMallocRaw(0, nColumn + 1);
+ uint32_t column_count = def->key_def->part_count;
+ char *aff = (char *)sqlite3DbMallocRaw(0, column_count + 1);
if (aff == NULL) {
sqlite3OomFault(db);
- return 0;
+ return NULL;
}
- int i;
- struct space *space = space_cache_find(def->space_id);
+ struct space *space = space_by_id(def->space_id);
assert(space != NULL);
- for (i = 0; i < nColumn; i++) {
+ for (uint32_t i = 0; i < column_count; i++) {
uint32_t x = def->key_def->parts[i].fieldno;
aff[i] = space->def->fields[x].affinity;
if (aff[i] == AFFINITY_UNDEFINED)
aff[i] = 'A';
}
- aff[i] = 0;
+ aff[column_count] = '\0';
return aff;
}
@@ -1951,11 +1941,13 @@ xferOptimization(Parse * pParse, /* Parser context */
break;
}
assert(pSrcIdx);
- emit_open_cursor(pParse, iSrc, pSrcIdx->tnum);
- sql_vdbe_set_p4_key_def(pParse, pSrcIdx);
+ struct space *src_space =
+ space_by_id(SQLITE_PAGENO_TO_SPACEID(pSrcIdx->tnum));
+ sql_emit_open_cursor(pParse, iSrc, pSrcIdx->tnum, src_space);
VdbeComment((v, "%s", pSrcIdx->zName));
- emit_open_cursor(pParse, iDest, pDestIdx->tnum);
- sql_vdbe_set_p4_key_def(pParse, pDestIdx);
+ struct space *dest_space =
+ space_by_id(SQLITE_PAGENO_TO_SPACEID(pDestIdx->tnum));
+ sql_emit_open_cursor(pParse, iDest, pDestIdx->tnum, dest_space);
sqlite3VdbeChangeP5(v, OPFLAG_BULKCSR);
VdbeComment((v, "%s", pDestIdx->zName));
addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iSrc, 0);
diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index 23e16189a..e7d9723fa 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -39,6 +39,9 @@
#include <stdlib.h>
#include <string.h>
+#include "box/box.h"
+#include "box/schema.h"
+
/*
* Walk the expression tree pExpr and increase the aggregate function
* depth (the Expr.op2 field) by N on every TK_AGG_FUNCTION node.
@@ -1432,6 +1435,21 @@ resolveSelectStep(Walker * pWalker, Select * p)
return WRC_Prune;
}
+struct Table *
+sql_table_construct_from_space(const struct space *space)
+{
+ assert(space != NULL);
+ struct Table *table = calloc(1, sizeof(*table));
+ if (table == NULL) {
+ diag_set(OutOfMemory, sizeof(table), "calloc", "table");
+ return NULL;
+ }
+ table->def = space_def_dup(space->def);
+ if (table->def == NULL)
+ return NULL;
+ return table;
+}
+
/*
* This routine walks an expression tree and resolves references to
* table columns and result-set columns. At the same time, do error
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 4b5ba4d3e..9e8ed97e7 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -6107,8 +6107,9 @@ sqlite3Select(Parse * pParse, /* The parser context */
* Open the cursor, execute the OP_Count,
* close the cursor.
*/
- emit_open_cursor(pParse, cursor,
- space->def->id << 10);
+ sql_emit_open_cursor(pParse, cursor,
+ space->def->id << 10,
+ 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 878daa8df..b7e7eafa8 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2536,8 +2536,6 @@ struct SrcList {
char *zName; /* Name of the table */
char *zAlias; /* The "B" part of a "A AS B" phrase. zName is the "A" */
Table *pTab; /* An SQL table corresponding to zName */
- /* A temporary hack: need to store eph. space. */
- struct space *space;
Select *pSelect; /* A SELECT statement used in place of a table name */
int addrFillSub; /* Address of subroutine to manifest a subquery */
int regReturn; /* Register holding return address of addrFillSub */
@@ -3553,20 +3551,6 @@ sql_index_column_sort_order(Index *idx, uint32_t column);
void sqlite3EndTable(Parse *, Token *, Token *, Select *);
-/**
- * DEPRECATED. All calls to be replaced w/ sql_emit_open_cursor.
- * Create cursor which will be positioned to the space/index.
- * It makes space lookup and loads pointer to it into register,
- * which is passes to OP_OpenWrite as an argument.
- *
- * @param parse Parse context.
- * @param cursor Number of cursor to be created.
- * @param entity_id Encoded space and index ids.
- * @retval address of last opcode.
- */
-int
-emit_open_cursor(struct Parse *parse, int cursor, int entity_id);
-
/**
* Create cursor which will be positioned to the space/index.
* It makes space lookup and loads pointer to it into register,
@@ -4125,9 +4109,10 @@ int sqlite3VarintLen(u64 v);
const char *sqlite3IndexAffinityStr(sqlite3 *, Index *);
/**
- * Return a pointer to the column affinity string associated with index
- * pIdx. A column affinity string has one character for each column in
- * the table, according to the affinity of the column:
+ * Return a pointer to the column affinity string associated with
+ * given index. A column affinity string has one character for
+ * each column in the table, according to the affinity of the
+ * column:
*
* Character Column affinity
* ------------------------------
@@ -4138,12 +4123,11 @@ const char *sqlite3IndexAffinityStr(sqlite3 *, Index *);
* 'F' REAL
*
* Memory for the buffer containing the column index affinity string
- * is managed along with the rest of the Index structure. It will be
- * released when sqlite3DeleteIndex() is called.
+ * is allocated on heap.
*
* @param db Database handle.
* @param def index_def where from affinity to be extracted.
- * @retval Affinity string.
+ * @retval Allocated affinity string, or NULL on OOM.
*/
char *
sql_index_affinity_str(struct sqlite3 *db, struct index_def *def);
@@ -4262,6 +4246,20 @@ void
sqlite3SelectWrongNumTermsError(struct Parse *parse, struct Select *p);
int sqlite3MatchSpanName(const char *, const char *, const char *);
+
+/**
+ * Create wrapper around space_def of the given space.
+ * This routine is required to support original SQLite interface,
+ * which accepts struct Table and struct Index DD arguments.
+ * Both table and space def are allocated on heap.
+ *
+ * @param space Space to be prototype.
+ * @retval New instance of struct Table allocated on malloc,
+ * or NULL in case of OOM.
+ */
+struct Table *
+sql_table_construct_from_space(const struct space *space);
+
int sqlite3ResolveExprNames(NameContext *, Expr *);
int sqlite3ResolveExprListNames(NameContext *, ExprList *);
void sqlite3ResolveSelectNames(Parse *, Select *, NameContext *);
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index f017384b5..a3a5bec81 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -42,6 +42,7 @@
#include "tarantoolInt.h"
#include "vdbeInt.h"
#include "whereInt.h"
+#include "box/coll_id_cache.h"
#include "box/session.h"
#include "box/schema.h"
@@ -371,10 +372,7 @@ whereScanInit(WhereScan * pScan, /* The WhereScan object being initialized */
pScan->is_column_seen = false;
if (pIdx) {
int j = iColumn;
- if (j >= pIdx->nColumn)
- iColumn = -1;
- else
- iColumn = pIdx->aiColumn[j];
+ iColumn = pIdx->aiColumn[j];
if (iColumn >= 0) {
char affinity =
pIdx->pTable->def->fields[iColumn].affinity;
@@ -393,12 +391,30 @@ whereScanInit(WhereScan * pScan, /* The WhereScan object being initialized */
return whereScanNext(pScan);
}
+/**
+ * Analogue of whereScanInit() but also can be called for spaces
+ * created via Lua interface. This function doesn't rely on
+ * regular SQLite structures representing data dictionary.
+ *
+ * @param scan The WhereScan object being initialized.
+ * @param clause The WHERE clause to be scanned.
+ * @param cursor Cursor to scan for.
+ * @param column Column to scan for.
+ * @param op_mask Operator(s) to scan for.
+ * @param space_def Def of the space related to WHERE clause.
+ * @param key_def Def of the index to be used to satisfy WHERE
+ * clause. May be NULL.
+ *
+ * @retval Return a pointer to the first match. Return NULL if
+ * there are no matches.
+ */
static WhereTerm *
-sql_where_scan_init(struct WhereScan *scan, struct WhereClause *clause,
+where_scan_init(struct WhereScan *scan, struct WhereClause *clause,
int cursor, int column, uint32_t op_mask,
struct space_def *space_def, struct key_def *key_def)
{
- scan->pOrigWC = scan->pWC = clause;
+ scan->pOrigWC = clause;
+ scan->pWC = clause;
scan->pIdxExpr = NULL;
scan->idxaff = 0;
scan->coll = NULL;
@@ -406,11 +422,11 @@ sql_where_scan_init(struct WhereScan *scan, struct WhereClause *clause,
if (key_def != NULL) {
int j = column;
column = key_def->parts[j].fieldno;
- if (column >= 0) {
- scan->idxaff = space_def->fields[column].affinity;
- scan->coll = key_def->parts[column].coll;
- scan->is_column_seen = true;
- }
+ scan->idxaff = space_def->fields[column].affinity;
+ uint32_t coll_id = space_def->fields[column].coll_id;
+ struct coll_id *coll = coll_by_id(coll_id);
+ scan->coll = coll != NULL ? coll->coll : NULL;
+ scan->is_column_seen = true;
}
scan->opMask = op_mask;
scan->k = 0;
@@ -472,34 +488,43 @@ sqlite3WhereFindTerm(WhereClause * pWC, /* The WHERE clause to be searched */
return pResult;
}
+/**
+ * Analogue of sqlite3WhereFindTerm() but also can be called
+ * for spaces created via Lua interface. This function doesn't
+ * rely on regular SQLite structures representing data
+ * dictionary.
+ *
+ * @param where_clause The WHERE clause to be examined.
+ * @param cursor Cursor number of LHS.
+ * @param column Column number of LHS
+ * @param is_ready RHS must not overlap with this mask.
+ * @param op Mask of WO_xx values describing operator.
+ * @param space_def Def of the space related to WHERE clause.
+ * @param key_def Def of the index to be used to satisfy WHERE
+ * clause. May be NULL.
+ *
+ * @retval New struct describing WHERE term.
+ */
WhereTerm *
-sql_where_find_term(WhereClause * pWC, /* The WHERE clause to be searched */
- int iCur, /* Cursor number of LHS */
- int iColumn, /* Column number of LHS */
- Bitmask notReady, /* RHS must not overlap with this mask */
- u32 op, /* Mask of WO_xx values describing operator */
- struct space_def *space_def,
- struct key_def *key_def) /* Must be compatible with this index, if not NULL */
+sql_where_find_term(WhereClause *where_clause, int cursor, int column,
+ Bitmask is_ready, u32 op, struct space_def *space_def,
+ struct key_def *key_def)
{
- WhereTerm *pResult = 0;
- WhereTerm *p;
+ WhereTerm *result = NULL;
WhereScan scan;
-
- p = sql_where_scan_init(&scan, pWC, iCur, iColumn, op, space_def,
- key_def);
+ WhereTerm *p = where_scan_init(&scan, where_clause, cursor, column,
+ op, space_def, key_def);
op &= WO_EQ;
- while (p) {
- if ((p->prereqRight & notReady) == 0) {
- if (p->prereqRight == 0 && (p->eOperator & op) != 0) {
- testcase(p->eOperator & WO_IS);
+ while (p != NULL) {
+ if ((p->prereqRight & is_ready) == 0) {
+ if (p->prereqRight == 0 && (p->eOperator & op) != 0)
return p;
- }
- if (pResult == 0)
- pResult = p;
+ if (result == NULL)
+ result = p;
}
p = whereScanNext(&scan);
}
- return pResult;
+ return result;
}
/*
@@ -3428,7 +3453,7 @@ wherePathSatisfiesOrderBy(WhereInfo * pWInfo, /* The WHERE clause */
/* Get the column number in the table (iColumn) and sort order
* (revIdx) for the j-th column of the index.
*/
- if (pIndex && j < pIndex->nColumn) {
+ if (pIndex != NULL) {
iColumn = pIndex->aiColumn[j];
revIdx = sql_index_column_sort_order(pIndex,
j);
@@ -4096,135 +4121,125 @@ wherePathSolver(WhereInfo * pWInfo, LogEst nRowEst)
return SQLITE_OK;
}
-/*
- * Most queries use only a single table (they are not joins) and have
- * simple == constraints against indexed fields. This routine attempts
- * to plan those simple cases using much less ceremony than the
- * general-purpose query planner, and thereby yield faster sqlite3_prepare()
- * times for the common case.
+/**
+ * Attempt at finding appropriate terms in WHERE clause.
*
- * Return non-zero on success, if this query can be handled by this
- * no-frills query planner. Return zero if this query needs the
- * general-purpose query planner.
+ * @param loop The loop @where belongs to.
+ * @param where The WHERE clause to be examined..
+ * @param cursor Cursor number of LHS.
+ * @param space_def Def of the space related to WHERE clause.
+ * @param index_def Def of the index to be used to satisfy WHERE
+ * clause. May be NULL.
+ * @retval 0 on success, -1 otherwise.
*/
static int
-sql_where_shortcut(struct WhereLoopBuilder *builder)
+where_assign_loop_terms(struct WhereLoop *loop, struct WhereClause *where,
+ int cursor, struct space_def *space_def,
+ struct index_def *idx_def)
{
- WhereInfo *pWInfo;
- struct SrcList_item *pItem;
- WhereClause *pWC;
- WhereTerm *pTerm;
- WhereLoop *pLoop;
- int iCur;
- int j;
+ uint32_t column_count = idx_def != NULL ? idx_def->key_def->part_count :
+ space_def->field_count;
+ if (column_count > ArraySize(loop->aLTermSpace))
+ return -1;
+ uint32_t i;
+ for (i = 0; i < column_count; ++i) {
+ struct WhereTerm *term =
+ sql_where_find_term(where, cursor, i, 0, WO_EQ,
+ space_def, idx_def != NULL ?
+ idx_def->key_def : NULL);
+ if (term == NULL)
+ break;
+ testcase(pTerm->eOperator & WO_IS);
+ loop->aLTerm[i] = term;
+ }
+ if (i != column_count)
+ return -1;
+ loop->wsFlags = WHERE_COLUMN_EQ | WHERE_ONEROW | WHERE_INDEXED |
+ WHERE_IDX_ONLY;
+ loop->nLTerm = i;
+ loop->nEq = i;
+ loop->index_def = idx_def;
+ /* TUNING: Cost of a unique index lookup is 15. */
+ assert(39 == sqlite3LogEst(15));
+ loop->rRun = 39;
+ return 0;
+}
- pWInfo = builder->pWInfo;
- if (pWInfo->wctrlFlags & WHERE_OR_SUBCLAUSE)
+/**
+ * Most queries use only a single table (they are not joins) and
+ * have simple == constraints against indexed fields. This
+ * routine attempts to plan those simple cases using much less
+ * ceremony than the general-purpose query planner, and thereby
+ * yield faster sqlite3_prepare() times for the common case.
+ *
+ * @param builder Where-Loop Builder.
+ * @retval Return non-zero on success, i.e. if this query can be
+ * handled by this no-frills query planner. Return zero
+ * if this query needs the general-purpose query planner.
+ */
+static int
+sql_where_shortcut(struct WhereLoopBuilder *builder)
+{
+ struct WhereInfo *where_info = builder->pWInfo;
+ if (where_info->wctrlFlags & WHERE_OR_SUBCLAUSE)
return 0;
- assert(pWInfo->pTabList->nSrc >= 1);
- pItem = pWInfo->pTabList->a;
- struct space_def *space_def = pItem->pTab->def;
+ assert(where_info->pTabList->nSrc >= 1);
+ struct SrcList_item *item = where_info->pTabList->a;
+ struct space_def *space_def = item->pTab->def;
assert(space_def != NULL);
-
- if (pItem->fg.isIndexedBy)
+ if (item->fg.isIndexedBy)
return 0;
- iCur = pItem->iCursor;
- pWC = &pWInfo->sWC;
- pLoop = builder->pNew;
- pLoop->wsFlags = 0;
- pLoop->nSkip = 0;
- pTerm = sqlite3WhereFindTerm(pWC, iCur, -1, 0, WO_EQ, 0);
- if (pTerm) {
- pLoop->wsFlags = WHERE_COLUMN_EQ | WHERE_IPK | WHERE_ONEROW;
- pLoop->aLTerm[0] = pTerm;
- pLoop->nLTerm = 1;
- pLoop->nEq = 1;
- /* TUNING: Cost of a PK lookup is 10 */
- pLoop->rRun = 33; /* 33==sqlite3LogEst(10) */
+ int cursor = item->iCursor;
+ struct WhereClause *clause = &where_info->sWC;
+ struct WhereLoop *loop = builder->pNew;
+ loop->wsFlags = 0;
+ loop->nSkip = 0;
+ loop->pIndex = NULL;
+ struct WhereTerm *term = sqlite3WhereFindTerm(clause, cursor, -1, 0,
+ WO_EQ, 0);
+ if (term != NULL) {
+ loop->wsFlags = WHERE_COLUMN_EQ | WHERE_IPK | WHERE_ONEROW;
+ loop->aLTerm[0] = term;
+ loop->nLTerm = 1;
+ loop->nEq = 1;
+ /* TUNING: Cost of a PK lookup is 10. */
+ assert(33 == sqlite3LogEst(10));
+ loop->rRun = 33;
} else {
- struct space *space = space_cache_find(space_def->id);
+ assert(loop->aLTermSpace == loop->aLTerm);
+ struct space *space = space_by_id(space_def->id);
if (space != NULL) {
for (uint32_t i = 0; i < space->index_count; ++i) {
- struct index_def *idx_def;
- idx_def = space->index[i]->def;
- int opMask;
- int nIdxCol = idx_def->key_def->part_count;
- assert(pLoop->aLTermSpace == pLoop->aLTerm);
- if (!idx_def->opts.is_unique
- /* || pIdx->pPartIdxWhere != 0 */
- || nIdxCol > ArraySize(pLoop->aLTermSpace)
- )
+ struct index_def *idx_def =
+ space->index[i]->def;
+ if (!idx_def->opts.is_unique)
continue;
- opMask = WO_EQ;
- for (j = 0; j < nIdxCol; j++) {
- pTerm = sql_where_find_term(pWC, iCur,
- j, 0,
- opMask,
- space_def,
- idx_def->
- key_def);
- if (pTerm == 0)
- break;
- testcase(pTerm->eOperator & WO_IS);
- pLoop->aLTerm[j] = pTerm;
- }
- if (j != nIdxCol)
- continue;
- pLoop->wsFlags = WHERE_COLUMN_EQ |
- WHERE_ONEROW | WHERE_INDEXED |
- WHERE_IDX_ONLY;
- pLoop->nLTerm = j;
- pLoop->nEq = j;
- pLoop->pIndex = NULL;
- pLoop->index_def = idx_def;
- /* TUNING: Cost of a unique index lookup is 15 */
- pLoop->rRun = 39; /* 39==sqlite3LogEst(15) */
- break;
+ if (where_assign_loop_terms(loop, clause,
+ cursor, space_def,
+ idx_def) == 0)
+ break;
}
} else {
- /* Space is ephemeral. */
+ /* Space is ephemeral. */
assert(space_def->id == 0);
- int opMask;
- int nIdxCol = space_def->field_count;
- assert(pLoop->aLTermSpace == pLoop->aLTerm);
- if ( nIdxCol > ArraySize(pLoop->aLTermSpace))
- return 0;
- opMask = WO_EQ;
- for (j = 0; j < nIdxCol; j++) {
- pTerm = sql_where_find_term(pWC, iCur,
- j, 0,
- opMask,
- space_def,
- NULL);
- if (pTerm == NULL)
- break;
- pLoop->aLTerm[j] = pTerm;
- }
- if (j != nIdxCol)
- return 0;
- pLoop->wsFlags = WHERE_COLUMN_EQ | WHERE_ONEROW |
- WHERE_INDEXED | WHERE_IDX_ONLY;
- pLoop->nLTerm = j;
- pLoop->nEq = j;
- pLoop->pIndex = NULL;
- pLoop->index_def = NULL;
- /* TUNING: Cost of a unique index lookup is 15 */
- pLoop->rRun = 39; /* 39==sqlite3LogEst(15) */
+ where_assign_loop_terms(loop, clause, cursor,
+ space_def, NULL);
}
}
- if (pLoop->wsFlags) {
- pLoop->nOut = (LogEst) 1;
- pWInfo->a[0].pWLoop = pLoop;
- pLoop->maskSelf = sqlite3WhereGetMask(&pWInfo->sMaskSet, iCur);
- pWInfo->a[0].iTabCur = iCur;
- pWInfo->nRowOut = 1;
- if (pWInfo->pOrderBy)
- pWInfo->nOBSat = pWInfo->pOrderBy->nExpr;
- if (pWInfo->wctrlFlags & WHERE_WANT_DISTINCT) {
- pWInfo->eDistinct = WHERE_DISTINCT_UNIQUE;
+ if (loop->wsFlags) {
+ loop->nOut = (LogEst) 1;
+ where_info->a[0].pWLoop = loop;
+ loop->maskSelf = sqlite3WhereGetMask(&where_info->sMaskSet,
+ cursor);
+ where_info->a[0].iTabCur = cursor;
+ where_info->nRowOut = 1;
+ if (where_info->pOrderBy)
+ where_info->nOBSat = where_info->pOrderBy->nExpr;
+ if (where_info->wctrlFlags & WHERE_WANT_DISTINCT) {
+ where_info->eDistinct = WHERE_DISTINCT_UNIQUE;
}
#ifdef SQLITE_DEBUG
- pLoop->cId = '0';
+ loop->cId = '0';
#endif
return 1;
}
@@ -4704,8 +4719,7 @@ sqlite3WhereBegin(Parse * pParse, /* The parser context */
*/
if (idx_def == NULL && pIx == NULL) continue;
bool is_primary = (pIx != NULL && IsPrimaryKeyIndex(pIx)) ||
- (idx_def != NULL && (idx_def->iid == 0)) |
- (idx_def == NULL && pIx == NULL);
+ (idx_def != NULL && (idx_def->iid == 0));
if (is_primary
&& (wctrlFlags & WHERE_OR_SUBCLAUSE) != 0) {
/* This is one term of an OR-optimization using
@@ -4752,9 +4766,10 @@ sqlite3WhereBegin(Parse * pParse, /* The parser context */
assert(iIndexCur >= 0);
if (op) {
if (pIx != NULL) {
- emit_open_cursor(pParse, iIndexCur,
- pIx->tnum);
- sql_vdbe_set_p4_key_def(pParse, pIx);
+ struct space *space =
+ space_by_id(SQLITE_PAGENO_TO_SPACEID(pIx->tnum));
+ sql_emit_open_cursor(pParse, iIndexCur,
+ pIx->tnum, space);
} else {
sql_emit_open_cursor(pParse, iIndexCur,
idx_def->iid,
diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index 60e0ac7e4..eaab0b657 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -87,7 +87,7 @@ explainAppendTerm(StrAccum * pStr, /* The text expression being built */
assert(def != NULL);
struct space *space = space_cache_find(def->space_id);
assert(space != NULL);
- name = space->def->fields[i].name;
+ name = space->def->fields[i + iTerm].name;
}
sqlite3StrAccumAppendAll(pStr, name);
}
@@ -143,7 +143,8 @@ explainIndexRange(StrAccum * pStr, WhereLoop * pLoop)
} else {
struct space *space = space_cache_find(def->space_id);
assert(space != NULL);
- z = space->def->fields[i].name;
+ uint32_t fieldno = def->key_def->parts[i].fieldno;
+ z = space->def->fields[fieldno].name;
}
if (i)
sqlite3StrAccumAppend(pStr, " AND ", 5);
--
2.15.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 3/3] sql: implement point where for DELETE stmts
2018-06-18 2:11 ` n.pettik
@ 2018-06-18 10:44 ` Vladislav Shpilevoy
2018-06-18 10:51 ` Vladislav Shpilevoy
2018-06-18 12:29 ` n.pettik
0 siblings, 2 replies; 19+ messages in thread
From: Vladislav Shpilevoy @ 2018-06-18 10:44 UTC (permalink / raw)
To: n.pettik, tarantool-patches; +Cc: Kirill Yukhin
Hello. Thanks for the patch!
See 6 comments below.
On 18/06/2018 05:11, n.pettik wrote:
> For some reason this patch got to the trunk without review fixes.
> Raw version of this patch contains several bugs, so they led to test fails
> (sql-tap/analyze6.test.lua and few sql-tap/tkt*).
>
> I suggest my own fixes. The whole patch is at the end of letter.
>
> Branch: https://github.com/tarantool/tarantool/commits/np/gh-3463-review-fixed-for-point-delete
> Issue: https://github.com/tarantool/tarantool/issues/3463
>
>>
>>> }
>>> struct ExprList *
>>> @@ -1221,6 +1216,22 @@ emit_open_cursor(Parse *parse_context, int cursor, int entity_id)
>>> space_ptr_reg);
>>> }
>>> +int
>>> +sql_emit_open_cursor(struct Parse *parse, int cursor, int index_id, struct space *space)
>>
>> 2. Few lines above I see emit_open_cursor. Please, try to remove one of them.
>> At the worst case you can just inline emit_open_cursor in the single place of
>> its usage, and rename sql_emit_open_cursor to emit_open_cursor.
>
> Ok, I just replaced all calls of emit_open_cursor() with sql_emit_open_cursor().
> The only difference is in doing space lookup outside emit_open_cursor().
1. How about rename this function to parser_emit_open_cursor?
According to Tarantool naming convention a method name should
be called as 'object_action_subject'.
Now we have too many different prefixes of parser methods:
sqlite3, sql, parse, parser, <no_prefix>. Lets use 'parser'.
>>
>>> +}
>>> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
>>> index 2b59130..de4e0c1 100644
>>> --- a/src/box/sql/delete.c
>>> +++ b/src/box/sql/delete.c
>>> @@ -184,7 +184,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>>> memset(&nc, 0, sizeof(nc));
>>> nc.pParse = parse;
>>> if (tab_list->a[0].pTab == NULL) {
>>> - struct Table *t = malloc(sizeof(struct Table));
>>> + struct Table *t = calloc(sizeof(struct Table), 1);
>>
>> 4. It must the part of the previous patch.
>
> Refactored this way:
>
> +++ b/src/box/sql/delete.c
> @@ -104,17 +104,28 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
> strlen(tab_name));
> if (space_id == BOX_ID_NIL)
> goto delete_from_cleanup;
> + /*
> + * In this case space has been created via Lua
> + * facilities, so there is no corresponding entry
> + * in table hash. Thus, lets create simple
> + * wrapper around space_def to support interface.
> + */
> + space = space_by_id(space_id);
> + tab_list->a[0].pTab = sql_table_construct_from_space(space);
> + if (tab_list->a[0].pTab == NULL)
> + goto delete_from_cleanup;
2. As I proposed in the review, why can not we use stack struct Table here?
Just declare it at the beginning of the function and here save its pointer.
> @@ -107,32 +107,22 @@ sqlite3IndexAffinityStr(sqlite3 *db, Index *index)
> char *
> sql_index_affinity_str(struct sqlite3 * db, struct index_def *def)
> {
> - char *aff;
> - /* The first time a column affinity string for a particular index is
> - * required, it is allocated and populated here. It is then stored as
> - * a member of the Index structure for subsequent use.
> - *
> - * The column affinity string will eventually be deleted by
> - * sqliteDeleteIndex() when the Index structure itself is cleaned
> - * up.
> - */
> - int nColumn = def->key_def->part_count;
> - aff = (char *)sqlite3DbMallocRaw(0, nColumn + 1);
> + uint32_t column_count = def->key_def->part_count;
> + char *aff = (char *)sqlite3DbMallocRaw(0, column_count + 1);
> if (aff == NULL) {
> sqlite3OomFault(db);
> - return 0;
> + return NULL;
> }
3. Why not just pass db to sqlite3DbMallocRaw? It does OomFault for us.
As I see in codeAllEqualityTerms() (where this function is used)
affinity is either allocated as sqlite3DbStrDup(pParse->db) or
in sql_index_affinity_str() using sqlite3DbMallocRaw(NULL).
Different ways of allocating (with db or not) and freeing leads
to very surprising assertions sometimes.
> - int i;
> - struct space *space = space_cache_find(def->space_id);
> + struct space *space = space_by_id(def->space_id);
> assert(space != NULL);
>
> - for (i = 0; i < nColumn; i++) {
> + for (uint32_t i = 0; i < column_count; i++) {
> uint32_t x = def->key_def->parts[i].fieldno;
> aff[i] = space->def->fields[x].affinity;
> if (aff[i] == AFFINITY_UNDEFINED)
> aff[i] = 'A';
> }
> - aff[i] = 0;
> + aff[column_count] = '\0';
>
> return aff;
> }
>
> =======================================================================
>
> From 9ced467a2f33ce02ac48f875db06d3eec7d5561d Mon Sep 17 00:00:00 2001
> From: Nikita Pettik <korablev@tarantool.org>
> Date: Mon, 18 Jun 2018 00:38:55 +0300
> Subject: [PATCH] sql: review fixes for a40287051
>
> ---
> src/box/sql/analyze.c | 6 +-
> src/box/sql/build.c | 27 +----
> src/box/sql/delete.c | 45 +++----
> src/box/sql/expr.c | 9 +-
> src/box/sql/fkey.c | 7 +-
> src/box/sql/insert.c | 36 +++---
> src/box/sql/resolve.c | 18 +++
> src/box/sql/select.c | 5 +-
> src/box/sql/sqliteInt.h | 42 ++++---
> src/box/sql/where.c | 309 +++++++++++++++++++++++++-----------------------
> src/box/sql/wherecode.c | 5 +-
> 11 files changed, 256 insertions(+), 253 deletions(-)
>> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
> index 23e16189a..e7d9723fa 100644
> --- a/src/box/sql/resolve.c
> +++ b/src/box/sql/resolve.c
> @@ -39,6 +39,9 @@
> #include <stdlib.h>
> #include <string.h>
>
> +#include "box/box.h"
> +#include "box/schema.h"
4. Why do you need these headers?
> +
> /*
> * Walk the expression tree pExpr and increase the aggregate function
> * depth (the Expr.op2 field) by N on every TK_AGG_FUNCTION node.
> diff --git a/src/box/sql/where.c b/src/box/sql/where.c
> index f017384b5..a3a5bec81 100644
> --- a/src/box/sql/where.c
> +++ b/src/box/sql/where.c
> @@ -4096,135 +4121,125 @@ wherePathSolver(WhereInfo * pWInfo, LogEst nRowEst)
> return SQLITE_OK;
> }
>
> -/*
> - * Most queries use only a single table (they are not joins) and have
> - * simple == constraints against indexed fields. This routine attempts
> - * to plan those simple cases using much less ceremony than the
> - * general-purpose query planner, and thereby yield faster sqlite3_prepare()
> - * times for the common case.
> +/**
> + * Attempt at finding appropriate terms in WHERE clause.
> *
> - * Return non-zero on success, if this query can be handled by this
> - * no-frills query planner. Return zero if this query needs the
> - * general-purpose query planner.
> + * @param loop The loop @where belongs to.
> + * @param where The WHERE clause to be examined..
> + * @param cursor Cursor number of LHS.
> + * @param space_def Def of the space related to WHERE clause.
> + * @param index_def Def of the index to be used to satisfy WHERE
> + * clause. May be NULL.
> + * @retval 0 on success, -1 otherwise.
> */
> static int
> -sql_where_shortcut(struct WhereLoopBuilder *builder)
> +where_assign_loop_terms(struct WhereLoop *loop, struct WhereClause *where,
> + int cursor, struct space_def *space_def,
> + struct index_def *idx_def)
5. Why WhereClause new method has prefix 'sql_where_', but WhereLoop method has prefix
'where_'? Lets make it 'where_clause_find_term' and 'where_loop_assign_terms'.
> +/**
> + * Most queries use only a single table (they are not joins) and
> + * have simple == constraints against indexed fields. This
> + * routine attempts to plan those simple cases using much less
> + * ceremony than the general-purpose query planner, and thereby
> + * yield faster sqlite3_prepare() times for the common case.
> + *
> + * @param builder Where-Loop Builder.
> + * @retval Return non-zero on success, i.e. if this query can be
> + * handled by this no-frills query planner. Return zero
> + * if this query needs the general-purpose query planner.
> + */
> +static int
> +sql_where_shortcut(struct WhereLoopBuilder *builder)
6. Same. For example, 'where_loop_builder_shortcut'.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 3/3] sql: implement point where for DELETE stmts
2018-06-18 10:44 ` Vladislav Shpilevoy
@ 2018-06-18 10:51 ` Vladislav Shpilevoy
2018-06-18 12:29 ` n.pettik
1 sibling, 0 replies; 19+ messages in thread
From: Vladislav Shpilevoy @ 2018-06-18 10:51 UTC (permalink / raw)
To: n.pettik, tarantool-patches; +Cc: Kirill Yukhin
Forgot to say: I have pushed some
other minor fixes on the branch.
On 18/06/2018 13:44, Vladislav Shpilevoy wrote:
> Hello. Thanks for the patch!
>
> See 6 comments below.
>
> On 18/06/2018 05:11, n.pettik wrote:
>> For some reason this patch got to the trunk without review fixes.
>> Raw version of this patch contains several bugs, so they led to test fails
>> (sql-tap/analyze6.test.lua and few sql-tap/tkt*).
>>
>> I suggest my own fixes. The whole patch is at the end of letter.
>>
>> Branch: https://github.com/tarantool/tarantool/commits/np/gh-3463-review-fixed-for-point-delete
>> Issue: https://github.com/tarantool/tarantool/issues/3463
>>
>>>
>>>> }
>>>> struct ExprList *
>>>> @@ -1221,6 +1216,22 @@ emit_open_cursor(Parse *parse_context, int cursor, int entity_id)
>>>> space_ptr_reg);
>>>> }
>>>> +int
>>>> +sql_emit_open_cursor(struct Parse *parse, int cursor, int index_id, struct space *space)
>>>
>>> 2. Few lines above I see emit_open_cursor. Please, try to remove one of them.
>>> At the worst case you can just inline emit_open_cursor in the single place of
>>> its usage, and rename sql_emit_open_cursor to emit_open_cursor.
>>
>> Ok, I just replaced all calls of emit_open_cursor() with sql_emit_open_cursor().
>> The only difference is in doing space lookup outside emit_open_cursor().
>
> 1. How about rename this function to parser_emit_open_cursor?
> According to Tarantool naming convention a method name should
> be called as 'object_action_subject'.
>
> Now we have too many different prefixes of parser methods:
> sqlite3, sql, parse, parser, <no_prefix>. Lets use 'parser'.
>
>>>
>>>> +}
>>>> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
>>>> index 2b59130..de4e0c1 100644
>>>> --- a/src/box/sql/delete.c
>>>> +++ b/src/box/sql/delete.c
>>>> @@ -184,7 +184,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>>>> memset(&nc, 0, sizeof(nc));
>>>> nc.pParse = parse;
>>>> if (tab_list->a[0].pTab == NULL) {
>>>> - struct Table *t = malloc(sizeof(struct Table));
>>>> + struct Table *t = calloc(sizeof(struct Table), 1);
>>>
>>> 4. It must the part of the previous patch.
>>
>> Refactored this way:
>>
>> +++ b/src/box/sql/delete.c
>> @@ -104,17 +104,28 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>> strlen(tab_name));
>> if (space_id == BOX_ID_NIL)
>> goto delete_from_cleanup;
>> + /*
>> + * In this case space has been created via Lua
>> + * facilities, so there is no corresponding entry
>> + * in table hash. Thus, lets create simple
>> + * wrapper around space_def to support interface.
>> + */
>> + space = space_by_id(space_id);
>> + tab_list->a[0].pTab = sql_table_construct_from_space(space);
>> + if (tab_list->a[0].pTab == NULL)
>> + goto delete_from_cleanup;
>
> 2. As I proposed in the review, why can not we use stack struct Table here?
> Just declare it at the beginning of the function and here save its pointer.
>
>
>> @@ -107,32 +107,22 @@ sqlite3IndexAffinityStr(sqlite3 *db, Index *index)
>> char *
>> sql_index_affinity_str(struct sqlite3 * db, struct index_def *def)
>> {
>> - char *aff;
>> - /* The first time a column affinity string for a particular index is
>> - * required, it is allocated and populated here. It is then stored as
>> - * a member of the Index structure for subsequent use.
>> - *
>> - * The column affinity string will eventually be deleted by
>> - * sqliteDeleteIndex() when the Index structure itself is cleaned
>> - * up.
>> - */
>> - int nColumn = def->key_def->part_count;
>> - aff = (char *)sqlite3DbMallocRaw(0, nColumn + 1);
>> + uint32_t column_count = def->key_def->part_count;
>> + char *aff = (char *)sqlite3DbMallocRaw(0, column_count + 1);
>> if (aff == NULL) {
>> sqlite3OomFault(db);
>> - return 0;
>> + return NULL;
>> }
>
> 3. Why not just pass db to sqlite3DbMallocRaw? It does OomFault for us.
>
> As I see in codeAllEqualityTerms() (where this function is used)
> affinity is either allocated as sqlite3DbStrDup(pParse->db) or
> in sql_index_affinity_str() using sqlite3DbMallocRaw(NULL).
>
> Different ways of allocating (with db or not) and freeing leads
> to very surprising assertions sometimes.
>
>> - int i;
>> - struct space *space = space_cache_find(def->space_id);
>> + struct space *space = space_by_id(def->space_id);
>> assert(space != NULL);
>> - for (i = 0; i < nColumn; i++) {
>> + for (uint32_t i = 0; i < column_count; i++) {
>> uint32_t x = def->key_def->parts[i].fieldno;
>> aff[i] = space->def->fields[x].affinity;
>> if (aff[i] == AFFINITY_UNDEFINED)
>> aff[i] = 'A';
>> }
>> - aff[i] = 0;
>> + aff[column_count] = '\0';
>> return aff;
>> }
>>
>> =======================================================================
>>
>> From 9ced467a2f33ce02ac48f875db06d3eec7d5561d Mon Sep 17 00:00:00 2001
>> From: Nikita Pettik <korablev@tarantool.org>
>> Date: Mon, 18 Jun 2018 00:38:55 +0300
>> Subject: [PATCH] sql: review fixes for a40287051
>>
>> ---
>> src/box/sql/analyze.c | 6 +-
>> src/box/sql/build.c | 27 +----
>> src/box/sql/delete.c | 45 +++----
>> src/box/sql/expr.c | 9 +-
>> src/box/sql/fkey.c | 7 +-
>> src/box/sql/insert.c | 36 +++---
>> src/box/sql/resolve.c | 18 +++
>> src/box/sql/select.c | 5 +-
>> src/box/sql/sqliteInt.h | 42 ++++---
>> src/box/sql/where.c | 309 +++++++++++++++++++++++++-----------------------
>> src/box/sql/wherecode.c | 5 +-
>> 11 files changed, 256 insertions(+), 253 deletions(-)
>>> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
>> index 23e16189a..e7d9723fa 100644
>> --- a/src/box/sql/resolve.c
>> +++ b/src/box/sql/resolve.c
>> @@ -39,6 +39,9 @@
>> #include <stdlib.h>
>> #include <string.h>
>> +#include "box/box.h"
>> +#include "box/schema.h"
>
> 4. Why do you need these headers?
>
>> +
>> /*
>> * Walk the expression tree pExpr and increase the aggregate function
>> * depth (the Expr.op2 field) by N on every TK_AGG_FUNCTION node.
>> diff --git a/src/box/sql/where.c b/src/box/sql/where.c
>> index f017384b5..a3a5bec81 100644
>> --- a/src/box/sql/where.c
>> +++ b/src/box/sql/where.c
>> @@ -4096,135 +4121,125 @@ wherePathSolver(WhereInfo * pWInfo, LogEst nRowEst)
>> return SQLITE_OK;
>> }
>> -/*
>> - * Most queries use only a single table (they are not joins) and have
>> - * simple == constraints against indexed fields. This routine attempts
>> - * to plan those simple cases using much less ceremony than the
>> - * general-purpose query planner, and thereby yield faster sqlite3_prepare()
>> - * times for the common case.
>> +/**
>> + * Attempt at finding appropriate terms in WHERE clause.
>> *
>> - * Return non-zero on success, if this query can be handled by this
>> - * no-frills query planner. Return zero if this query needs the
>> - * general-purpose query planner.
>> + * @param loop The loop @where belongs to.
>> + * @param where The WHERE clause to be examined..
>> + * @param cursor Cursor number of LHS.
>> + * @param space_def Def of the space related to WHERE clause.
>> + * @param index_def Def of the index to be used to satisfy WHERE
>> + * clause. May be NULL.
>> + * @retval 0 on success, -1 otherwise.
>> */
>> static int
>> -sql_where_shortcut(struct WhereLoopBuilder *builder)
>> +where_assign_loop_terms(struct WhereLoop *loop, struct WhereClause *where,
>> + int cursor, struct space_def *space_def,
>> + struct index_def *idx_def)
>
> 5. Why WhereClause new method has prefix 'sql_where_', but WhereLoop method has prefix
> 'where_'? Lets make it 'where_clause_find_term' and 'where_loop_assign_terms'.
>
>> +/**
>> + * Most queries use only a single table (they are not joins) and
>> + * have simple == constraints against indexed fields. This
>> + * routine attempts to plan those simple cases using much less
>> + * ceremony than the general-purpose query planner, and thereby
>> + * yield faster sqlite3_prepare() times for the common case.
>> + *
>> + * @param builder Where-Loop Builder.
>> + * @retval Return non-zero on success, i.e. if this query can be
>> + * handled by this no-frills query planner. Return zero
>> + * if this query needs the general-purpose query planner.
>> + */
>> +static int
>> +sql_where_shortcut(struct WhereLoopBuilder *builder)
>
> 6. Same. For example, 'where_loop_builder_shortcut'.
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 3/3] sql: implement point where for DELETE stmts
2018-06-18 10:44 ` Vladislav Shpilevoy
2018-06-18 10:51 ` Vladislav Shpilevoy
@ 2018-06-18 12:29 ` n.pettik
2018-06-18 12:40 ` Vladislav Shpilevoy
1 sibling, 1 reply; 19+ messages in thread
From: n.pettik @ 2018-06-18 12:29 UTC (permalink / raw)
To: tarantool-patches; +Cc: Vladislav Shpilevoy
Thanks for fixes, I have squashed them.
> 1. How about rename this function to parser_emit_open_cursor?
> According to Tarantool naming convention a method name should
> be called as 'object_action_subject'.
>
> Now we have too many different prefixes of parser methods:
> sqlite3, sql, parse, parser, <no_prefix>. Lets use 'parser’.
Ok, but I decided to call it vdbe_emit_open_cursor() since all staff connected
with code generation rather belongs to vdbe:
+++ b/src/box/sql/analyze.c
@@ -178,7 +178,7 @@ openStatTable(Parse * pParse, /* Parsing context */
for (i = 0; aTable[i]; i++) {
struct space *space =
space_by_id(SQLITE_PAGENO_TO_SPACEID(aRoot[i]));
- sql_emit_open_cursor(pParse, iStatCur + i, aRoot[i], space);
+ vdbe_emit_open_cursor(pParse, iStatCur + i, aRoot[i], space);
+++ b/src/box/sql/build.c
@@ -1181,8 +1181,8 @@ space_checks_expr_list(uint32_t space_id)
}
int
-sql_emit_open_cursor(struct Parse *parse_context, int cursor, int index_id,
- struct space *space)
+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;
@@ -2651,7 +2651,7 @@ 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));
- sql_emit_open_cursor(pParse, iIdx, tnum, space);
+ vdbe_emit_open_cursor(pParse, iIdx, tnum, space);
+++ b/src/box/sql/expr.c
@@ -2489,8 +2489,8 @@ sqlite3FindInIndex(Parse * pParse, /* Parsing context */
P4_DYNAMIC);
struct space *space =
space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum));
- sql_emit_open_cursor(pParse, iTab,
- pIdx->tnum, space);
+ vdbe_emit_open_cursor(pParse, iTab,
+ pIdx->tnum, space);
+++ b/src/box/sql/insert.c
@@ -55,7 +55,7 @@ sqlite3OpenTable(Parse * pParse, /* Generate code into this VDBE */
assert(pPk != 0);
assert(pPk->tnum == pTab->tnum);
struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pPk->tnum));
- sql_emit_open_cursor(pParse, iCur, pPk->tnum, space);
+ vdbe_emit_open_cursor(pParse, iCur, pPk->tnum, space);
@@ -1943,11 +1941,11 @@ xferOptimization(Parse * pParse, /* Parser context */
assert(pSrcIdx);
struct space *src_space =
space_by_id(SQLITE_PAGENO_TO_SPACEID(pSrcIdx->tnum));
- sql_emit_open_cursor(pParse, iSrc, pSrcIdx->tnum, src_space);
+ vdbe_emit_open_cursor(pParse, iSrc, pSrcIdx->tnum, src_space);
VdbeComment((v, "%s", pSrcIdx->zName));
struct space *dest_space =
space_by_id(SQLITE_PAGENO_TO_SPACEID(pDestIdx->tnum));
- sql_emit_open_cursor(pParse, iDest, pDestIdx->tnum, dest_space);
+ vdbe_emit_open_cursor(pParse, iDest, pDestIdx->tnum, dest_space);
+++ b/src/box/sql/select.c
@@ -6107,9 +6107,9 @@ sqlite3Select(Parse * pParse, /* The parser context */
* Open the cursor, execute the OP_Count,
* close the cursor.
*/
- sql_emit_open_cursor(pParse, cursor,
- space->def->id << 10,
- space);
+ vdbe_emit_open_cursor(pParse, cursor,
+ space->def->id << 10,
+ space);
+++ b/src/box/sql/sqliteInt.h
@@ -3564,8 +3564,8 @@ void sqlite3EndTable(Parse *, Token *, Token *, Select *);
* @retval address of last opcode.
*/
int
-sql_emit_open_cursor(struct Parse *parse, int cursor, int index_id,
- struct space *space);
+vdbe_emit_open_cursor(struct Parse *parse, int cursor, int index_id,
+ struct space *space);
>>>
>>>> +}
>>>> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
>>>> index 2b59130..de4e0c1 100644
>>>> --- a/src/box/sql/delete.c
>>>> +++ b/src/box/sql/delete.c
>>>> @@ -184,7 +184,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>>>> memset(&nc, 0, sizeof(nc));
>>>> nc.pParse = parse;
>>>> if (tab_list->a[0].pTab == NULL) {
>>>> - struct Table *t = malloc(sizeof(struct Table));
>>>> + struct Table *t = calloc(sizeof(struct Table), 1);
>>>
>>> 4. It must the part of the previous patch.
>> Refactored this way:
>> +++ b/src/box/sql/delete.c
>> @@ -104,17 +104,28 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>> strlen(tab_name));
>> if (space_id == BOX_ID_NIL)
>> goto delete_from_cleanup;
>> + /*
>> + * In this case space has been created via Lua
>> + * facilities, so there is no corresponding entry
>> + * in table hash. Thus, lets create simple
>> + * wrapper around space_def to support interface.
>> + */
>> + space = space_by_id(space_id);
>> + tab_list->a[0].pTab = sql_table_construct_from_space(space);
>> + if (tab_list->a[0].pTab == NULL)
>> + goto delete_from_cleanup;
>
> 2. As I proposed in the review, why can not we use stack struct Table here?
> Just declare it at the beginning of the function and here save its pointer.
Well, OK it is possible:
+++ b/src/box/sql/delete.c
@@ -111,9 +111,12 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
* wrapper around space_def to support interface.
*/
space = space_by_id(space_id);
- tab_list->a[0].pTab = sql_table_construct_from_space(space);
- if (tab_list->a[0].pTab == NULL)
- goto delete_from_cleanup;
+ struct Table tmp_tab;
+ memset(&tmp_tab, 0, sizeof(tmp_tab));
+ tmp_tab.def = space->def;
+ /* Prevent from freeing memory in DeleteTable. */
+ tmp_tab.nTabRef = 2;
+ tab_list->a[0].pTab = &tmp_tab;
-struct Table *
-sql_table_construct_from_space(const struct space *space)
-{
- assert(space != NULL);
- struct Table *table = calloc(1, sizeof(*table));
- if (table == NULL) {
- diag_set(OutOfMemory, sizeof(table), "calloc", "table");
- return NULL;
- }
- table->def = space_def_dup(space->def);
- if (table->def == NULL)
- return NULL;
- return table;
-}
-
-
-/**
- * Create wrapper around space_def of the given space.
- * This routine is required to support original SQLite interface,
- * which accepts struct Table and struct Index DD arguments.
- * Both table and space def are allocated on heap.
- *
- * @param space Space to be prototype.
- * @retval New instance of struct Table allocated on malloc,
- * or NULL in case of OOM.
- */
-struct Table *
-sql_table_construct_from_space(const struct space *space);
-
>
>
>> @@ -107,32 +107,22 @@ sqlite3IndexAffinityStr(sqlite3 *db, Index *index)
>> char *
>> sql_index_affinity_str(struct sqlite3 * db, struct index_def *def)
>> {
>> - char *aff;
>> - /* The first time a column affinity string for a particular index is
>> - * required, it is allocated and populated here. It is then stored as
>> - * a member of the Index structure for subsequent use.
>> - *
>> - * The column affinity string will eventually be deleted by
>> - * sqliteDeleteIndex() when the Index structure itself is cleaned
>> - * up.
>> - */
>> - int nColumn = def->key_def->part_count;
>> - aff = (char *)sqlite3DbMallocRaw(0, nColumn + 1);
>> + uint32_t column_count = def->key_def->part_count;
>> + char *aff = (char *)sqlite3DbMallocRaw(0, column_count + 1);
>> if (aff == NULL) {
>> sqlite3OomFault(db);
>> - return 0;
>> + return NULL;
>> }
>
> 3. Why not just pass db to sqlite3DbMallocRaw? It does OomFault for us.
Ok:
@@ -108,11 +108,9 @@ char *
sql_index_affinity_str(struct sqlite3 *db, struct index_def *def)
{
uint32_t column_count = def->key_def->part_count;
- char *aff = (char *)sqlite3DbMallocRaw(0, column_count + 1);
- if (aff == NULL) {
- sqlite3OomFault(db);
+ char *aff = (char *)sqlite3DbMallocRaw(db, column_count + 1);
+ if (aff == NULL)
return NULL;
- }
>> 11 files changed, 256 insertions(+), 253 deletions(-)
>>> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
>> index 23e16189a..e7d9723fa 100644
>> --- a/src/box/sql/resolve.c
>> +++ b/src/box/sql/resolve.c
>> @@ -39,6 +39,9 @@
>> #include <stdlib.h>
>> #include <string.h>
>> +#include "box/box.h"
>> +#include "box/schema.h"
>
> 4. Why do you need these headers?
Looks like I forgot to remove them. Anyway, in your fixes you removed them. Thx.
>> */
>> static int
>> -sql_where_shortcut(struct WhereLoopBuilder *builder)
>> +where_assign_loop_terms(struct WhereLoop *loop, struct WhereClause *where,
>> + int cursor, struct space_def *space_def,
>> + struct index_def *idx_def)
>
> 5. Why WhereClause new method has prefix 'sql_where_', but WhereLoop method has prefix
> 'where_'? Lets make it 'where_clause_find_term' and 'where_loop_assign_terms’.
-sql_where_find_term(struct WhereClause *where_clause, int cursor, int column,
- Bitmask is_ready, u32 op, struct space_def *space_def,
- struct key_def *key_def)
+where_clase_find_term(struct WhereClause *where_clause, int cursor, int column,
+ Bitmask is_ready, u32 op, struct space_def *space_def,
+ struct key_def *key_def)
@@ -4133,7 +4133,7 @@ wherePathSolver(WhereInfo * pWInfo, LogEst nRowEst)
* @retval 0 on success, -1 otherwise.
*/
static int
-where_assign_loop_terms(struct WhereLoop *loop, struct WhereClause *where,
+where_loop_assign_terms(struct WhereLoop *loop, struct WhereClause *where,
int cursor, struct space_def *space_def,
struct index_def *idx_def)
@@ -4144,9 +4144,9 @@ where_assign_loop_terms(struct WhereLoop *loop, struct WhereClause *where,
uint32_t i;
for (i = 0; i < column_count; ++i) {
struct WhereTerm *term =
- sql_where_find_term(where, cursor, i, 0, WO_EQ,
- space_def, idx_def != NULL ?
- idx_def->key_def : NULL);
+ where_clause_find_term(where, cursor, i, 0, WO_EQ,
+ space_def, idx_def != NULL ?
+ idx_def->key_def : NULL);
@@ -4178,7 +4178,7 @@ where_assign_loop_terms(struct WhereLoop *loop, struct WhereClause *where,
* if this query needs the general-purpose query planner.
*/
static int
-sql_where_shortcut(struct WhereLoopBuilder *builder)
+where_loop_builder_shortcut(struct WhereLoopBuilder *builder)
{
struct WhereInfo *where_info = builder->pWInfo;
if (where_info->wctrlFlags & WHERE_OR_SUBCLAUSE)
@@ -4214,7 +4214,7 @@ sql_where_shortcut(struct WhereLoopBuilder *builder)
space->index[i]->def;
if (!idx_def->opts.is_unique)
continue;
- if (where_assign_loop_terms(loop, clause,
+ if (where_loop_assign_terms(loop, clause,
cursor, space_def,
idx_def) == 0)
break;
@@ -4222,7 +4222,7 @@ sql_where_shortcut(struct WhereLoopBuilder *builder)
} else {
/* Space is ephemeral. */
assert(space_def->id == 0);
- where_assign_loop_terms(loop, clause, cursor,
+ where_loop_assign_terms(loop, clause, cursor,
space_def, NULL);
@@ -4537,7 +4537,7 @@ sqlite3WhereBegin(Parse * pParse, /* The parser context */
}
#endif
- if (nTabList != 1 || sql_where_shortcut(&sWLB) == 0) {
+ if (nTabList != 1 || where_loop_builder_shortcut(&sWLB) == 0) {
>
>> +/**
>> + * Most queries use only a single table (they are not joins) and
>> + * have simple == constraints against indexed fields. This
>> + * routine attempts to plan those simple cases using much less
>> + * ceremony than the general-purpose query planner, and thereby
>> + * yield faster sqlite3_prepare() times for the common case.
>> + *
>> + * @param builder Where-Loop Builder.
>> + * @retval Return non-zero on success, i.e. if this query can be
>> + * handled by this no-frills query planner. Return zero
>> + * if this query needs the general-purpose query planner.
>> + */
>> +static int
>> +sql_where_shortcut(struct WhereLoopBuilder *builder)
>
> 6. Same. For example, 'where_loop_builder_shortcut’.
Fixed. See code above.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 3/3] sql: implement point where for DELETE stmts
2018-06-18 12:29 ` n.pettik
@ 2018-06-18 12:40 ` Vladislav Shpilevoy
2018-06-18 14:00 ` n.pettik
0 siblings, 1 reply; 19+ messages in thread
From: Vladislav Shpilevoy @ 2018-06-18 12:40 UTC (permalink / raw)
To: n.pettik, tarantool-patches
Thanks for the fixes! See 2 comments below.
>> 2. As I proposed in the review, why can not we use stack struct Table here?
>> Just declare it at the beginning of the function and here save its pointer.
>
> Well, OK it is possible:
>
> +++ b/src/box/sql/delete.c
> @@ -111,9 +111,12 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
> * wrapper around space_def to support interface.
> */
> space = space_by_id(space_id);
> - tab_list->a[0].pTab = sql_table_construct_from_space(space);
> - if (tab_list->a[0].pTab == NULL)
> - goto delete_from_cleanup;
> + struct Table tmp_tab;
> + memset(&tmp_tab, 0, sizeof(tmp_tab));
> + tmp_tab.def = space->def;
> + /* Prevent from freeing memory in DeleteTable. */
> + tmp_tab.nTabRef = 2;
> + tab_list->a[0].pTab = &tmp_tab;
1. Are you sure you can save the pointer to the stack value and then
use it out of the declaration context?
Here tmp_tab is declared inside {} block, but pointer on it is
used out the former.
>>> 11 files changed, 256 insertions(+), 253 deletions(-)
>>>> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
>>> index 23e16189a..e7d9723fa 100644
>>> --- a/src/box/sql/resolve.c
>>> +++ b/src/box/sql/resolve.c
>>> @@ -39,6 +39,9 @@
>>> #include <stdlib.h>
>>> #include <string.h>
>>> +#include "box/box.h"
>>> +#include "box/schema.h"
>>
>> 4. Why do you need these headers?
>
> Looks like I forgot to remove them. Anyway, in your fixes you removed them. Thx.
>
>>> */
>>> static int
>>> -sql_where_shortcut(struct WhereLoopBuilder *builder)
>>> +where_assign_loop_terms(struct WhereLoop *loop, struct WhereClause *where,
>>> + int cursor, struct space_def *space_def,
>>> + struct index_def *idx_def)
>>
>> 5. Why WhereClause new method has prefix 'sql_where_', but WhereLoop method has prefix
>> 'where_'? Lets make it 'where_clause_find_term' and 'where_loop_assign_terms’.
>
> -sql_where_find_term(struct WhereClause *where_clause, int cursor, int column,
> - Bitmask is_ready, u32 op, struct space_def *space_def,
> - struct key_def *key_def)
> +where_clase_find_term(struct WhereClause *where_clause, int cursor, int column,
2. clase - clause. Else the patch can not be built.
> + Bitmask is_ready, u32 op, struct space_def *space_def,
> + struct key_def *key_def)
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 3/3] sql: implement point where for DELETE stmts
2018-06-18 12:40 ` Vladislav Shpilevoy
@ 2018-06-18 14:00 ` n.pettik
2018-06-18 14:17 ` Vladislav Shpilevoy
0 siblings, 1 reply; 19+ messages in thread
From: n.pettik @ 2018-06-18 14:00 UTC (permalink / raw)
To: tarantool-patches; +Cc: Vladislav Shpilevoy
> On 18 Jun 2018, at 15:40, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>
> Thanks for the fixes! See 2 comments below.
>
>>> 2. As I proposed in the review, why can not we use stack struct Table here?
>>> Just declare it at the beginning of the function and here save its pointer.
>> Well, OK it is possible:
>> +++ b/src/box/sql/delete.c
>> @@ -111,9 +111,12 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>> * wrapper around space_def to support interface.
>> */
>> space = space_by_id(space_id);
>> - tab_list->a[0].pTab = sql_table_construct_from_space(space);
>> - if (tab_list->a[0].pTab == NULL)
>> - goto delete_from_cleanup;
>> + struct Table tmp_tab;
>> + memset(&tmp_tab, 0, sizeof(tmp_tab));
>> + tmp_tab.def = space->def;
>> + /* Prevent from freeing memory in DeleteTable. */
>> + tmp_tab.nTabRef = 2;
>> + tab_list->a[0].pTab = &tmp_tab;
>
> 1. Are you sure you can save the pointer to the stack value and then
> use it out of the declaration context?
>
> Here tmp_tab is declared inside {} block, but pointer on it is
> used out the former.
Well, I'm not so sure about that..
@@ -99,6 +99,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
*/
bool is_complex = false;
const char *tab_name = tab_list->a->zName;
+ struct Table tmp_tab;
if (sqlite3LocateTable(parse, LOCATE_NOERR, tab_name) == NULL) {
space_id = box_space_id_by_name(tab_name,
strlen(tab_name));
@@ -111,7 +112,6 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
* wrapper around space_def to support interface.
*/
space = space_by_id(space_id);
- struct Table tmp_tab;
>>> 5. Why WhereClause new method has prefix 'sql_where_', but WhereLoop method has prefix
>>> 'where_'? Lets make it 'where_clause_find_term' and 'where_loop_assign_terms’.
>> -sql_where_find_term(struct WhereClause *where_clause, int cursor, int column,
>> - Bitmask is_ready, u32 op, struct space_def *space_def,
>> - struct key_def *key_def)
>> +where_clase_find_term(struct WhereClause *where_clause, int cursor, int column,
>
> 2. clase - clause. Else the patch can not be built.
Oops, I am sorry:
@@ -506,9 +506,9 @@ sqlite3WhereFindTerm(WhereClause * pWC, /* The WHERE clause to be searched */
* @retval New struct describing WHERE term.
*/
static inline struct WhereTerm *
-where_clase_find_term(struct WhereClause *where_clause, int cursor, int column,
- Bitmask is_ready, u32 op, struct space_def *space_def,
- struct key_def *key_def)
+where_clause_find_term(struct WhereClause *where_clause, int cursor, int column,
+ Bitmask is_ready, u32 op, struct space_def *space_def,
+ struct key_def *key_def)
^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 3/3] sql: implement point where for DELETE stmts
2018-06-18 14:00 ` n.pettik
@ 2018-06-18 14:17 ` Vladislav Shpilevoy
2018-06-19 8:03 ` Kirill Yukhin
0 siblings, 1 reply; 19+ messages in thread
From: Vladislav Shpilevoy @ 2018-06-18 14:17 UTC (permalink / raw)
To: tarantool-patches, n.pettik
Now LGTM.
On 18/06/2018 17:00, n.pettik wrote:
>
>> On 18 Jun 2018, at 15:40, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>>
>> Thanks for the fixes! See 2 comments below.
>>
>>>> 2. As I proposed in the review, why can not we use stack struct Table here?
>>>> Just declare it at the beginning of the function and here save its pointer.
>>> Well, OK it is possible:
>>> +++ b/src/box/sql/delete.c
>>> @@ -111,9 +111,12 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>>> * wrapper around space_def to support interface.
>>> */
>>> space = space_by_id(space_id);
>>> - tab_list->a[0].pTab = sql_table_construct_from_space(space);
>>> - if (tab_list->a[0].pTab == NULL)
>>> - goto delete_from_cleanup;
>>> + struct Table tmp_tab;
>>> + memset(&tmp_tab, 0, sizeof(tmp_tab));
>>> + tmp_tab.def = space->def;
>>> + /* Prevent from freeing memory in DeleteTable. */
>>> + tmp_tab.nTabRef = 2;
>>> + tab_list->a[0].pTab = &tmp_tab;
>>
>> 1. Are you sure you can save the pointer to the stack value and then
>> use it out of the declaration context?
>>
>> Here tmp_tab is declared inside {} block, but pointer on it is
>> used out the former.
>
> Well, I'm not so sure about that..
>
> @@ -99,6 +99,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
> */
> bool is_complex = false;
> const char *tab_name = tab_list->a->zName;
> + struct Table tmp_tab;
> if (sqlite3LocateTable(parse, LOCATE_NOERR, tab_name) == NULL) {
> space_id = box_space_id_by_name(tab_name,
> strlen(tab_name));
> @@ -111,7 +112,6 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
> * wrapper around space_def to support interface.
> */
> space = space_by_id(space_id);
> - struct Table tmp_tab;
>
>>>> 5. Why WhereClause new method has prefix 'sql_where_', but WhereLoop method has prefix
>>>> 'where_'? Lets make it 'where_clause_find_term' and 'where_loop_assign_terms’.
>>> -sql_where_find_term(struct WhereClause *where_clause, int cursor, int column,
>>> - Bitmask is_ready, u32 op, struct space_def *space_def,
>>> - struct key_def *key_def)
>>> +where_clase_find_term(struct WhereClause *where_clause, int cursor, int column,
>>
>> 2. clase - clause. Else the patch can not be built.
>
> Oops, I am sorry:
>
> @@ -506,9 +506,9 @@ sqlite3WhereFindTerm(WhereClause * pWC, /* The WHERE clause to be searched */
> * @retval New struct describing WHERE term.
> */
> static inline struct WhereTerm *
> -where_clase_find_term(struct WhereClause *where_clause, int cursor, int column,
> - Bitmask is_ready, u32 op, struct space_def *space_def,
> - struct key_def *key_def)
> +where_clause_find_term(struct WhereClause *where_clause, int cursor, int column,
> + Bitmask is_ready, u32 op, struct space_def *space_def,
> + struct key_def *key_def)
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [tarantool-patches] Re: [PATCH 3/3] sql: implement point where for DELETE stmts
2018-06-18 14:17 ` Vladislav Shpilevoy
@ 2018-06-19 8:03 ` Kirill Yukhin
0 siblings, 0 replies; 19+ messages in thread
From: Kirill Yukhin @ 2018-06-19 8:03 UTC (permalink / raw)
To: tarantool-patches; +Cc: n.pettik
Hello Nikita, Vlad,
On 18 июн 17:17, Vladislav Shpilevoy wrote:
> Now LGTM.
Patch checked into 2.0 branch.
--
Regards, Kirill Yukhin
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2018-06-19 8:07 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-01 15:16 [tarantool-patches] [PATCH 0/3] sql: implement point where for DELETE stmts Kirill Yukhin
2018-06-01 15:16 ` [tarantool-patches] [PATCH 1/3] sql: fetch primary index for affinity only Kirill Yukhin
2018-06-01 18:00 ` [tarantool-patches] " Vladislav Shpilevoy
2018-06-07 12:03 ` Kirill Yukhin
2018-06-07 15:01 ` Vladislav Shpilevoy
2018-06-08 8:25 ` Kirill Yukhin
2018-06-01 15:16 ` [tarantool-patches] [PATCH 2/3] sql: remove expressions from SQL indexes Kirill Yukhin
2018-06-01 18:00 ` [tarantool-patches] " Vladislav Shpilevoy
2018-06-01 15:16 ` [tarantool-patches] [PATCH 3/3] sql: implement point where for DELETE stmts Kirill Yukhin
2018-06-01 18:00 ` [tarantool-patches] " Vladislav Shpilevoy
2018-06-18 2:11 ` n.pettik
2018-06-18 10:44 ` Vladislav Shpilevoy
2018-06-18 10:51 ` Vladislav Shpilevoy
2018-06-18 12:29 ` n.pettik
2018-06-18 12:40 ` Vladislav Shpilevoy
2018-06-18 14:00 ` n.pettik
2018-06-18 14:17 ` Vladislav Shpilevoy
2018-06-19 8:03 ` Kirill Yukhin
2018-06-14 12:41 ` [tarantool-patches] Re: [PATCH 0/3] " Kirill Yukhin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox