[tarantool-patches] Re: [PATCH 3/3] sql: implement point where for DELETE stmts
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Jun 1 21:00:51 MSK 2018
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) {
More information about the Tarantool-patches
mailing list