[tarantool-patches] Re: [PATCH 3/3] sql: implement point where for DELETE stmts

[Vladislav Shpilevoy <v.shpilevoy@tarantool.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) {