[tarantool-patches] Re: [PATCH 2/2] sql: refactor delete routines

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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 <t_name> WHERE
> + *        <columns[0]> = <values[0]>
> + *    AND  ...
> + *    AND <columns[nPairs - 1]> = <values[nPairs - 1]>;

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:{
>