[tarantool-patches] Re: [PATCH 2/2] sql: replace KeyInfo with key_def

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

Hello. Thanks for contribution! See 38 comments below.

On 08/05/2018 10:56, Kirill Yukhin wrote:
> KeyInfo is a legacy struct which was heavily used in SQL
> front-end. This patch replaces all its usages w/ Tarantool's
> natural key description structure called key_def.
> 
> This change is a prt of data dictionary intagration effort:

1. prt?
2. intagration?

> Tarantool indexes don't aware of KeyInfo, that is why it was
> evicted.
> 
> Legacy KeyInfo memory handling was ref-counting based and now
> is replaced w/ memory duplication. This state of affairs should
> be improved in future.
> 
> Part of #3235
> ---
>   src/box/sql.c                 |  19 ++-
>   src/box/sql/analyze.c         |   6 +-
>   src/box/sql/build.c           |  79 +++------
>   src/box/sql/delete.c          |   6 +-
>   src/box/sql/expr.c            |  52 +++---
>   src/box/sql/fkey.c            |   2 +-
>   src/box/sql/insert.c          |  12 +-
>   src/box/sql/pragma.c          |   4 +-
>   src/box/sql/select.c          | 381 ++++++++++++++++++------------------------
>   src/box/sql/sqliteInt.h       |  50 ++----
>   src/box/sql/tarantoolInt.h    |   5 +-
>   src/box/sql/update.c          |   6 +-
>   src/box/sql/vdbe.c            |  80 +++++----
>   src/box/sql/vdbe.h            |  34 ++--
>   src/box/sql/vdbeInt.h         |  13 +-
>   src/box/sql/vdbeapi.c         | 183 --------------------
>   src/box/sql/vdbeaux.c         | 247 ++++++++++-----------------
>   src/box/sql/vdbemem.c         |  14 +-
>   src/box/sql/vdbesort.c        |  54 +++---
>   src/box/sql/where.c           |  21 ++-
>   src/box/sql/wherecode.c       |   2 +-
>   src/box/tuple.c               |   9 +
>   src/box/tuple.h               |   9 +
>   test/sql-tap/index1.test.lua  |   2 +
>   test/sql-tap/index4.test.lua  |   1 +
>   test/sql-tap/selectA.test.lua |   1 +
>   26 files changed, 496 insertions(+), 796 deletions(-)
> 
> diff --git a/src/box/sql.c b/src/box/sql.c
> index 838fcf6..42372ea 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -371,7 +371,7 @@ int tarantoolSqlite3Count(BtCursor *pCur, i64 *pnEntry)
>    * @retval SQLITE_OK on success, SQLITE_TARANTOOL_ERROR otherwise.
>    */
>   int tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count,
> -				    struct coll *aColl)
> +				    struct key_def *def)

3. Please, update the comment as well.
> @@ -380,11 +380,16 @@ int tarantoolSqlite3EphemeralCreate(BtCursor *pCur, uint32_t field_count,
>   	if (ephemer_key_def == NULL)
>   		return SQL_TARANTOOL_ERROR;
>   	for (uint32_t part = 0; part < field_count; ++part) {
> +		struct coll *coll;
> +		if (part < def->part_count)
> +			coll = def->parts[part].coll;
> +		else
> +			coll = NULL;
>   		key_def_set_part(ephemer_key_def, part /* part no */,
>   				 part /* filed no */,
>   				 FIELD_TYPE_SCALAR,
>   				 ON_CONFLICT_ACTION_NONE /* nullable_action */,
> -				 aColl /* coll */,
> +				 coll /* coll */,

4. Lets remove these annotations for each argument. It makes no sense.
> @@ -933,7 +938,8 @@ rename_fail:
>    * Performs exactly as extract_key + sqlite3VdbeCompareMsgpack,
>    * only faster.
>    */
> -int tarantoolSqlite3IdxKeyCompare(BtCursor *pCur, UnpackedRecord *pUnpacked,
> +int tarantoolSqlite3IdxKeyCompare(struct sqlite3 *db,
> +				  BtCursor *pCur, UnpackedRecord *pUnpacked,
>   				  int *res)

5. I see, that db is used only to store it in Mem.db in sqlite3VdbeCompareMsgpack.
This mem is then passed to vdbeCompareMemString, which do not use db. So this
argument can be removed as well as res. You can simply return comparison result.

Same about sqlite3VdbeRecordCompareMsgpack.
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index ae662fb..f9b54ef 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -2643,14 +2650,14 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage)
>   	} else {
>   		tnum = pIndex->tnum;
>   	}
> -	pKey = sqlite3KeyInfoOfIndex(pParse, db, pIndex);
> -	assert(pKey != 0 || db->mallocFailed || pParse->nErr);
> +	struct key_def *def = sql_index_key_def(pIndex);
> +	assert(def != NULL || db->mallocFailed || pParse->nErr);
>   
>   	/* Open the sorter cursor if we are to use one. */
>   	iSorter = pParse->nTab++;
>   	sqlite3VdbeAddOp4(v, OP_SorterOpen, iSorter, 0, pIndex->nColumn,
> -			  (char *)
> -			  sqlite3KeyInfoRef(pKey), P4_KEYINFO);
> +			  (char *)def, P4_KEYDEF);
> +	assert(0);

6. Please, use unreachable() macro.
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index aff534d3..6729eef 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -562,12 +562,11 @@ sqliteProcessJoin(Parse * pParse, Select * p)
>   	return 0;
>   }
>   
> -/* Forward reference */
> -static KeyInfo *
> -keyInfoFromExprList(Parse * pParse,	/* Parsing context */
> -		    ExprList * pList,	/* Form the KeyInfo object from this ExprList */
> -		    int iStart,		/* Begin with this column of pList */
> -		    int nExtra);	/* Add this many extra columns to the end */
> +static struct key_def *
> +sql_expr_list_to_key_def(struct Parse *parse,
> +			 struct ExprList *list,
> +			 int start);

7. It would be good to see the comment on the first function declaration.
> -/*
> - * Given an expression list, generate a KeyInfo structure that records
> +/**
> + * Given an expression list, generate a key_defd structure that records

8. key_defd -> key_def.

9. Please, wrap the comment on 66 symbols.

>    * the collating sequence for each expression in that expression list.
>    *
>    * If the ExprList is an ORDER BY or GROUP BY clause then the resulting
> - * KeyInfo structure is appropriate for initializing a virtual index to
> + * key_def structure is appropriate for initializing a virtual index to
>    * implement that clause.  If the ExprList is the result set of a SELECT
> - * then the KeyInfo structure is appropriate for initializing a virtual
> + * then the key_info structure is appropriate for initializing a virtual
>    * index to implement a DISTINCT test.
>    *
> - * Space to hold the KeyInfo structure is obtained from malloc.  The calling
> - * function is responsible for seeing that this structure is eventually
> - * freed.
> + * Space to hold the key_info structure is obtained from malloc.

10. Leading white space.

> + * The calling function is responsible for seeing that this
> + * structure is eventually freed.
> + *
> + * @param Parsing context.
> + * @param Expression list.
> + * @param No of leading parts to skip.
> + *
> + * @retval Allocated key_def, NULL in case of OOM.

11. Please, specify parameter names.

>    */
> -static KeyInfo *
> -keyInfoFromExprList(Parse * pParse,	/* Parsing context */
> -		    ExprList * pList,	/* Form the KeyInfo object from this ExprList */
> -		    int iStart,		/* Begin with this column of pList */
> -		    int nExtra)		/* Add this many extra columns to the end */
> +static struct key_def *
> +sql_expr_list_to_key_def(struct Parse *parse,
> +			 struct ExprList *list,
> +			 int start)

12. Lets put arguments into the line.

>   {
> -	int nExpr;
> -	KeyInfo *pInfo;
> -	struct ExprList_item *pItem;
> -	sqlite3 *db = pParse->db;
> -	int i;
> -
> -	nExpr = pList->nExpr;
> -	pInfo = sqlite3KeyInfoAlloc(db, nExpr - iStart, nExtra + 1);
> -	if (pInfo) {
> -		assert(sqlite3KeyInfoIsWriteable(pInfo));
> -		for (i = iStart, pItem = pList->a + iStart; i < nExpr;
> -		     i++, pItem++) {
> +	struct key_def *def;
> +	int expr_count = list->nExpr;
> +	def = key_def_new(expr_count);

13. How about struct key_def *def = key_def_new(expr_count); ?

> +	if (def != NULL) {
> +		int i;
> +		struct ExprList_item *item;
> +		for (i = start, item = list->a + start; i < expr_count;

14. How about 'for (int i...' instead of 'int i; for (...' ?

> +		     ++i, ++item) {
>   			bool unused;
> -			pInfo->aColl[i - iStart] =
> -				sql_expr_coll(pParse, pItem->pExpr, &unused);
> -			pInfo->aSortOrder[i - iStart] = pItem->sortOrder;
> +			struct coll *coll = sql_expr_coll(parse, item->pExpr,
> +							  &unused);
> +			enum sort_order sort_order = item->sortOrder;

15. How about inline it into the call below?

16. Please, set db->mallocFailed if key_def_new returns NULL.

> @@ -2118,54 +2058,62 @@ multiSelectCollSeq(Parse * pParse, Select * p, int iCol, bool *is_found)
>   	return coll;
>   }
>   
> -/*
> +/**
>    * The select statement passed as the second parameter is a compound SELECT
> - * with an ORDER BY clause. This function allocates and returns a KeyInfo
> + * with an ORDER BY clause. This function allocates and returns a key_def
>    * structure suitable for implementing the ORDER BY.
>    *
> - * Space to hold the KeyInfo structure is obtained from malloc. The calling
> + * Space to hold the key_def structure is obtained from malloc. The calling
>    * function is responsible for ensuring that this structure is eventually
>    * freed.
> + *
> + * @param Parsing context.
> + * @param Select struct to analyze.
> + * @param No of extra slots to allocate.
> + *
> + * @retval Allocated key_def, NULL in case of OOM.
>    */
> -static KeyInfo *
> -multiSelectOrderByKeyInfo(Parse * pParse, Select * p, int nExtra)
> +static struct key_def *
> +sql_multiselect_orderby_to_key_def(struct Parse *parse,
> +				   struct Select *s,
> +				   int extra)

17. All the same about this function.

>   {
> -	ExprList *pOrderBy = p->pOrderBy;
> -	int nOrderBy = p->pOrderBy->nExpr;
> -	sqlite3 *db = pParse->db;
> -	KeyInfo *pRet = sqlite3KeyInfoAlloc(db, nOrderBy + nExtra, 1);
> -	if (pRet) {
> -		int i;
> -		for (i = 0; i < nOrderBy; i++) {
> -			struct ExprList_item *pItem = &pOrderBy->a[i];
> -			Expr *pTerm = pItem->pExpr;
> +	int ob_count = s->pOrderBy->nExpr;
> +	struct key_def *key_def = key_def_new(ob_count + extra);
> +	if (key_def != NULL) {

18. Lets return if key_def == NULL and reduce indentation level. The code
will be more compact.
> @@ -2475,9 +2424,10 @@ multiSelect(Parse * pParse,	/* Parsing context */
>   	if (dest.eDest == SRT_EphemTab) {
>   		assert(p->pEList);
>   		int nCols = p->pEList->nExpr;
> -		KeyInfo *pKeyInfo = sqlite3KeyInfoAlloc(pParse->db, nCols + 1, 0);
> +		struct key_def *def;
> +		def = key_def_new(nCols + 1);

19. Lets avoid this style: 'declare, new line, assign'. Here it is ok to do
struct key_def *def = key_def_new(nCols + 1);
> @@ -2793,26 +2743,28 @@ multiSelect(Parse * pParse,	/* Parsing context */
>   	 * no temp tables are required.
>   	 */
>   	if (p->selFlags & SF_UsesEphemeral) {
> -		int i;		/* Loop counter */
> -		KeyInfo *pKeyInfo;	/* Collating sequence for the result set */
> -		Select *pLoop;	/* For looping through SELECT statements */
> -		struct coll **apColl;	/* For looping through pKeyInfo->aColl[] */
> -		int nCol;	/* Number of columns in result set */
> +		int nCol;
> +		struct key_def *key_def;

20. Same.
> @@ -2823,13 +2775,12 @@ multiSelect(Parse * pParse,	/* Parsing context */
>   				}
>   				sqlite3VdbeChangeP2(v, addr, nCol);
>   				sqlite3VdbeChangeP4(v, addr,
> -						    (char *)
> -						    sqlite3KeyInfoRef(pKeyInfo),
> -						    P4_KEYINFO);
> +						    (char *)key_def_dup(key_def),

21. What if dup is failed?
> @@ -2871,7 +2822,7 @@ sqlite3SelectWrongNumTermsError(Parse * pParse, Select * p)
>    * If regPrev>0 then it is the first register in a vector that
>    * records the previous output.  mem[regPrev] is a flag that is false
>    * if there has been no previous output.  If regPrev>0 then code is
> - * generated to suppress duplicates.  pKeyInfo is used for comparing
> + * generated to suppress duplicates.  def is used for comparing

22. Lets rewrite this comment in Tarantool style.
> @@ -2884,7 +2835,7 @@ generateOutputSubroutine(Parse * pParse,	/* Parsing context */
>   			 SelectDest * pDest,	/* Where to send the data */
>   			 int regReturn,		/* The return address register */
>   			 int regPrev,		/* Previous result register.  No uniqueness if 0 */
> -			 KeyInfo * pKeyInfo,	/* For comparing with previous entry */
> +			 struct key_def *def,	/* For comparing with previous entry */

23. And remove these annotations. And def here can be const key_def *.
> @@ -3145,7 +3098,8 @@ multiSelectOrderBy(Parse * pParse,	/* Parsing context */
>   	int iSub2;		/* EQP id of right-hand query */
>   
>   	assert(p->pOrderBy != 0);
> -	assert(pKeyDup == 0);	/* "Managed" code needs this.  Ticket #3382. */
> +	/* "Managed" code needs this.  Ticket #3382. */
> +	assert(def_dup == NULL);

24. This assertion makes no sense - obviously dup here is NULL, it is declared
as NULL few lines above.
> @@ -3216,27 +3170,30 @@ multiSelectOrderBy(Parse * pParse,	/* Parsing context */
>   	p->pOrderBy = pOrderBy;
>   	pPrior->pOrderBy = sqlite3ExprListDup(pParse->db, pOrderBy, 0);
>   
> -	/* Allocate a range of temporary registers and the KeyInfo needed
> +	/* Allocate a range of temporary registers and the key_def needed
>   	 * for the logic that removes duplicate result rows when the
>   	 * operator is UNION, EXCEPT, or INTERSECT (but not UNION ALL).
>   	 */
>   	if (op == TK_ALL) {
>   		regPrev = 0;
>   	} else {
> -		int nExpr = p->pEList->nExpr;
> -		assert(nOrderBy >= nExpr || db->mallocFailed);
> +		int expr_count = p->pEList->nExpr;
> +		assert(nOrderBy >= expr_count || db->mallocFailed);
>   		regPrev = pParse->nMem + 1;
> -		pParse->nMem += nExpr + 1;
> +		pParse->nMem += expr_count + 1;
>   		sqlite3VdbeAddOp2(v, OP_Integer, 0, regPrev);
> -		pKeyDup = sqlite3KeyInfoAlloc(db, nExpr, 1);
> -		if (pKeyDup) {
> -			assert(sqlite3KeyInfoIsWriteable(pKeyDup));
> -			for (i = 0; i < nExpr; i++) {
> +		def_dup = key_def_new(expr_count);

25. I do not see, where do you free it. So this key_def leaks. In two
functions below (generateOutputSubroutine) it is used only to create
more duplicates.
> @@ -3320,9 +3277,8 @@ multiSelectOrderBy(Parse * pParse,	/* Parsing context */
>   		VdbeNoopComment((v, "Output routine for B"));
>   		addrOutB = generateOutputSubroutine(pParse,
>   						    p, &destB, pDest, regOutB,
> -						    regPrev, pKeyDup, labelEnd);
> +						    regPrev, def_dup, labelEnd);
>   	}
> -	sqlite3KeyInfoUnref(pKeyDup);

This is where def_dup leaks.
> @@ -5979,8 +5932,8 @@ sqlite3Select(Parse * pParse,		/* The parser context */
>   			}
>   			sqlite3VdbeAddOp4(v, OP_Compare, iAMem, iBMem,
>   					  pGroupBy->nExpr,
> -					  (char *)sqlite3KeyInfoRef(pKeyInfo),
> -					  P4_KEYINFO);
> +					  (char*) key_def_dup(def),
> +					  P4_KEYDEF);

26. What if key_def_dup returned NULL? db->mallocFailed is not set. Or you can
reuse diag and set here pParse->rc into TARANTOOL_ERROR. In other places too.

> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index a811932..7d32556 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -2081,7 +2062,8 @@ struct KeyInfo {
>    * b-tree.
>    */
>   struct UnpackedRecord {
> -	KeyInfo *pKeyInfo;	/* Collation and sort-order information */
> +	/* Collation and sort-order information */

27. Please, put a dot at the end of sentence. And use /** prefix for
comments out of function body.
> @@ -3529,11 +3511,18 @@ const char *
>   index_collation_name(Index *, uint32_t);
>   struct coll *
>   sql_index_collation(Index *idx, uint32_t column);
> -struct coll *
> -sql_default_coll();
>   bool
>   space_is_view(Table *);
>   
> +/**
> + * Return key_def of provided struct Index.
> + *
> + * @param idx pointer to `struct Index` object
> + * @retval pointer to `struct key_def`

28. Please, say here that this function makes duplicate of original key_def,
and can return NULL.
> diff --git a/src/box/sql/update.c b/src/box/sql/update.c
> index f3bd0b7..d380b8c 100644
> --- a/src/box/sql/update.c
> +++ b/src/box/sql/update.c
> @@ -358,12 +358,12 @@ sqlite3Update(Parse * pParse,		/* The parser context */
>   	sqlite3VdbeAddOp2(v, OP_Null, 0, iPk);
>   
>   	if (isView) {
> -		KeyInfo *pKeyInfo = sqlite3KeyInfoAlloc(pParse->db, nKey, 0);
> +		struct key_def *def = key_def_new(nKey);

29. Here and in other places alloc() == NULL was ignored, because it set
db->mallocFailed. With key_def we can not ignore OOM.
> diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
> index e244606..4bd48d5 100644
> --- a/src/box/sql/vdbe.h
> +++ b/src/box/sql/vdbe.h
> @@ -257,14 +265,20 @@ char *sqlite3VdbeExpandSql(Vdbe *, const char *);
>   #endif
>   int sqlite3MemCompare(const Mem *, const Mem *, const struct coll *);
>   
> -void sqlite3VdbeRecordUnpackMsgpack(KeyInfo *, int, const void *,
> -				    UnpackedRecord *);
> -int sqlite3VdbeRecordCompare(int, const void *, UnpackedRecord *);
> -int sqlite3VdbeRecordCompareWithSkip(int, const void *, UnpackedRecord *, int);
> -UnpackedRecord *sqlite3VdbeAllocUnpackedRecord(KeyInfo *);
> +void sqlite3VdbeRecordUnpackMsgpack(struct sqlite3 *db,
> +				    struct key_def *key_def,
> +				    int key_count, const void * msgpack,
> +				    UnpackedRecord *dest);
> +int sqlite3VdbeRecordCompare(struct sqlite3 *db, int key_count,
> +			     const void *key1, UnpackedRecord *key2);
> +int sqlite3VdbeRecordCompareWithSkip(struct sqlite3 *db,
> +				     int key_count, const void *key1,
> +				     struct UnpackedRecord *key2, bool is_skip);

30. Db argument here is not needed. CompareWithSkip allocs nothing.

> +UnpackedRecord *sqlite3VdbeAllocUnpackedRecord(struct sqlite3 *,
> +					       struct key_def *);
>   int sql_vdbe_mem_alloc_region(Mem *, uint32_t);
>   
> -typedef int (*RecordCompare) (int, const void *, UnpackedRecord *);
> +typedef int (*RecordCompare) (struct sqlite3 *db, int, const void *, UnpackedRecord *);

31. Same.
> diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
> index b3998ea..c2352e7 100644
> --- a/src/box/sql/vdbeaux.c
> +++ b/src/box/sql/vdbeaux.c
> @@ -948,11 +948,9 @@ freeP4(sqlite3 * db, int p4type, void *p4)
>   			sqlite3DbFree(db, p4);
>   			break;
>   		}
> -	case P4_KEYINFO:{
> -			if (db->pnBytesFreed == 0)
> -				sqlite3KeyInfoUnref((KeyInfo *) p4);
> -			break;
> -		}
> +	case P4_KEYDEF:
> +		free(p4);

32. Please use key_def_delete. It will be hard to find all free() and
replace them to key_def_delete, when key_def will consists of more than
one memory blocks.

In other places too.
>   #ifdef SQLITE_ENABLE_EXPLAIN_COMMENTS
> @@ -1512,26 +1509,28 @@ displayP4(Op * pOp, char *zTemp, int nTemp)
>   	assert(nTemp >= 20);
>   	sqlite3StrAccumInit(&x, 0, zTemp, nTemp, 0);
>   	switch (pOp->p4type) {
> -	case P4_KEYINFO:{
> -			int j;
> -			KeyInfo *pKeyInfo;
> +	case P4_KEYDEF:{
> +			struct key_def *def;
>   
> -			if (pOp->p4.pKeyInfo == NULL) {
> +			if (pOp->p4.key_def == NULL) {
>   				sqlite3XPrintf(&x, "k[NULL]");
>   			} else {
> -				pKeyInfo = pOp->p4.pKeyInfo;
> -				assert(pKeyInfo->aSortOrder != 0);
> -				sqlite3XPrintf(&x, "k(%d", pKeyInfo->nField);
> -				for (j = 0; j < pKeyInfo->nField; j++) {
> -					struct coll *pColl = pKeyInfo->aColl[j];
> -					const char *zColl =
> -					    pColl ? pColl->name : "";
> -					if (strcmp(zColl, "BINARY") == 0)
> -						zColl = "B";
> +				def = pOp->p4.key_def;
> +				sqlite3XPrintf(&x, "k(%d", def->part_count);
> +				for (int j = 0; j < (int)def->part_count; j++) {
> +					struct coll *coll = def->parts[j].coll;
> +					const char *coll_str =
> +					    coll != NULL ? coll->name : "";
> +					if (strcmp(coll_str, "BINARY") == 0)
> +						coll_str = "B";
> +					const char *sort_order = "";
> +					if (def->parts[j].sort_order ==
> +					    SORT_ORDER_DESC) {
> +						sort_order = "-";
> +					}
>   					sqlite3XPrintf(&x, ",%s%s",
> -						       pKeyInfo->
> -						       aSortOrder[j] ? "-" : "",
> -						       zColl);
> +						       sort_order,
> +						       coll_str);

33. How about to move it into key_def.h/.cc and name key_def_str() or something?
> @@ -3545,7 +3543,8 @@ sql_vdbe_mem_alloc_region(Mem *vdbe_mem, uint32_t size)
>    * Return false if there is a disagreement.
>    */
>   static int
> -vdbeRecordCompareDebug(int nKey1, const void *pKey1,	/* Left key */
> +vdbeRecordCompareDebug(struct sqlite3 *db,
> +		       int nKey1, const void *pKey1,	/* Left key */
>   		       const UnpackedRecord * pPKey2,	/* Right key */
>   		       int desiredResult)		/* Correct answer */
>   {

34. It does not require db - this function and all nested ones do not use it
for anything except useless saving into struct Mem on stack.
> @@ -4286,68 +4279,6 @@ vdbeFreeUnpacked(sqlite3 * db, UnpackedRecord * p)
>   }
>   #endif				/* SQLITE_ENABLE_PREUPDATE_HOOK */
>   
> -#ifdef SQLITE_ENABLE_PREUPDATE_HOOK
> -/*
> - * Invoke the pre-update hook. If this is an UPDATE or DELETE pre-update call,
> - * then cursor passed as the second argument should point to the row about
> - * to be update or deleted. If the application calls sqlite3_preupdate_old(),
> - * the required value will be read from the row the cursor points to.
> - */
> -void
> -sqlite3VdbePreUpdateHook(Vdbe * v,		/* Vdbe pre-update hook is invoked by */

35. This function still is declared in vdbeInt.h.
> diff --git a/test/sql-tap/index1.test.lua b/test/sql-tap/index1.test.lua
> index 201838d..c0177c8 100755
> --- a/test/sql-tap/index1.test.lua
> +++ b/test/sql-tap/index1.test.lua
> @@ -528,7 +528,9 @@ test:do_execsql_test(
>           INSERT INTO t1 VALUES(2, 2,4);
>           INSERT INTO t1 VALUES(3, 3,8);
>           INSERT INTO t1 VALUES(4, 1,12);
> +	pragma vdbe_debug=1;
>           SELECT b FROM t1 WHERE a=1 ORDER BY b;
> +	pragma vdbe_debug=0;

36. Why?
> diff --git a/test/sql-tap/index4.test.lua b/test/sql-tap/index4.test.lua
> index 22e5066..85d3b3c 100755
> --- a/test/sql-tap/index4.test.lua
> +++ b/test/sql-tap/index4.test.lua
> @@ -22,6 +22,7 @@ testprefix = "index4"
>   test:do_execsql_test(
>       1.1,
>       [[
> +    pragma vdbe_debug=1;

37. Same.
> diff --git a/test/sql-tap/selectA.test.lua b/test/sql-tap/selectA.test.lua
> index fc482e9..fa3a025 100755
> --- a/test/sql-tap/selectA.test.lua
> +++ b/test/sql-tap/selectA.test.lua
> @@ -1155,6 +1155,7 @@ test:do_execsql_test(
>   test:do_execsql_test(
>       "selectA-2.94",
>       [[
> +    pragma vdbe_debug=1;

38. Same.