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

[Vladislav Shpilevoy]

Thanks for review fixes! Almost done! See 13 comments below.

>> On 16/05/2018 19:29, Kirill Yukhin wrote:
>>> Hi Vlad,
>>> On 16 мая 18:24, Kirill Yukhin wrote:
>>> --- 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.
> I'll submit a question to SQLite author.

1. Obviously, it is a bug. The code is very simple, and it is easy to see,
that table is never unreferenced on the error. Lets fix this in Tarantool.
Will Hipp fix this or not in vanila SQLite after you write him - it will be
his problem.

> 
>>> +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.
> Fixed.

2. Now I see, that sql_delete_by_where is never called.

Searching 4937 files for "sql_delete_by_where"

/Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/delete.c:
   381
   382  void
   383: sql_delete_by_where(struct Parse *parse, char *t_name, const char **columns,
   384  		    struct Expr **values, int pairs_count)
   385  {

/Users/v.shpilevoy/Work/Repositories/tarantool/src/box/sql/sqliteInt.h:
  3730   */
  3731  void
  3732: sql_delete_by_where(struct Parse *parse, char *t_name, const char **columns,
  3733  		    struct Expr **values, int pairs_count);
  3734

> 
>>> +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.
> Well, actualy no. Cursor is a cursor, this is immediate value in VDBE. Register (reg_pk) is a memory
> cell. They're not the same thing for sure.
> 
>>> +			short npk, bool is_count,

3. Oh, now I see. I was confused by names mismatch in the function declaration and implementation.
In the implementation you use 'reg_pk' name, but in the declaration you use 'ipk'. Please, fix
this too.
> 
>>> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
>>> --- 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.
> Fixed.
> 
>>> + *    pSrc->a[0].pIndex     Pointer to the INDEXED BY index, if

4. Now it is out of 66 symbols. Sorry, it is not my rules(

> @@ -3782,11 +3834,118 @@ int sqlite3ExprContainsSubquery(Expr *);
> +/**
> + * 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
> + *       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 mode is not
> + *   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 idx_noseek is not a valid cursor number, the position
> + *   of cursor should be preserved instead.
> + *
> + * @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 need_update_count. If non-zero, increment the row change

5. Non-zero -> true. Now it is boolean variable.

> + *        counter.
> + * @param onconf Default ON CONFLICT policy for triggers.
> + * @param mode ONEPASS_OFF, _SINGLE, or _MULTI.  See above.

6. 'mode' in @param, but 'eMode' in arguments.

> + * @param idx_noseek If it is a valid cursor number (>=0),
> + *        then it identifies an index cursor that already points
> + *        to the index entry to be deleted.
> + */
> +void
> +sql_generate_row_delete(struct Parse *parse, struct Table *table,
> +			struct Trigger *trigger_list, int cursor, int ipk,
> +			short npk, bool need_update_count,
> +			enum on_conflict_action onconf, u8 eMode,
> +			int idx_noseek);
> +
> +/**
> + * Generate code that will assemble an index key and stores it in
> + * register reg_out.  The key with be for index pIdx which is an

7. No pIdx. 'The key will be' maybe?

> + * index on table. cursor is the index of a cursor open on the
> + * table table and pointing to the entry that needs indexing.
> + * cursor must be the cursor of the PRIMARY KEY index.
> + *
> + * Return a register number which is the first in a block of
> + * registers that holds the elements of the index key.  The
> + * block of registers has already been deallocated by the time
> + * this routine returns.
> + *
> + * If *part_idx_label is not NULL, fill it in with a label and

8. *part_idx_label can not be NULL - it is integer. Maybe part_idx_label
with no '*'?

> + * jump to that label if pIdx is a partial index that should be

9. No pIdx.

> + * skipped. The label should be resolved using
> + * sql_resolve_part_idx_label(). A partial index should be skipped
> + * if its WHERE clause evaluates to false or null.  If index is
> + * not a partial index, *piPartIdxLabel will be set to zero which

10. No piPartIdxLabel.

> + * is an empty label that is ignored by sql_resolve_part_idx_label().
> + *
> + * The pPrior and regPrior parameters are used to implement a

11. No pPrior and regPrior.

> + * cache to avoid unnecessary register loads.  If prev is not
> + * NULL, then it is a pointer to a different index for which an
> + * index key has just been computed into register reg_prev. If the
> + * current pIdx index is generating its key into the same

12. No pIdx.

> + * sequence of registers and if prev and index share a column in
> + * common, then the register corresponding to that column already
> + * holds the correct value and the loading of that register is
> + * skipped. This optimization is helpful when doing a DELETE or
> + * an INTEGRITY_CHECK on a table with multiple indices, and
> + * especially with the PRIMARY KEY columns of the index.
> + *
> + * @param parse Parsing context.
> + * @param index The index for which to generate a key.

13. Index is a key?