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 6D32A25322 for ; Thu, 17 May 2018 10:23:57 -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 VkBs6xnU5IJs for ; Thu, 17 May 2018 10:23:57 -0400 (EDT) Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [94.100.177.112]) (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 059BF25319 for ; Thu, 17 May 2018 10:23:56 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 2/2] sql: refactor delete routines References: <36b38ab497e52e9552d07bc98e11ec52697ce1e7.1526483543.git.kyukhin@tarantool.org> <20180516162905.krtf6mksrndo7equ@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Thu, 17 May 2018 17:23:54 +0300 MIME-Version: 1.0 In-Reply-To: <20180516162905.krtf6mksrndo7equ@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: tarantool-patches@freelists.org, Kirill Yukhin Hello. Thanks for the patch! I can not build, wonder travis is ok. Maybe Mac travis uses gcc instead of clang? See my 20 comments below. /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/delete.c:482:6: error: variable 'where' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized] if (src == NULL) ^~~~~~~~~~~ /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/delete.c:516:27: note: uninitialized use occurs here sql_expr_free(parse->db, where, false); ^~~~~ /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/delete.c:482:2: note: remove the 'if' if its condition is always false if (src == NULL) ^~~~~~~~~~~~~~~~ /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/delete.c:478:6: error: variable 'where' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized] if (parse->nErr > 0 || parse->db->mallocFailed) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/delete.c:516:27: note: uninitialized use occurs here sql_expr_free(parse->db, where, false); ^~~~~ /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/delete.c:478:2: note: remove the 'if' if its condition is always false if (parse->nErr > 0 || parse->db->mallocFailed) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/delete.c:478:6: error: variable 'where' is used uninitialized whenever '||' condition is true [-Werror,-Wsometimes-uninitialized] if (parse->nErr > 0 || parse->db->mallocFailed) ^~~~~~~~~~~~~~~ /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/delete.c:516:27: note: uninitialized use occurs here sql_expr_free(parse->db, where, false); ^~~~~ /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/delete.c:478:6: note: remove the '||' if its condition is always false if (parse->nErr > 0 || parse->db->mallocFailed) ^~~~~~~~~~~~~~~~~~ /Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/delete.c:488:2: note: variable 'where' is declared here struct Expr *where = NULL; ^ On 16/05/2018 19:29, Kirill Yukhin wrote: > Hi Vlad, > On 16 мая 18:24, Kirill Yukhin wrote: >> Refactor DELETE FROM statements translation, update > I've removed one more dead routine. Patch is in the bottom. > > -- > Regards, Kirill Yukhin > > sql: refactor SQL delete routines > > Refactor DELETE FROM statements translation, update > all live routines according to Tarantool's coding style. > Use key_def instead of Table* where possible. > Remove useless index delete routine as well. > > Part of #3235 > --- > src/box/sql/build.c | 6 +- > src/box/sql/delete.c | 1136 ++++++++++++++++------------------------------- > src/box/sql/fkey.c | 3 +- > src/box/sql/insert.c | 26 +- > src/box/sql/parse.y | 2 +- > src/box/sql/sqliteInt.h | 206 ++++++++- > src/box/sql/trigger.c | 12 +- > src/box/sql/update.c | 9 +- > src/box/sql/vdbe.c | 6 +- > src/box/sql/vdbeaux.c | 7 +- > 10 files changed, 619 insertions(+), 794 deletions(-) > > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c > index 3455f52..0c77057 100644 > --- a/src/box/sql/delete.c > +++ b/src/box/sql/delete.c > -Table * > -sqlite3SrcListLookup(Parse * pParse, SrcList * pSrc) > +struct Table * > +sql_list_lookup_table(struct Parse *parse, SrcList *src_list) > { > + struct SrcList_item *item = src_list->a; > + struct Table *table; > + assert(item != NULL && src_list->nSrc == 1); > + table = sqlite3LocateTable(parse, 0, item->zName); > + sqlite3DeleteTable(parse->db, item->pTab); > + item->pTab = table; > + if (table != NULL) > + table->nTabRef++; > + if (sqlite3IndexedByLookup(parse, item)) > + table = NULL; 1. What about --nTabRef; ? sqlite3IndexedByLookup does not unref table. Looks like original SQLite bug. > +sql_delete_by_where(struct Parse *parse, char *t_name, const char **columns, > + struct Expr **values, int pairs_count) > { > - Expr *where = NULL; > - SrcList *src; > - > - assert(nPairs > 0); > - if (pParse->nErr > 0 || pParse->db->mallocFailed) > + if (parse->nErr > 0 || parse->db->mallocFailed) > goto error; > - src = sql_alloc_src_list(pParse->db); > - src->a[0].zName = sqlite3DbStrDup(pParse->db, zTab); > + struct SrcList *src = sql_alloc_src_list(parse->db); > + src->a[0].zName = sqlite3DbStrDup(parse->db, t_name); > if (src == NULL) 2. Maybe first check src on NULL and then use it? Looks like the second SQLite bug. > > -/* Make sure "isView" and other macros defined above are undefined. Otherwise > - * they may interfere with compilation of other functions in this file > - * (or in another file, if this file becomes part of the amalgamation). > - */ > -#ifdef isView > -#undef isView > -#endif > -#ifdef pTrigger > -#undef pTrigger > -#endif > - > -/* > - * This routine generates VDBE code that causes a single row of a > - * single table to be deleted. Both the original table entry and > - * all indices are removed. > - * > - * Preconditions: > - * > - * 1. iDataCur is an open cursor on the btree that is the canonical data > - * store for the table. (This will be the PRIMARY KEY index) > - * > - * 2. Read/write cursors for all indices of pTab must be open as > - * cursor number iIdxCur+i for the i-th index. > - * > - * 3. The primary key for the row to be deleted must be stored in a > - * sequence of nPk memory cells starting at iPk. If nPk==0 that means > - * that a search record formed from OP_MakeRecord is contained in the > - * single memory location iPk. > - * > - * eMode: > - * Parameter eMode may be passed either ONEPASS_OFF (0), ONEPASS_SINGLE, or > - * ONEPASS_MULTI. If eMode is not ONEPASS_OFF, then the cursor > - * iDataCur already points to the row to delete. If eMode is ONEPASS_OFF > - * then this function must seek iDataCur to the entry identified by iPk > - * and nPk before reading from it. > - * > - * If eMode is ONEPASS_MULTI, then this call is being made as part > - * of a ONEPASS delete that affects multiple rows. In this case, if > - * iIdxNoSeek is a valid cursor number (>=0), then its position should > - * be preserved following the delete operation. Or, if iIdxNoSeek is not > - * a valid cursor number, the position of iDataCur should be preserved > - * instead. > - * > - * iIdxNoSeek: > - * If iIdxNoSeek is a valid cursor number (>=0), then it identifies an > - * index cursor (from within array of cursors starting at iIdxCur) that > - * already points to the index entry to be deleted. > - */ > void > -sqlite3GenerateRowDelete(Parse * pParse, /* Parsing context */ > - Table * pTab, /* Table containing the row to be deleted */ > - Trigger * pTrigger, /* List of triggers to (potentially) fire */ > - int iDataCur, /* Cursor from which column data is extracted */ > - int iIdxCur, /* First index cursor */ > - int iPk, /* First memory cell containing the PRIMARY KEY */ > - i16 nPk, /* Number of PRIMARY KEY memory cells */ > - u8 count, /* If non-zero, increment the row change counter */ > - enum on_conflict_action onconf, /* Default ON CONFLICT policy for triggers */ > - u8 eMode, /* ONEPASS_OFF, _SINGLE, or _MULTI. See above */ > - int iIdxNoSeek) /* Cursor number of cursor that does not need seeking */ > +sql_generate_row_delete(struct Parse *parse, struct Table *table, > + struct Trigger *trigger_list, int cursor, int reg_pk, 3. I think, it is time to invent a standard name for cursors: I see, that both 'cursor' and 'reg_pk' are cursors, but their names differs very. My proposal: each cursor variable must end with '_cursor'. So data_cursor and pk_cursor instead of cursor and reg_pk. > + short npk, bool is_count, 4. is_count -> need_update_count. This flag means, that global nChanges must be updated. > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index e056d63..6f23859 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -3671,15 +3670,69 @@ int sqlite3Select(Parse *, Select *, SelectDest *); > Select *sqlite3SelectNew(Parse *, ExprList *, SrcList *, Expr *, ExprList *, > Expr *, ExprList *, u32, Expr *, Expr *); > void sqlite3SelectDelete(sqlite3 *, Select *); > -Table *sqlite3SrcListLookup(Parse *, SrcList *); > + > +/** > + * While a SrcList can in general represent multiple tables and > + * subqueries (as in the FROM clause of a SELECT statement) in > + * this case it contains the name of a single table, as one might > + * find in an INSERT, DELETE, or UPDATE statement. Look up that > + * table in the symbol table and return a pointer. Set an error > + * message and return NULL if the table name is not found or if > + * any other error occurs. > + * > + * The following fields are initialized appropriate in src_list: > + * > + * pSrc->a[0].pTab Pointer to the Table object. 5. No pSrc anymore. > + * pSrc->a[0].pIndex Pointer to the INDEXED BY index, if > + * there is one. > + * > + * @param parse Parsing context. > + * @param src_list List containing single table element. > + * @retval Table object if found, NULL oterwise. > + */ > +struct Table * > +sql_list_lookup_table(struct Parse *parse, SrcList *src_list); > + > int sqlite3IsReadOnly(Parse *, Table *, int); 6. This function has no implementation. > void sqlite3OpenTable(Parse *, int iCur, Table *, int); > -#if defined(SQLITE_ENABLE_UPDATE_DELETE_LIMIT) > -Expr *sqlite3LimitWhere(Parse *, SrcList *, Expr *, ExprList *, Expr *, Expr *, > - char *); > -#endif > -void sqlite3DeleteFrom(Parse *, SrcList *, Expr *); 7. This function still exists in parse.y, and has no implementation. > -void sqlite3DeleteByKey(Parse *, char *, const char **, Expr **, int); > +/** > + * Generate code for a DELETE FROM statement. > + * > + * DELETE FROM table_wxyz WHERE a<5 AND b NOT NULL; > + * \________/ \________________/ > + * pTabList pWhere 8. No pTabList, no pWhere. > + * > + * @param parse Parsing context. > + * @param tab_list List of single element which table from which > + * deletetion if performed. > + * @param where The WHERE clause. May be NULL. > + */ > +void > +sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list, > + struct Expr *where); > + > +/** > + * Generate VDBE code for > + * DELETE FROM WHERE > + * = > + * AND ... > + * AND = ; 9. No nPairs. > + * > + * This function does not increment the nested counter and is > + * faster than nested parsing of the request above. > + * > + * @param parse Parser context. > + * @param t_name Table name. > + * @param columns Column names array. > + * @param values Column values array. > + * @param pairs_count Length of @columns and @values. > + * > + * In case of error the @values elements are deleted. > + */ > +void > +sql_delete_by_where(struct Parse *parse, char *t_name, const char **columns, > + struct Expr **values, int pairs_count); > + > void sqlite3Update(Parse *, SrcList *, ExprList *, Expr *, > enum on_conflict_action); > WhereInfo *sqlite3WhereBegin(Parse *, SrcList *, Expr *, ExprList *, ExprList *, > @@ -3782,11 +3835,121 @@ int sqlite3ExprContainsSubquery(Expr *); > int sqlite3ExprIsInteger(Expr *, int *); > int sqlite3ExprCanBeNull(const Expr *); > int sqlite3ExprNeedsNoAffinityChange(const Expr *, char); > -void sqlite3GenerateRowDelete(Parse *, Table *, Trigger *, int, int, int, i16, > - u8, enum on_conflict_action, u8, int); > -void sqlite3GenerateRowIndexDelete(Parse *, Table *, int, int); > -int sqlite3GenerateIndexKey(Parse *, Index *, int, int, int *, Index *, int); 10. Still exists in where.c and sqliteInt.h. > -void sqlite3ResolvePartIdxLabel(Parse *, int); > + > +/** > + * This routine generates VDBE code that causes a single row of a > + * single table to be deleted. Both the original table entry and > + * all indices are removed. > + * > + * Preconditions: > + * > + * 1. cursor is an open cursor on the btree that is the > + * canonical data store for the table. (This will be the > + * PRIMARY KEY index) > + * > + * 2. The primary key for the row to be deleted must be stored > + * in a sequence of nPk memory cells starting at iPk. If 11. No iPk, no nPk. > + * nPk==0 that means that a search record formed from > + * OP_MakeRecord is contained in the single memory location > + * iPk. > + * > + * mode: > + * Parameter mode may be passed either ONEPASS_OFF (0), > + * ONEPASS_SINGLE, or ONEPASS_MULTI. If eMode is not 12. No eMode. > + * ONEPASS_OFF, then the cursor already points to the row to > + * delete. If eMode is ONEPASS_OFF then this function must seek > + * cursor to the entry identified by ipk and npk before reading > + * from it. > + * > + * If mode is ONEPASS_MULTI, then this call is being made as > + * part of a ONEPASS delete that affects multiple rows. In this > + * case, if idx_noseek is a valid cursor number (>=0), then its > + * position should be preserved following the delete operation. > + * Or, if iIdxNoSeek is not a valid cursor number, the position 13. No iIdxNoSeek. Same parameter name mismatches in next functions. > + * of cursor should be preserved instead. > + * > + * idx_noseek: > + * If idx_noseek is a valid cursor number (>=0), then it > + * identifies an index cursor that already points to the index > + * entry to be deleted. 14. I think, it can be moved into @param idx_noseek description. (That will be named idx_noseek_cursor). > + * > + * @param parse Parsing context. > + * @param table Table containing the row to be deleted. > + * @param trigger_list List of triggers to (potentially) fire. > + * @param cursor Cursor from which column data is extracted/ > + * @param ipk First memory cell containing the PRIMARY KEY. > + * @param npk umber of PRIMARY KEY memory cells. > + * @param is_count If non-zero, increment the row change counter. 15. need_update_count. > + * @param onconf Default ON CONFLICT policy for triggers. > + * @param mode ONEPASS_OFF, _SINGLE, or _MULTI. See above. > + * @param idx_noseek Cursor number of cursor that does not need > + seeking. > + */ > +void > +sql_generate_row_delete(struct Parse *parse, struct Table *table, > + struct Trigger *trigger_list, int cursor, int ipk, > + short npk, bool is_count, > + enum on_conflict_action onconf, u8 eMode, > + int idx_noseek); > + > diff --git a/src/box/sql/update.c b/src/box/sql/update.c > index de1349b..60cb229 100644 > --- a/src/box/sql/update.c > +++ b/src/box/sql/update.c > @@ -163,7 +163,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ > > /* Locate the table which we want to update. > */ > - pTab = sqlite3SrcListLookup(pParse, pTabList); > + pTab = sql_list_lookup_table(pParse, pTabList); > if (pTab == 0) > goto update_cleanup; > > @@ -187,7 +187,9 @@ sqlite3Update(Parse * pParse, /* The parser context */ > if (sqlite3ViewGetColumnNames(pParse, pTab)) { > goto update_cleanup; > } > - if (sqlite3IsReadOnly(pParse, pTab, tmask)) { > + if (isView && tmask == 0) { > + sqlite3ErrorMsg(pParse, "cannot modify %s because it is a view", > + pTab->zName); > goto update_cleanup; > } > > @@ -324,7 +326,7 @@ sqlite3Update(Parse * pParse, /* The parser context */ > */ > #if !defined(SQLITE_OMIT_VIEW) && !defined(SQLITE_OMIT_TRIGGER) > if (isView) { > - sqlite3MaterializeView(pParse, pTab, pWhere, iDataCur); > + sql_materialize_view(pParse, pTab->zName, pWhere, iDataCur); > /* Number of columns from SELECT plus ID.*/ > nKey = pTab->nCol + 1; > } > @@ -603,7 +605,6 @@ sqlite3Update(Parse * pParse, /* The parser context */ > nKey); > VdbeCoverageNeverTaken(v); > } > - sqlite3GenerateRowIndexDelete(pParse, pTab, iDataCur, iIdxCur); 16. Why? > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index b8d4c1a..2ad6f3e 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -3236,20 +3236,20 @@ case OP_OpenTEphemeral: { > BtCursor *pBtCur; > assert(pOp->p1 >= 0); > assert(pOp->p2 > 0); > - assert(pOp->p4type != P4_KEYDEF || pOp->p4.key_def != NULL); > + assert(pOp->p4type == P4_KEYDEF); 17. Why was this check needed in the previous patch? > > pCx = allocateCursor(p, pOp->p1, pOp->p2, CURTYPE_TARANTOOL); > if (pCx == 0) goto no_mem; > pCx->nullRow = 1; > > - pCx->key_def = pOp->p4.key_def; > pBtCur = pCx->uc.pCursor; > /* Ephemeral spaces don't have space_id */ > pBtCur->eState = CURSOR_INVALID; > pBtCur->curFlags = BTCF_TEphemCursor; > > rc = tarantoolSqlite3EphemeralCreate(pCx->uc.pCursor, pOp->p2, > - pCx->key_def); > + pOp->p4.key_def); > + pCx->key_def = pCx->uc.pCursor->index->def->key_def; 18. Lets merge this into the previous patch. > diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c > index e67fcae..4a8a9d2 100644 > --- a/src/box/sql/vdbeaux.c > +++ b/src/box/sql/vdbeaux.c > @@ -1134,7 +1134,7 @@ sqlite3VdbeChangeP4(Vdbe * p, int addr, const char *zP4, int n) > } if (n == P4_BOOL) { > pOp->p4.b = *(bool*)zP4; > pOp->p4type = P4_BOOL; > - } else if (zP4 != 0) { > + } else { 19. Why? > @@ -1571,7 +1571,10 @@ displayP4(Op * pOp, char *zTemp, int nTemp) > #endif > case P4_COLLSEQ:{ > struct coll *pColl = pOp->p4.pColl; > - sqlite3XPrintf(&x, "(%.20s)", pColl->name); > + if (pColl != NULL) > + sqlite3XPrintf(&x, "(%.20s)", pColl->name); > + else > + sqlite3XPrintf(&x, "(binary)"); 20. Stray change? > break; > } > case P4_FUNCDEF:{ >