From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id E7B3F29275 for ; Fri, 1 Jun 2018 14:00:54 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id GQ00iUpAXWGV for ; Fri, 1 Jun 2018 14:00:54 -0400 (EDT) Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 8194329278 for ; Fri, 1 Jun 2018 14:00:54 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 3/3] sql: implement point where for DELETE stmts References: From: Vladislav Shpilevoy Message-ID: <16d25107-35e6-d6db-d0d5-01a47f1d3909@tarantool.org> Date: Fri, 1 Jun 2018 21:00:51 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Kirill Yukhin Cc: tarantool-patches@freelists.org 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) {