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

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

[Kirill Yukhin]

This patches consists of two preparational patches and the
main patch which enables DELETE's from spaces created w/ Lua.

Issue: https://github.com/tarantool/tarantool/issues/3235
Branch: https://github.com/tarantool/tarantool/tree/kyukhin/gh-3235-delete-with-where

Kirill Yukhin (3):
  sql: fetch primary index for affinity only
  sql: remove expressions from SQL indexes
  sql: implement point where for DELETE stmts

 src/box/field_def.c           |   1 +
 src/box/sql.c                 |   4 +-
 src/box/sql/build.c           |  63 ++++++-----
 src/box/sql/delete.c          | 103 +++++++++++------
 src/box/sql/expr.c            |  12 +-
 src/box/sql/insert.c          | 105 +++++++++---------
 src/box/sql/sqliteInt.h       |  63 +++++++++--
 src/box/sql/where.c           | 251 +++++++++++++++++++++++++++++++-----------
 src/box/sql/whereInt.h        |   2 +
 src/box/sql/wherecode.c       | 128 +++++++++++++++------
 src/box/sql/whereexpr.c       |  36 +-----
 test/sql-tap/delete1.test.lua |  24 +++-
 12 files changed, 527 insertions(+), 265 deletions(-)

-- 
2.16.2

[tarantool-patches] [PATCH 1/3] sql: fetch primary index for affinity only

[Kirill Yukhin]

This small patch removes usages of primary index throughout
code sql_table_delete_from, limiting use to fetching of
affinities only. We cannot use space_def here, since primary
index might contain calculated columns.

Part of #3235
---
 src/box/sql/delete.c | 54 ++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 38 insertions(+), 16 deletions(-)

diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index ddad54b..28713c8 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -183,6 +183,16 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 		struct NameContext nc;
 		memset(&nc, 0, sizeof(nc));
 		nc.pParse = parse;
+		if (tab_list->a[0].pTab == NULL) {
+			struct Table *t = malloc(sizeof(struct Table));
+			if (t == NULL) {
+				sqlite3OomFault(parse->db);
+				goto delete_from_cleanup;
+			}
+			assert(space != NULL);
+			t->def = space->def;
+			tab_list->a[0].pTab = t;
+		}
 		nc.pSrcList = tab_list;
 		if (sqlite3ResolveExprNames(&nc, where))
 			goto delete_from_cleanup;
@@ -196,7 +206,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 		 * is held in ephemeral table, there is no PK for
 		 * it, so columns should be loaded manually.
 		 */
-		struct Index *pk = NULL;
+		struct key_def *pk_def = NULL;
 		int reg_pk = parse->nMem + 1;
 		int pk_len;
 		int eph_cursor = parse->nTab++;
@@ -207,13 +217,17 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 			sqlite3VdbeAddOp2(v, OP_OpenTEphemeral,
 					  eph_cursor, pk_len);
 		} else {
-			pk = sqlite3PrimaryKeyIndex(table);
-			assert(pk != NULL);
-			pk_len = index_column_count(pk);
+			assert(space->index_count > 0);
+			pk_def = key_def_dup(space->index[0]->def->key_def);
+			if(pk_def == NULL) {
+				sqlite3OomFault(parse->db);
+				goto delete_from_cleanup;
+			}
+			pk_len = pk_def->part_count;
 			parse->nMem += pk_len;
-			sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, eph_cursor,
-					  pk_len);
-			sql_vdbe_set_p4_key_def(parse, pk);
+			sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, eph_cursor,
+					  pk_len, 0,
+					  (char *)pk_def, P4_KEYDEF);
 		}
 
 		/* Construct a query to find the primary key for
@@ -252,11 +266,9 @@ 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++) {
-				assert(pk->aiColumn[i] >= 0);
 				sqlite3ExprCodeGetColumnOfTable(v, table->def,
 								tab_cursor,
-								pk->
-								aiColumn[i],
+								pk_def->parts[i].fieldno,
 								reg_pk + i);
 			}
 		} else {
@@ -287,10 +299,14 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 			 * key.
 			 */
 			key_len = 0;
-			const char *zAff = is_view ? NULL :
-					  sqlite3IndexAffinityStr(parse->db, pk);
+			const char *aff = NULL;
+			struct Index *pk = sqlite3PrimaryKeyIndex(table);
+			if (pk != NULL) {
+				aff = is_view ? NULL :
+					sqlite3IndexAffinityStr(parse->db, pk);
+			}
 			sqlite3VdbeAddOp4(v, OP_MakeRecord, reg_pk, pk_len,
-					  reg_key, zAff, pk_len);
+					  reg_key, aff, pk_len);
 			/* Set flag to save memory allocating one
 			 * by malloc.
 			 */
@@ -325,8 +341,14 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 					  (void *)space, P4_SPACEPTR);
 			sqlite3VdbeAddOp3(v, OP_OpenWrite, tab_cursor,
 					  table->tnum, space_ptr_reg);
-			sql_vdbe_set_p4_key_def(parse, pk);
-			VdbeComment((v, "%s", pk->zName));
+			struct key_def *def = key_def_dup(pk_def);
+			if (def == NULL) {
+				sqlite3OomFault(parse->db);
+				goto delete_from_cleanup;
+			}
+			sqlite3VdbeAppendP4(v, def, P4_KEYDEF);
+
+			VdbeComment((v, "%s", space->index[0]->def->name));
 
 			if (one_pass == ONEPASS_MULTI)
 				sqlite3VdbeJumpHere(v, iAddrOnce);
@@ -339,7 +361,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 		if (one_pass != ONEPASS_OFF) {
 			/* OP_Found will use an unpacked key. */
 			assert(key_len == pk_len);
-			assert(pk != NULL || table->pSelect != NULL);
+			assert(pk_def != NULL || table->pSelect != NULL);
 			sqlite3VdbeAddOp4Int(v, OP_NotFound, tab_cursor,
 					     addr_bypass, reg_key, key_len);
 
-- 
2.16.2

[tarantool-patches] Re: [PATCH 1/3] sql: fetch primary index for affinity only

[Vladislav Shpilevoy]

Hello. Thanks for the patch! See 2 comments below.

On 01/06/2018 18:16, Kirill Yukhin wrote:
> This small patch removes usages of primary index throughout
> code sql_table_delete_from, limiting use to fetching of
> affinities only. We cannot use space_def here, since primary
> index might contain calculated columns.

1. What are calculated columns?

> 
> Part of #3235
> ---
>   src/box/sql/delete.c | 54 ++++++++++++++++++++++++++++++++++++----------------
>   1 file changed, 38 insertions(+), 16 deletions(-)
> 
> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> index ddad54b..28713c8 100644
> --- a/src/box/sql/delete.c
> +++ b/src/box/sql/delete.c
> @@ -183,6 +183,16 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>   		struct NameContext nc;
>   		memset(&nc, 0, sizeof(nc));
>   		nc.pParse = parse;
> +		if (tab_list->a[0].pTab == NULL) {
> +			struct Table *t = malloc(sizeof(struct Table));
> +			if (t == NULL) {
> +				sqlite3OomFault(parse->db);
> +				goto delete_from_cleanup;
> +			}
> +			assert(space != NULL);
> +			t->def = space->def;
> +			tab_list->a[0].pTab = t;

2. Why can not you declare struct Table on the stack at the top of this
function, and here use its address? Can this pTab be used out of
sql_table_delete_from()? And I do not see where do you delete this
table.

> +		}
>   		nc.pSrcList = tab_list;
>   		if (sqlite3ResolveExprNames(&nc, where))
>   			goto delete_from_cleanup;

[tarantool-patches] Re: [PATCH 1/3] sql: fetch primary index for affinity only

[Kirill Yukhin]

Hello Vlad,
My answers are inlined. Updated patch is in the bottom.

On 01 июн 21:00, Vladislav Shpilevoy wrote:
> Hello. Thanks for the patch! See 2 comments below.
> 
> On 01/06/2018 18:16, Kirill Yukhin wrote:
> > This small patch removes usages of primary index throughout
> > code sql_table_delete_from, limiting use to fetching of
> > affinities only. We cannot use space_def here, since primary
> > index might contain calculated columns.
> 1. What are calculated columns?
A.k.a. computed columns, those which ultimately are expressions.

> > Part of #3235
> > ---
> >   src/box/sql/delete.c | 54 ++++++++++++++++++++++++++++++++++++----------------
> >   1 file changed, 38 insertions(+), 16 deletions(-)
> > 
> > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> > index ddad54b..28713c8 100644
> > --- a/src/box/sql/delete.c
> > +++ b/src/box/sql/delete.c
> > @@ -183,6 +183,16 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
> >   		struct NameContext nc;
> >   		memset(&nc, 0, sizeof(nc));
> >   		nc.pParse = parse;
> > +		if (tab_list->a[0].pTab == NULL) {
> > +			struct Table *t = malloc(sizeof(struct Table));
> > +			if (t == NULL) {
> > +				sqlite3OomFault(parse->db);
> > +				goto delete_from_cleanup;
> > +			}
> > +			assert(space != NULL);
> > +			t->def = space->def;
> > +			tab_list->a[0].pTab = t;
> 
> 2. Why can not you declare struct Table on the stack at the top of this
> function, and here use its address? Can this pTab be used out of
> sql_table_delete_from()? And I do not see where do you delete this
> table.
Relaced w/ stack variable.

--
Regards, Kirill Yukhin

commit 6ef9616b71c6e253336c012442b90c4d2ebb2c55
Author: Kirill Yukhin <kyukhin@tarantool.org>
Date:   Wed May 23 15:31:37 2018 +0300

    sql: fetch primary index for affinity only
    
    This small patch removes usages of primary index throughout
    code sql_table_delete_from, limiting use to fetching of
    affinities only. We cannot use space_def here, since primary
    index might contain calculated columns.
    
    Part of #3235

diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index ddad54b..a4a8da6 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -76,6 +76,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 		      struct Expr *where)
 {
 	struct sqlite3 *db = parse->db;
+	struct Table loc_table;
 	if (parse->nErr || db->mallocFailed)
 		goto delete_from_cleanup;
 
@@ -183,6 +184,12 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 		struct NameContext nc;
 		memset(&nc, 0, sizeof(nc));
 		nc.pParse = parse;
+		if (tab_list->a[0].pTab == NULL) {
+			assert(space != NULL);
+			memset(&loc_table, 0, sizeof(struct Table));
+			loc_table.def = space->def;
+			tab_list->a[0].pTab = &loc_table;
+		}
 		nc.pSrcList = tab_list;
 		if (sqlite3ResolveExprNames(&nc, where))
 			goto delete_from_cleanup;
@@ -196,7 +203,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 		 * is held in ephemeral table, there is no PK for
 		 * it, so columns should be loaded manually.
 		 */
-		struct Index *pk = NULL;
+		struct key_def *pk_def = NULL;
 		int reg_pk = parse->nMem + 1;
 		int pk_len;
 		int eph_cursor = parse->nTab++;
@@ -207,13 +214,17 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 			sqlite3VdbeAddOp2(v, OP_OpenTEphemeral,
 					  eph_cursor, pk_len);
 		} else {
-			pk = sqlite3PrimaryKeyIndex(table);
-			assert(pk != NULL);
-			pk_len = index_column_count(pk);
+			assert(space->index_count > 0);
+			pk_def = key_def_dup(space->index[0]->def->key_def);
+			if(pk_def == NULL) {
+				sqlite3OomFault(parse->db);
+				goto delete_from_cleanup;
+			}
+			pk_len = pk_def->part_count;
 			parse->nMem += pk_len;
-			sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, eph_cursor,
-					  pk_len);
-			sql_vdbe_set_p4_key_def(parse, pk);
+			sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, eph_cursor,
+					  pk_len, 0,
+					  (char *)pk_def, P4_KEYDEF);
 		}
 
 		/* Construct a query to find the primary key for
@@ -252,11 +263,9 @@ 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++) {
-				assert(pk->aiColumn[i] >= 0);
 				sqlite3ExprCodeGetColumnOfTable(v, table->def,
 								tab_cursor,
-								pk->
-								aiColumn[i],
+								pk_def->parts[i].fieldno,
 								reg_pk + i);
 			}
 		} else {
@@ -287,10 +296,14 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 			 * key.
 			 */
 			key_len = 0;
-			const char *zAff = is_view ? NULL :
-					  sqlite3IndexAffinityStr(parse->db, pk);
+			const char *aff = NULL;
+			struct Index *pk = sqlite3PrimaryKeyIndex(table);
+			if (pk != NULL) {
+				aff = is_view ? NULL :
+					sqlite3IndexAffinityStr(parse->db, pk);
+			}
 			sqlite3VdbeAddOp4(v, OP_MakeRecord, reg_pk, pk_len,
-					  reg_key, zAff, pk_len);
+					  reg_key, aff, pk_len);
 			/* Set flag to save memory allocating one
 			 * by malloc.
 			 */
@@ -325,8 +338,14 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 					  (void *)space, P4_SPACEPTR);
 			sqlite3VdbeAddOp3(v, OP_OpenWrite, tab_cursor,
 					  table->tnum, space_ptr_reg);
-			sql_vdbe_set_p4_key_def(parse, pk);
-			VdbeComment((v, "%s", pk->zName));
+			struct key_def *def = key_def_dup(pk_def);
+			if (def == NULL) {
+				sqlite3OomFault(parse->db);
+				goto delete_from_cleanup;
+			}
+			sqlite3VdbeAppendP4(v, def, P4_KEYDEF);
+
+			VdbeComment((v, "%s", space->index[0]->def->name));
 
 			if (one_pass == ONEPASS_MULTI)
 				sqlite3VdbeJumpHere(v, iAddrOnce);
@@ -339,7 +358,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 		if (one_pass != ONEPASS_OFF) {
 			/* OP_Found will use an unpacked key. */
 			assert(key_len == pk_len);
-			assert(pk != NULL || table->pSelect != NULL);
+			assert(pk_def != NULL || table->pSelect != NULL);
 			sqlite3VdbeAddOp4Int(v, OP_NotFound, tab_cursor,
 					     addr_bypass, reg_key, key_len);
 

[tarantool-patches] Re: [PATCH 1/3] sql: fetch primary index for affinity only

[Vladislav Shpilevoy]

Hello. Thanks for the fixes! See one minor comment below.

And I do not see the new commit on the branch. Branch still has the
old one + 2 my review fixes commits.


> --
> Regards, Kirill Yukhin
> 
> commit 6ef9616b71c6e253336c012442b90c4d2ebb2c55
> Author: Kirill Yukhin <kyukhin@tarantool.org>
> Date:   Wed May 23 15:31:37 2018 +0300
> 
>      sql: fetch primary index for affinity only
>      
>      This small patch removes usages of primary index throughout
>      code sql_table_delete_from, limiting use to fetching of
>      affinities only. We cannot use space_def here, since primary
>      index might contain calculated columns.
>      
>      Part of #3235
> 
> diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> index ddad54b..a4a8da6 100644
> --- a/src/box/sql/delete.c
> +++ b/src/box/sql/delete.c
> @@ -325,8 +338,14 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>   					  (void *)space, P4_SPACEPTR);
>   			sqlite3VdbeAddOp3(v, OP_OpenWrite, tab_cursor,
>   					  table->tnum, space_ptr_reg);
> -			sql_vdbe_set_p4_key_def(parse, pk);
> -			VdbeComment((v, "%s", pk->zName));
> +			struct key_def *def = key_def_dup(pk_def);
> +			if (def == NULL) {
> +				sqlite3OomFault(parse->db);
> +				goto delete_from_cleanup;
> +			}
> +			sqlite3VdbeAppendP4(v, def, P4_KEYDEF);

Why could not you at first dup key_def, and then just use AddOp4? Instead of
AddOp3 + dup + AppendP4.

> +
> +			VdbeComment((v, "%s", space->index[0]->def->name));
>   
>   			if (one_pass == ONEPASS_MULTI)
>   				sqlite3VdbeJumpHere(v, iAddrOnce);

[tarantool-patches] Re: [PATCH 1/3] sql: fetch primary index for affinity only

[Kirill Yukhin]

Hello Vlad,
Updated patch in the bottom.

On 07 июн 18:01, Vladislav Shpilevoy wrote:
> Hello. Thanks for the fixes! See one minor comment below.
> 
> And I do not see the new commit on the branch. Branch still has the
> old one + 2 my review fixes commits.
Pushed.

> > --
> > Regards, Kirill Yukhin
> > 
> > diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
> > index ddad54b..a4a8da6 100644
> > --- a/src/box/sql/delete.c
> > +++ b/src/box/sql/delete.c
> > @@ -325,8 +338,14 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
> >   					  (void *)space, P4_SPACEPTR);
> >   			sqlite3VdbeAddOp3(v, OP_OpenWrite, tab_cursor,
> >   					  table->tnum, space_ptr_reg);
> > -			sql_vdbe_set_p4_key_def(parse, pk);
> > -			VdbeComment((v, "%s", pk->zName));
> > +			struct key_def *def = key_def_dup(pk_def);
> > +			if (def == NULL) {
> > +				sqlite3OomFault(parse->db);
> > +				goto delete_from_cleanup;
> > +			}
> > +			sqlite3VdbeAppendP4(v, def, P4_KEYDEF);
> Why could not you at first dup key_def, and then just use AddOp4? Instead of
> AddOp3 + dup + AppendP4.
Done.

--
Regards, Kirill Yukhin

commit 34e707406484a144e538c05677e1e6ea5a819fd3
Author: Kirill Yukhin <kyukhin@tarantool.org>
Date:   Wed May 23 15:31:37 2018 +0300

    sql: fetch primary index for affinity only
    
    This small patch removes usages of primary index throughout
    code sql_table_delete_from, limiting use to fetching of
    affinities only. We cannot use space_def here, since primary
    index might contain calculated columns.
    
    Part of #3235

diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index ddad54b..eff4da1 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -76,6 +76,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 		      struct Expr *where)
 {
 	struct sqlite3 *db = parse->db;
+	struct Table loc_table;
 	if (parse->nErr || db->mallocFailed)
 		goto delete_from_cleanup;
 
@@ -183,6 +184,12 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 		struct NameContext nc;
 		memset(&nc, 0, sizeof(nc));
 		nc.pParse = parse;
+		if (tab_list->a[0].pTab == NULL) {
+			assert(space != NULL);
+			memset(&loc_table, 0, sizeof(struct Table));
+			loc_table.def = space->def;
+			tab_list->a[0].pTab = &loc_table;
+		}
 		nc.pSrcList = tab_list;
 		if (sqlite3ResolveExprNames(&nc, where))
 			goto delete_from_cleanup;
@@ -196,7 +203,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 		 * is held in ephemeral table, there is no PK for
 		 * it, so columns should be loaded manually.
 		 */
-		struct Index *pk = NULL;
+		struct key_def *pk_def = NULL;
 		int reg_pk = parse->nMem + 1;
 		int pk_len;
 		int eph_cursor = parse->nTab++;
@@ -207,13 +214,17 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 			sqlite3VdbeAddOp2(v, OP_OpenTEphemeral,
 					  eph_cursor, pk_len);
 		} else {
-			pk = sqlite3PrimaryKeyIndex(table);
-			assert(pk != NULL);
-			pk_len = index_column_count(pk);
+			assert(space->index_count > 0);
+			pk_def = key_def_dup(space->index[0]->def->key_def);
+			if(pk_def == NULL) {
+				sqlite3OomFault(parse->db);
+				goto delete_from_cleanup;
+			}
+			pk_len = pk_def->part_count;
 			parse->nMem += pk_len;
-			sqlite3VdbeAddOp2(v, OP_OpenTEphemeral, eph_cursor,
-					  pk_len);
-			sql_vdbe_set_p4_key_def(parse, pk);
+			sqlite3VdbeAddOp4(v, OP_OpenTEphemeral, eph_cursor,
+					  pk_len, 0,
+					  (char *)pk_def, P4_KEYDEF);
 		}
 
 		/* Construct a query to find the primary key for
@@ -252,11 +263,9 @@ 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++) {
-				assert(pk->aiColumn[i] >= 0);
 				sqlite3ExprCodeGetColumnOfTable(v, table->def,
 								tab_cursor,
-								pk->
-								aiColumn[i],
+								pk_def->parts[i].fieldno,
 								reg_pk + i);
 			}
 		} else {
@@ -287,10 +296,14 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 			 * key.
 			 */
 			key_len = 0;
-			const char *zAff = is_view ? NULL :
-					  sqlite3IndexAffinityStr(parse->db, pk);
+			const char *aff = NULL;
+			struct Index *pk = sqlite3PrimaryKeyIndex(table);
+			if (pk != NULL) {
+				aff = is_view ? NULL :
+					sqlite3IndexAffinityStr(parse->db, pk);
+			}
 			sqlite3VdbeAddOp4(v, OP_MakeRecord, reg_pk, pk_len,
-					  reg_key, zAff, pk_len);
+					  reg_key, aff, pk_len);
 			/* Set flag to save memory allocating one
 			 * by malloc.
 			 */
@@ -323,10 +336,16 @@ 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);
-			sqlite3VdbeAddOp3(v, OP_OpenWrite, tab_cursor,
-					  table->tnum, space_ptr_reg);
-			sql_vdbe_set_p4_key_def(parse, pk);
-			VdbeComment((v, "%s", pk->zName));
+			struct key_def *def = key_def_dup(pk_def);
+			if (def == NULL) {
+				sqlite3OomFault(parse->db);
+				goto delete_from_cleanup;
+			}
+			sqlite3VdbeAddOp4(v, OP_OpenWrite, tab_cursor,
+					  table->tnum, space_ptr_reg,
+					  (void *)def, P4_KEYDEF);
+
+			VdbeComment((v, "%s", space->index[0]->def->name));
 
 			if (one_pass == ONEPASS_MULTI)
 				sqlite3VdbeJumpHere(v, iAddrOnce);
@@ -339,7 +358,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 		if (one_pass != ONEPASS_OFF) {
 			/* OP_Found will use an unpacked key. */
 			assert(key_len == pk_len);
-			assert(pk != NULL || table->pSelect != NULL);
+			assert(pk_def != NULL || table->pSelect != NULL);
 			sqlite3VdbeAddOp4Int(v, OP_NotFound, tab_cursor,
 					     addr_bypass, reg_key, key_len);
 

[tarantool-patches] [PATCH 2/3] sql: remove expressions from SQL indexes

[Kirill Yukhin]

Legacy SQL FE was able to create indexes w/ expressions.
Tarantool will employ diofferenct scheme to implement
functional indexes, thus code  handling it in SQL FE is
dead and redundant. Remove it.

Part of #3235
---
 src/box/sql/build.c     | 22 ++++++---------
 src/box/sql/delete.c    |  3 +--
 src/box/sql/expr.c      | 12 ++-------
 src/box/sql/insert.c    | 72 +++++++++++++++----------------------------------
 src/box/sql/sqliteInt.h |  6 -----
 src/box/sql/where.c     | 27 ++-----------------
 src/box/sql/wherecode.c | 13 +++------
 src/box/sql/whereexpr.c | 36 +++----------------------
 8 files changed, 42 insertions(+), 149 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 28e4d7a..65bba1f 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -252,7 +252,6 @@ static void
 freeIndex(sqlite3 * db, Index * p)
 {
 	sql_expr_delete(db, p->pPartIdxWhere, false);
-	sql_expr_list_delete(db, p->aColExpr);
 	sqlite3DbFree(db, p->zColAff);
 	sqlite3DbFree(db, p);
 }
@@ -3042,8 +3041,7 @@ sql_create_index(struct Parse *parse, struct Token *token,
 
 	/* Analyze the list of expressions that form the terms of the index and
 	 * report any errors.  In the common case where the expression is exactly
-	 * a table column, store that column in aiColumn[].  For general expressions,
-	 * populate pIndex->aColExpr and store XN_EXPR (-2) in aiColumn[].
+	 * a table column, store that column in aiColumn[].
 	 *
 	 * TODO: Issue a warning if two or more columns of the index are identical.
 	 * TODO: Issue a warning if the table primary key is used as part of the
@@ -3938,17 +3936,13 @@ sqlite3UniqueConstraint(Parse * pParse,	/* Parsing context */
 	Table *pTab = pIdx->pTable;
 
 	sqlite3StrAccumInit(&errMsg, pParse->db, 0, 0, 200);
-	if (pIdx->aColExpr) {
-		sqlite3XPrintf(&errMsg, "index '%q'", pIdx->zName);
-	} else {
-		for (j = 0; j < pIdx->nColumn; j++) {
-			char *zCol;
-			assert(pIdx->aiColumn[j] >= 0);
-			zCol = pTab->def->fields[pIdx->aiColumn[j]].name;
-			if (j)
-				sqlite3StrAccumAppend(&errMsg, ", ", 2);
-			sqlite3XPrintf(&errMsg, "%s.%s", pTab->def->name, zCol);
-		}
+	for (j = 0; j < pIdx->nColumn; j++) {
+		char *zCol;
+		assert(pIdx->aiColumn[j] >= 0);
+		zCol = pTab->def->fields[pIdx->aiColumn[j]].name;
+		if (j)
+			sqlite3StrAccumAppend(&errMsg, ", ", 2);
+		sqlite3XPrintf(&errMsg, "%s.%s", pTab->def->name, zCol);
 	}
 	zErr = sqlite3StrAccumFinish(&errMsg);
 	sqlite3HaltConstraint(pParse,
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 28713c8..2b59130 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -564,8 +564,7 @@ sql_generate_index_key(struct Parse *parse, struct Index *index, int cursor,
 			     prev->pPartIdxWhere != NULL))
 		prev = NULL;
 	for (int j = 0; j < col_cnt; j++) {
-		if (prev != NULL && prev->aiColumn[j] == index->aiColumn[j]
-		    && prev->aiColumn[j] != XN_EXPR) {
+		if (prev != NULL && prev->aiColumn[j] == index->aiColumn[j]) {
 			/*
 			 * This column was already computed by the
 			 * previous index.
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 8866f6f..a69d38b 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -3484,16 +3484,8 @@ sqlite3ExprCodeLoadIndexColumn(Parse * pParse,	/* The parsing context */
     )
 {
 	i16 iTabCol = pIdx->aiColumn[iIdxCol];
-	if (iTabCol == XN_EXPR) {
-		assert(pIdx->aColExpr);
-		assert(pIdx->aColExpr->nExpr > iIdxCol);
-		pParse->iSelfTab = iTabCur;
-		sqlite3ExprCodeCopy(pParse, pIdx->aColExpr->a[iIdxCol].pExpr,
-				    regOut);
-	} else {
-		sqlite3ExprCodeGetColumnOfTable(pParse->pVdbe, pIdx->pTable->def,
-						iTabCur, iTabCol, regOut);
-	}
+	sqlite3ExprCodeGetColumnOfTable(pParse->pVdbe, pIdx->pTable->def,
+					iTabCur, iTabCol, regOut);
 }
 
 void
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 59c61c7..3dbf855 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -98,21 +98,9 @@ sqlite3IndexAffinityStr(sqlite3 * db, Index * pIdx)
 		}
 		for (n = 0; n < nColumn; n++) {
 			i16 x = pIdx->aiColumn[n];
-			if (x >= 0) {
-				char affinity = pIdx->pTable->
-					def->fields[x].affinity;
-				pIdx->zColAff[n] = affinity;
-			} else {
-				char aff;
-				assert(x == XN_EXPR);
-				assert(pIdx->aColExpr != 0);
-				aff =
-				    sqlite3ExprAffinity(pIdx->aColExpr->a[n].
-							pExpr);
-				if (aff == 0)
-					aff = AFFINITY_BLOB;
-				pIdx->zColAff[n] = aff;
-			}
+			char affinity = pIdx->pTable->
+				def->fields[x].affinity;
+			pIdx->zColAff[n] = affinity;
 		}
 		pIdx->zColAff[n] = 0;
 	}
@@ -1256,34 +1244,24 @@ sqlite3GenerateConstraintChecks(Parse * pParse,		/* The parser context */
 		for (i = 0; i < nIdxCol; i++) {
 			int iField = pIdx->aiColumn[i];
 			int x;
-			if (iField == XN_EXPR) {
-				pParse->ckBase = regNewData + 1;
-				sqlite3ExprCodeCopy(pParse,
-						    pIdx->aColExpr->a[i].pExpr,
-						    regIdx + i);
-				pParse->ckBase = 0;
-				VdbeComment((v, "%s column %d", pIdx->zName,
-					     i));
-			} else {
-				/* OP_SCopy copies value in separate register,
-				 * which later will be used by OP_NoConflict.
-				 * But OP_NoConflict is necessary only in cases
-				 * when bytecode is needed for proper UNIQUE
-				 * constraint handling.
-				 */
-				if (uniqueByteCodeNeeded) {
-					if (iField == pTab->iPKey)
-						x = regNewData;
-					else
-						x = iField + regNewData + 1;
-
-					assert(iField >= 0);
-					sqlite3VdbeAddOp2(v, OP_SCopy,
-							  x, regIdx + i);
-					VdbeComment((v, "%s",
-						     pTab->def->fields[
-							iField].name));
-				}
+			/* OP_SCopy copies value in separate register,
+			 * which later will be used by OP_NoConflict.
+			 * But OP_NoConflict is necessary only in cases
+			 * when bytecode is needed for proper UNIQUE
+			 * constraint handling.
+			 */
+			if (uniqueByteCodeNeeded) {
+				if (iField == pTab->iPKey)
+					x = regNewData;
+				else
+					x = iField + regNewData + 1;
+
+				assert(iField >= 0);
+				sqlite3VdbeAddOp2(v, OP_SCopy,
+						  x, regIdx + i);
+				VdbeComment((v, "%s",
+					     pTab->def->fields[
+						     iField].name));
 			}
 		}
 
@@ -1688,14 +1666,6 @@ xferCompatibleIndex(Index * pDest, Index * pSrc)
 		if (pSrc->aiColumn[i] != pDest->aiColumn[i]) {
 			return 0;	/* Different columns indexed */
 		}
-		if (pSrc->aiColumn[i] == XN_EXPR) {
-			assert(pSrc->aColExpr != 0 && pDest->aColExpr != 0);
-			if (sqlite3ExprCompare(pSrc->aColExpr->a[i].pExpr,
-					       pDest->aColExpr->a[i].pExpr,
-					       -1) != 0) {
-				return 0;	/* Different expressions in the index */
-			}
-		}
 		if (sql_index_column_sort_order(pSrc, i) !=
 		    sql_index_column_sort_order(pDest, i)) {
 			return 0;	/* Different sort orders */
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 01351a1..943fda9 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2116,7 +2116,6 @@ struct Index {
 	/** Array of collation identifiers. */
 	uint32_t *coll_id_array;
 	Expr *pPartIdxWhere;	/* WHERE clause for partial indices */
-	ExprList *aColExpr;	/* Column expressions */
 	int tnum;		/* DB Page containing root of this index */
 	u16 nColumn;		/* Number of columns stored in the index */
 	u8 onError;		/* ON_CONFLICT_ACTION_ABORT, _IGNORE, _REPLACE,
@@ -2161,11 +2160,6 @@ index_field_tuple_est(struct Index *idx, uint32_t field);
 #define IsUniqueIndex(X)      (((X)->idxType == SQLITE_IDXTYPE_UNIQUE) || \
 				((X)->idxType == SQLITE_IDXTYPE_PRIMARYKEY))
 
-/* The Index.aiColumn[] values are normally positive integer.  But
- * there are some negative values that have special meaning:
- */
-#define XN_EXPR      (-2)	/* Indexed column is an expression */
-
 #ifdef DEFAULT_TUPLE_COUNT
 #undef DEFAULT_TUPLE_COUNT
 #endif
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index e791647..d312587 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -265,11 +265,6 @@ whereScanNext(WhereScan * pScan)
 			for (pTerm = pWC->a + k; k < pWC->nTerm; k++, pTerm++) {
 				if (pTerm->leftCursor == iCur
 				    && pTerm->u.leftColumn == iColumn
-				    && (iColumn != XN_EXPR
-					|| sqlite3ExprCompare(pTerm->pExpr->
-							      pLeft,
-							      pScan->pIdxExpr,
-							      iCur) == 0)
 				    && (pScan->iEquiv <= 1
 					|| !ExprHasProperty(pTerm->pExpr,
 							    EP_FromJoin))
@@ -377,9 +372,7 @@ whereScanInit(WhereScan * pScan,	/* The WhereScan object being initialized */
 	if (pIdx) {
 		int j = iColumn;
 		iColumn = pIdx->aiColumn[j];
-		if (iColumn == XN_EXPR) {
-			pScan->pIdxExpr = pIdx->aColExpr->a[j].pExpr;
-		} else if (iColumn >= 0) {
+		if (iColumn >= 0) {
 			char affinity =
 				pIdx->pTable->def->fields[iColumn].affinity;
 			pScan->idxaff = affinity;
@@ -387,8 +380,6 @@ whereScanInit(WhereScan * pScan,	/* The WhereScan object being initialized */
 			pScan->coll = sql_index_collation(pIdx, j, &id);
 			pScan->is_column_seen = true;
 		}
-	} else if (iColumn == XN_EXPR) {
-		return 0;
 	}
 	pScan->opMask = opMask;
 	pScan->k = 0;
@@ -2713,7 +2704,6 @@ indexMightHelpWithOrderBy(WhereLoopBuilder * pBuilder,
 			  Index * pIndex, int iCursor)
 {
 	ExprList *pOB;
-	ExprList *aColExpr;
 	int ii, jj;
 	int nIdxCol = index_column_count(pIndex);
 	if (index_is_unordered(pIndex))
@@ -2729,16 +2719,6 @@ indexMightHelpWithOrderBy(WhereLoopBuilder * pBuilder,
 				if (pExpr->iColumn == pIndex->aiColumn[jj])
 					return 1;
 			}
-		} else if ((aColExpr = pIndex->aColExpr) != 0) {
-			for (jj = 0; jj < nIdxCol; jj++) {
-				if (pIndex->aiColumn[jj] != XN_EXPR)
-					continue;
-				if (sqlite3ExprCompare
-				    (pExpr, aColExpr->a[jj].pExpr,
-				     iCursor) == 0) {
-					return 1;
-				}
-			}
 		}
 	}
 	return 0;
@@ -3429,10 +3409,7 @@ wherePathSatisfiesOrderBy(WhereInfo * pWInfo,	/* The WHERE clause */
 						if (pOBExpr->iColumn != iColumn)
 							continue;
 					} else {
-						if (sqlite3ExprCompare(pOBExpr,
-								       pIndex->aColExpr->a[j].pExpr, iCur)) {
-							continue;
-						}
+						continue;
 					}
 					if (iColumn >= 0) {
 						bool is_found;
diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index 09b2671..bf2a2a2 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -48,8 +48,6 @@ static const char *
 explainIndexColumnName(Index * pIdx, int i)
 {
 	i = pIdx->aiColumn[i];
-	if (i == XN_EXPR)
-		return "<expr>";
 	return pIdx->pTable->def->fields[i].name;
 }
 
@@ -719,7 +717,6 @@ codeAllEqualityTerms(Parse * pParse,	/* Parsing context */
 		for (j = 0; j < nSkip; j++) {
 			sqlite3VdbeAddOp3(v, OP_Column, iIdxCur,
 					  pIdx->aiColumn[j], regBase + j);
-			testcase(pIdx->aiColumn[j] == XN_EXPR);
 			VdbeComment((v, "%s", explainIndexColumnName(pIdx, j)));
 		}
 	}
@@ -1259,8 +1256,8 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about t
 			 * FYI: entries in an index are ordered as follows:
 			 *      NULL, ... NULL, min_value, ...
 			 */
-			if ((j >= 0 && pIdx->pTable->def->fields[j].is_nullable)
-			    || j == XN_EXPR) {
+			if (j >= 0
+			    && pIdx->pTable->def->fields[j].is_nullable) {
 				assert(pLoop->nSkip == 0);
 				bSeekPastNull = 1;
 				nExtraReg = 1;
@@ -1305,11 +1302,9 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about t
 #endif
 			if (pRangeStart == 0) {
 				j = pIdx->aiColumn[nEq];
-				if ((j >= 0 &&
-				     pIdx->pTable->def->fields[j].is_nullable)||
-				    j == XN_EXPR) {
+				if (j >= 0 &&
+				    pIdx->pTable->def->fields[j].is_nullable)
 					bSeekPastNull = 1;
-				}
 			}
 		}
 		assert(pRangeEnd == 0
diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
index aa6d452..e5d6b44 100644
--- a/src/box/sql/whereexpr.c
+++ b/src/box/sql/whereexpr.c
@@ -887,26 +887,19 @@ exprSelectUsage(WhereMaskSet * pMaskSet, Select * pS)
  * in any index.  Return TRUE (1) if pExpr is an indexed term and return
  * FALSE (0) if not.  If TRUE is returned, also set *piCur to the cursor
  * number of the table that is indexed and *piColumn to the column number
- * of the column that is indexed, or XN_EXPR (-2) if an expression is being
- * indexed.
+ * of the column that is indexed.
  *
  * If pExpr is a TK_COLUMN column reference, then this routine always returns
  * true even if that particular column is not indexed, because the column
  * might be added to an automatic index later.
  */
 static int
-exprMightBeIndexed(SrcList * pFrom,	/* The FROM clause */
-		   int op,	/* The specific comparison operator */
-		   Bitmask mPrereq,	/* Bitmask of FROM clause terms referenced by pExpr */
+exprMightBeIndexed(int op,	/* The specific comparison operator */
 		   Expr * pExpr,	/* An operand of a comparison operator */
 		   int *piCur,	/* Write the referenced table cursor number here */
 		   int *piColumn	/* Write the referenced table column number here */
     )
 {
-	Index *pIdx;
-	int i;
-	int iCur;
-
 	/* If this expression is a vector to the left or right of a
 	 * inequality constraint (>, <, >= or <=), perform the processing
 	 * on the first element of the vector.
@@ -923,27 +916,6 @@ exprMightBeIndexed(SrcList * pFrom,	/* The FROM clause */
 		*piColumn = pExpr->iColumn;
 		return 1;
 	}
-	if (mPrereq == 0)
-		return 0;	/* No table references */
-	if ((mPrereq & (mPrereq - 1)) != 0)
-		return 0;	/* Refs more than one table */
-	for (i = 0; mPrereq > 1; i++, mPrereq >>= 1) {
-	}
-	iCur = pFrom->a[i].iCursor;
-	for (pIdx = pFrom->a[i].pTab->pIndex; pIdx; pIdx = pIdx->pNext) {
-		if (pIdx->aColExpr == 0)
-			continue;
-		for (i = 0; i < pIdx->nColumn; i++) {
-			if (pIdx->aiColumn[i] != XN_EXPR)
-				continue;
-			if (sqlite3ExprCompare
-			    (pExpr, pIdx->aColExpr->a[i].pExpr, iCur) == 0) {
-				*piCur = iCur;
-				*piColumn = XN_EXPR;
-				return 1;
-			}
-		}
-	}
 	return 0;
 }
 
@@ -1038,13 +1010,13 @@ exprAnalyze(SrcList * pSrc,	/* the FROM clause */
 		}
 
 		if (exprMightBeIndexed
-		    (pSrc, op, prereqLeft, pLeft, &iCur, &iColumn)) {
+		    (op, pLeft, &iCur, &iColumn)) {
 			pTerm->leftCursor = iCur;
 			pTerm->u.leftColumn = iColumn;
 			pTerm->eOperator = operatorMask(op) & opMask;
 		}
 		if (pRight
-		    && exprMightBeIndexed(pSrc, op, pTerm->prereqRight, pRight,
+		    && exprMightBeIndexed(op, pRight,
 					  &iCur, &iColumn)
 		    ) {
 			WhereTerm *pNew;
-- 
2.16.2

[tarantool-patches] Re: [PATCH 2/3] sql: remove expressions from SQL indexes

[Vladislav Shpilevoy]

Thanks for the patch! See 2 comments below.

On 01/06/2018 18:16, Kirill Yukhin wrote:
> Legacy SQL FE was able to create indexes w/ expressions.
> Tarantool will employ diofferenct scheme to implement

1. diofferenct?

> functional indexes, thus code  handling it in SQL FE is
> dead and redundant. Remove it.
> 
> Part of #3235

2. I pushed my review fixes. Please, consider them as squash if ok.

> ---
>   src/box/sql/build.c     | 22 ++++++---------
>   src/box/sql/delete.c    |  3 +--
>   src/box/sql/expr.c      | 12 ++-------
>   src/box/sql/insert.c    | 72 +++++++++++++++----------------------------------
>   src/box/sql/sqliteInt.h |  6 -----
>   src/box/sql/where.c     | 27 ++-----------------
>   src/box/sql/wherecode.c | 13 +++------
>   src/box/sql/whereexpr.c | 36 +++----------------------
>   8 files changed, 42 insertions(+), 149 deletions(-)
> 

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

[Kirill Yukhin]

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/field_def.c b/src/box/field_def.c
index 4d39d03..8dbead6 100644
--- a/src/box/field_def.c
+++ b/src/box/field_def.c
@@ -95,6 +95,7 @@ const struct opt_def field_def_reg[] = {
 		     nullable_action, NULL),
 	OPT_DEF("collation", OPT_UINT32, struct field_def, coll_id),
 	OPT_DEF("default", OPT_STRPTR, struct field_def, default_value),
+	OPT_DEF("affinity", OPT_UINT32, struct field_def, affinity),
 	OPT_END,
 };
 
diff --git a/src/box/sql.c b/src/box/sql.c
index 7379cb4..b1d346e 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1453,7 +1453,7 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
 		uint32_t cid = def->fields[i].coll_id;
 		struct field_def *field = &def->fields[i];
 		const char *default_str = field->default_value;
-		int base_len = 4;
+		int base_len = 5;
 		if (cid != COLL_NONE)
 			base_len += 1;
 		if (default_str != NULL)
@@ -1474,6 +1474,8 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
 		assert(def->fields[i].is_nullable ==
 			       action_is_nullable(def->fields[i].nullable_action));
 		p = enc->encode_str(p, t, strlen(t));
+		p = enc->encode_str(p, "affinity", 8);
+		p = enc->encode_uint(p, def->fields[i].affinity);
 		p = enc->encode_str(p, "is_nullable", 11);
 		p = enc->encode_bool(p, def->fields[i].is_nullable);
 		p = enc->encode_str(p, "nullable_action", 15);
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;
+	}
 }
 
 struct ExprList *
@@ -1197,23 +1202,13 @@ space_checks_expr_list(uint32_t space_id)
 	return space->def->opts.checks;
 }
 
-/**
- * Create cursor which will be positioned to the space/index.
- * It makes space lookup and loads pointer to it into register,
- * which is passes to OP_OpenWrite as an argument.
- *
- * @param parse_context Parse context.
- * @param cursor Number of cursor to be created.
- * @param entity_id Encoded space and index ids.
- * @retval address of last opcode.
- */
 int
-emit_open_cursor(Parse *parse_context, int cursor, int entity_id)
+emit_open_cursor(struct Parse *parse_context, int cursor, int entity_id)
 {
 	assert(entity_id > 0);
 	struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(entity_id));
 	assert(space != NULL);
-	Vdbe *vdbe = parse_context->pVdbe;
+	struct Vdbe *vdbe = parse_context->pVdbe;
 	int space_ptr_reg = ++parse_context->nMem;
 	sqlite3VdbeAddOp4(vdbe, OP_LoadPtr, 0, space_ptr_reg, 0, (void*)space,
 			  P4_SPACEPTR);
@@ -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)
+{
+	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);
+}
 /*
  * Generate code that will increment the schema cookie.
  *
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);
 			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;
+				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);
+			}
 			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);
@@ -446,7 +461,8 @@ sql_generate_row_delete(struct Parse *parse, struct Table *table,
 	/* If there are any triggers to fire, allocate a range of registers to
 	 * use for the old.* references in the triggers.
 	 */
-	if (sqlite3FkRequired(table, NULL) || trigger_list != NULL) {
+	if (table != NULL &&
+	    (sqlite3FkRequired(table, NULL) || trigger_list != NULL)) {
 		/* Mask of OLD.* columns in use */
 		/* TODO: Could use temporary registers here. */
 		uint32_t mask =
@@ -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) {
 		uint8_t p5 = 0;
 		sqlite3VdbeAddOp2(v, OP_Delete, cursor,
 				  (need_update_count ? OPFLAG_NCHANGE : 0));
@@ -520,16 +536,20 @@ sql_generate_row_delete(struct Parse *parse, struct Table *table,
 		sqlite3VdbeChangeP5(v, p5);
 	}
 
-	/* Do any ON CASCADE, SET NULL or SET DEFAULT operations
-	 * required to handle rows (possibly in other tables) that
-	 * refer via a foreign key to the row just deleted.
-	 */
-	sqlite3FkActions(parse, table, 0, first_old_reg, 0);
+	if (table != NULL) {
+		/* Do any ON CASCADE, SET NULL or SET DEFAULT
+		 * operations required to handle rows (possibly
+		 * in other tables) that refer via a foreign
+		 * key to the row just deleted.
+		 */
 
-	/* Invoke AFTER DELETE trigger programs. */
-	sqlite3CodeRowTrigger(parse, trigger_list,
-			      TK_DELETE, 0, TRIGGER_AFTER, table, first_old_reg,
-			      onconf, label);
+		sqlite3FkActions(parse, table, 0, first_old_reg, 0);
+
+		/* Invoke AFTER DELETE trigger programs. */
+		sqlite3CodeRowTrigger(parse, trigger_list,
+				      TK_DELETE, 0, TRIGGER_AFTER, table,
+				      first_old_reg, onconf, label);
+	}
 
 	/* Jump here if the row had already been deleted before
 	 * any BEFORE trigger programs were invoked. Or if a trigger program
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.
+	 *
+	 * 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);
+	if (aff == NULL) {
+		sqlite3OomFault(db);
+		return 0;
+	}
+	int i;
+	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;
+}
+
 /*
  * Compute the affinity string for table pTab, if it has not already been
  * computed.  As an optimization, omit trailing AFFINITY_BLOB affinities.
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;
 		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 */
@@ -3552,8 +3554,36 @@ enum sort_order
 sql_index_column_sort_order(Index *idx, uint32_t column);
 
 void sqlite3EndTable(Parse *, Token *, Token *, Select *);
+
+/**
+ * DEPRECATED. All calls to be replaced w/ sql_emit_open_cursor.
+ * Create cursor which will be positioned to the space/index.
+ * It makes space lookup and loads pointer to it into register,
+ * which is passes to OP_OpenWrite as an argument.
+ *
+ * @param parse Parse context.
+ * @param cursor Number of cursor to be created.
+ * @param entity_id Encoded space and index ids.
+ * @retval address of last opcode.
+ */
+int
+emit_open_cursor(struct Parse *parse, int cursor, int entity_id);
+
+/**
+ * Create cursor which will be positioned to the space/index.
+ * It makes space lookup and loads pointer to it into register,
+ * which is passes to OP_OpenWrite as an argument.
+ *
+ * @param parse_context Parse context.
+ * @param cursor Number of cursor to be created.
+ * @param index_id Encoded index id (encoding is void actually, so
+ *        pas it as is). In future will be replaced with pointer
+ *        to struct index.
+ * @retval address of last opcode.
+ */
 int
-emit_open_cursor(Parse *, int, int);
+sql_emit_open_cursor(struct Parse *parse, int cursor, int index_id,
+		     struct space *space);
 
 int sqlite3ParseUri(const char *, const char *, unsigned int *,
 		    sqlite3_vfs **, char **, char **);
@@ -4085,6 +4115,31 @@ int sqlite3VarintLen(u64 v);
 #define putVarint    sqlite3PutVarint
 
 const char *sqlite3IndexAffinityStr(sqlite3 *, Index *);
+
+/**
+ * Return a pointer to the column affinity string associated with index
+ * pIdx. A column affinity string has one character for each column in
+ * the table, according to the affinity of the column:
+ *
+ *  Character      Column affinity
+ *  ------------------------------
+ *  'A'            BLOB
+ *  'B'            TEXT
+ *  'C'            NUMERIC
+ *  'D'            INTEGER
+ *  'F'            REAL
+ *
+ * Memory for the buffer containing the column index affinity string
+ * is managed along with the rest of the Index structure. It will be
+ * released when sqlite3DeleteIndex() is called.
+ *
+ * @param db Database handle.
+ * @param def index_def where from affinity to be extracted.
+ * @retval Affinity string.
+ */
+char *
+sql_index_affinity_str(struct sqlite3 *db, struct index_def *def);
+
 void sqlite3TableAffinity(Vdbe *, Table *, int);
 char sqlite3CompareAffinity(Expr * pExpr, char aff2);
 int sqlite3IndexAffinityOk(Expr * pExpr, char idx_affinity);
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];
 		if (iColumn >= 0) {
 			char affinity =
 				pIdx->pTable->def->fields[iColumn].affinity;
@@ -390,6 +393,34 @@ whereScanInit(WhereScan * pScan,	/* The WhereScan object being initialized */
 	return whereScanNext(pScan);
 }
 
+static WhereTerm *
+sql_where_scan_init(struct WhereScan *scan, struct WhereClause *clause,
+		    int cursor, int column, uint32_t op_mask,
+		    struct space_def *space_def, struct key_def *key_def)
+{
+	scan->pOrigWC = scan->pWC = clause;
+	scan->pIdxExpr = NULL;
+	scan->idxaff = 0;
+	scan->coll = NULL;
+	scan->is_column_seen = false;
+	if (key_def != NULL) {
+		int j = column;
+		column = key_def->parts[j].fieldno;
+		if (column >= 0) {
+			scan->idxaff = space_def->fields[column].affinity;
+			scan->coll = key_def->parts[column].coll;
+			scan->is_column_seen = true;
+		}
+	}
+	scan->opMask = op_mask;
+	scan->k = 0;
+	scan->aiCur[0] = cursor;
+	scan->aiColumn[0] = column;
+	scan->nEquiv = 1;
+	scan->iEquiv = 1;
+	return whereScanNext(scan);
+}
+
 /*
  * Search for a term in the WHERE clause that is of the form "X <op> <expr>"
  * where X is a reference to the iColumn of table iCur or of index pIdx
@@ -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);
+	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;
 }
 
 /*
@@ -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) {
 					iColumn = pIndex->aiColumn[j];
 					revIdx = sql_index_column_sort_order(pIndex,
 									     j);
@@ -4046,7 +4108,7 @@ wherePathSolver(WhereInfo * pWInfo, LogEst nRowEst)
  * general-purpose query planner.
  */
 static int
-whereShortCut(WhereLoopBuilder * pBuilder)
+sql_where_shortcut(struct WhereLoopBuilder *builder)
 {
 	WhereInfo *pWInfo;
 	struct SrcList_item *pItem;
@@ -4055,20 +4117,20 @@ whereShortCut(WhereLoopBuilder * pBuilder)
 	WhereLoop *pLoop;
 	int iCur;
 	int j;
-	Table *pTab;
-	Index *pIdx;
 
-	pWInfo = pBuilder->pWInfo;
+	pWInfo = builder->pWInfo;
 	if (pWInfo->wctrlFlags & WHERE_OR_SUBCLAUSE)
 		return 0;
 	assert(pWInfo->pTabList->nSrc >= 1);
 	pItem = pWInfo->pTabList->a;
-	pTab = pItem->pTab;
+	struct space_def *space_def = pItem->pTab->def;
+	assert(space_def != NULL);
+
 	if (pItem->fg.isIndexedBy)
 		return 0;
 	iCur = pItem->iCursor;
 	pWC = &pWInfo->sWC;
-	pLoop = pBuilder->pNew;
+	pLoop = builder->pNew;
 	pLoop->wsFlags = 0;
 	pLoop->nSkip = 0;
 	pTerm = sqlite3WhereFindTerm(pWC, iCur, -1, 0, WO_EQ, 0);
@@ -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;
+			}
+		} 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) {
@@ -4420,7 +4522,7 @@ sqlite3WhereBegin(Parse * pParse,	/* The parser context */
 	}
 #endif
 
-	if (nTabList != 1 || whereShortCut(&sWLB) == 0) {
+	if (nTabList != 1 || sql_where_shortcut(&sWLB) == 0) {
 		rc = whereLoopAddAll(&sWLB);
 		if (rc)
 			goto whereBeginError;
@@ -4583,13 +4685,31 @@ sqlite3WhereBegin(Parse * pParse,	/* The parser context */
 		}
 		if (pLoop->wsFlags & WHERE_INDEXED) {
 			Index *pIx = pLoop->pIndex;
+			struct index_def *idx_def = pLoop->index_def;
+			struct space *space = space_cache_find(pTabItem->pTab->def->id);
 			int iIndexCur;
 			int op = OP_OpenRead;
 			/* iAuxArg is always set if to a positive value if ONEPASS is possible */
 			assert(iAuxArg != 0
 			       || (pWInfo->
 				   wctrlFlags & WHERE_ONEPASS_DESIRED) == 0);
-			if (IsPrimaryKeyIndex(pIx)
+			/* Check if index is primary. Either of
+			 * points should be true:
+			 * 1. struct Index is non-NULL and is
+			 *    primary
+			 * 2. idx_def is non-NULL and it is
+			 *    primary
+			 * 3. (goal of this comment) both pIx and
+			 *    idx_def are NULL in which case it is
+			 *    ephemeral table, but not in Tnt sense.
+			 *    It is something w/ defined space_def
+			 *    and nothing else. Skip such loops.
+			 */
+			if (idx_def == NULL && pIx == NULL) continue;
+			bool is_primary = (pIx != NULL && IsPrimaryKeyIndex(pIx)) ||
+					   (idx_def != NULL && (idx_def->iid == 0)) |
+				(idx_def == NULL && pIx == NULL);
+			if (is_primary
 			    && (wctrlFlags & WHERE_OR_SUBCLAUSE) != 0) {
 				/* This is one term of an OR-optimization using
 				 * the PRIMARY KEY.  No need for a separate index
@@ -4597,13 +4717,31 @@ sqlite3WhereBegin(Parse * pParse,	/* The parser context */
 				iIndexCur = pLevel->iTabCur;
 				op = 0;
 			} else if (pWInfo->eOnePass != ONEPASS_OFF) {
-				Index *pJ = pTabItem->pTab->pIndex;
-				iIndexCur = iAuxArg;
-				assert(wctrlFlags & WHERE_ONEPASS_DESIRED);
-				while (ALWAYS(pJ) && pJ != pIx) {
-					iIndexCur++;
-					pJ = pJ->pNext;
+				if (pIx != NULL) {
+					Index *pJ = pTabItem->pTab->pIndex;
+					iIndexCur = iAuxArg;
+					assert(wctrlFlags &
+					       WHERE_ONEPASS_DESIRED);
+					while (ALWAYS(pJ) && pJ != pIx) {
+						iIndexCur++;
+						pJ = pJ->pNext;
+					}
+				} else {
+					if (space != NULL) {
+						for(uint32_t i = 0;
+						    i < space->index_count;
+						    ++i) {
+							if (space->index[i]->def ==
+							    idx_def) {
+								iIndexCur = iAuxArg + i;
+								break;
+							}
+						}
+					} else {
+						iIndexCur = iAuxArg;
+					}
 				}
+				assert(wctrlFlags & WHERE_ONEPASS_DESIRED);
 				op = OP_OpenWrite;
 				pWInfo->aiCurOnePass[1] = iIndexCur;
 			} else if (iAuxArg
@@ -4614,11 +4752,17 @@ sqlite3WhereBegin(Parse * pParse,	/* The parser context */
 				iIndexCur = pParse->nTab++;
 			}
 			pLevel->iIdxCur = iIndexCur;
-			assert(pIx->pSchema == pTab->pSchema);
 			assert(iIndexCur >= 0);
 			if (op) {
-				emit_open_cursor(pParse, iIndexCur, pIx->tnum);
-				sql_vdbe_set_p4_key_def(pParse, pIx);
+				if (pIx != NULL) {
+					emit_open_cursor(pParse, iIndexCur,
+							 pIx->tnum);
+					sql_vdbe_set_p4_key_def(pParse, pIx);
+				} else {
+					sql_emit_open_cursor(pParse, iIndexCur,
+							     idx_def->iid,
+							     space);
+				}
 				if ((pLoop->wsFlags & WHERE_CONSTRAINT) != 0
 				    && (pLoop->
 					wsFlags & (WHERE_COLUMN_RANGE |
@@ -4627,7 +4771,10 @@ sqlite3WhereBegin(Parse * pParse,	/* The parser context */
 					wctrlFlags & WHERE_ORDERBY_MIN) == 0) {
 					sqlite3VdbeChangeP5(v, OPFLAG_SEEKEQ);	/* Hint to COMDB2 */
 				}
-				VdbeComment((v, "%s", pIx->zName));
+				if (pIx != NULL)
+					VdbeComment((v, "%s", pIx->zName));
+				else
+					VdbeComment((v, "%s", idx_def->name));
 #ifdef SQLITE_ENABLE_COLUMN_USED_MASK
 				{
 					u64 colUsed = 0;
@@ -4809,7 +4956,6 @@ sqlite3WhereEnd(WhereInfo * pWInfo)
 	for (i = 0, pLevel = pWInfo->a; i < pWInfo->nLevel; i++, pLevel++) {
 		int k, last;
 		VdbeOp *pOp;
-		Index *pIdx = 0;
 		struct SrcList_item *pTabItem = &pTabList->a[pLevel->iFrom];
 		Table *pTab MAYBE_UNUSED = pTabItem->pTab;
 		assert(pTab != 0);
@@ -4836,12 +4982,15 @@ sqlite3WhereEnd(WhereInfo * pWInfo)
 		 * that reference the table and converts them into opcodes that
 		 * reference the index.
 		 */
+		Index *pIdx = NULL;
+		struct index_def *def = NULL;
 		if (pLoop->wsFlags & (WHERE_INDEXED | WHERE_IDX_ONLY)) {
 			pIdx = pLoop->pIndex;
+			def = pLoop->index_def;
 		} else if (pLoop->wsFlags & WHERE_MULTI_OR) {
 			pIdx = pLevel->u.pCovidx;
 		}
-		if (pIdx && !db->mallocFailed) {
+		if ((pIdx != NULL || def != NULL) && !db->mallocFailed) {
 			last = sqlite3VdbeCurrentAddr(v);
 			k = pLevel->addrBody;
 			pOp = sqlite3VdbeGetOp(v, k);
@@ -4850,7 +4999,8 @@ sqlite3WhereEnd(WhereInfo * pWInfo)
 					continue;
 				if (pOp->opcode == OP_Column) {
 					int x = pOp->p2;
-					assert(pIdx->pTable == pTab);
+					assert(pIdx == NULL ||
+					       pIdx->pTable == pTab);
 					if (x >= 0) {
 						pOp->p2 = x;
 						pOp->p1 = pLevel->iIdxCur;
diff --git a/src/box/sql/whereInt.h b/src/box/sql/whereInt.h
index 1303365..548cbcb 100644
--- a/src/box/sql/whereInt.h
+++ b/src/box/sql/whereInt.h
@@ -141,6 +141,8 @@ struct WhereLoop {
 	u16 nBtm;	/* Size of BTM vector */
 	u16 nTop;	/* Size of TOP vector */
 	Index *pIndex;	/* Index used, or NULL */
+	/** Index definition, if there's no pIndex. */
+	struct index_def *index_def;
 	u32 wsFlags;		/* WHERE_* flags describing the plan */
 	u16 nLTerm;		/* Number of entries in aLTerm[] */
 	u16 nSkip;		/* Number of NULL aLTerm[] entries */
diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index bf2a2a2..1ce1db0 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -38,6 +38,7 @@
  * that actually generate the bulk of the WHERE loop code.  The original where.c
  * file retains the code that does query planning and analysis.
  */
+#include "box/schema.h"
 #include "sqliteInt.h"
 #include "whereInt.h"
 
@@ -62,6 +63,7 @@ explainIndexColumnName(Index * pIdx, int i)
 static void
 explainAppendTerm(StrAccum * pStr,	/* The text expression being built */
 		  Index * pIdx,		/* Index to read column names from */
+		  struct index_def *def,
 		  int nTerm,		/* Number of terms */
 		  int iTerm,		/* Zero-based index of first term. */
 		  int bAnd,		/* Non-zero to append " AND " */
@@ -78,9 +80,16 @@ explainAppendTerm(StrAccum * pStr,	/* The text expression being built */
 	for (i = 0; i < nTerm; i++) {
 		if (i)
 			sqlite3StrAccumAppend(pStr, ",", 1);
-		sqlite3StrAccumAppendAll(pStr,
-					 explainIndexColumnName(pIdx,
-								iTerm + i));
+		const char *name;
+		if (pIdx != NULL) {
+			name = explainIndexColumnName(pIdx, iTerm + i);
+		} else {
+			assert(def != NULL);
+                        struct space *space = space_cache_find(def->space_id);
+                        assert(space != NULL);
+                        name = space->def->fields[i].name;
+		}
+		sqlite3StrAccumAppendAll(pStr, name);
 	}
 	if (nTerm > 1)
 		sqlite3StrAccumAppend(pStr, ")", 1);
@@ -116,16 +125,26 @@ static void
 explainIndexRange(StrAccum * pStr, WhereLoop * pLoop)
 {
 	Index *pIndex = pLoop->pIndex;
+	struct index_def *def = pLoop->index_def;
 	u16 nEq = pLoop->nEq;
 	u16 nSkip = pLoop->nSkip;
 	int i, j;
 
+	assert(pIndex != NULL || def != NULL);
+
 	if (nEq == 0
 	    && (pLoop->wsFlags & (WHERE_BTM_LIMIT | WHERE_TOP_LIMIT)) == 0)
 		return;
 	sqlite3StrAccumAppend(pStr, " (", 2);
 	for (i = 0; i < nEq; i++) {
-		const char *z = explainIndexColumnName(pIndex, i);
+		const char *z;
+		if (pIndex != NULL) {
+			z = explainIndexColumnName(pIndex, i);
+		} else {
+			struct space *space = space_cache_find(def->space_id);
+			assert(space != NULL);
+			z = space->def->fields[i].name;
+		}
 		if (i)
 			sqlite3StrAccumAppend(pStr, " AND ", 5);
 		sqlite3XPrintf(pStr, i >= nSkip ? "%s=?" : "ANY(%s)", z);
@@ -133,11 +152,11 @@ explainIndexRange(StrAccum * pStr, WhereLoop * pLoop)
 
 	j = i;
 	if (pLoop->wsFlags & WHERE_BTM_LIMIT) {
-		explainAppendTerm(pStr, pIndex, pLoop->nBtm, j, i, ">");
+		explainAppendTerm(pStr, pIndex, def, pLoop->nBtm, j, i, ">");
 		i = 1;
 	}
 	if (pLoop->wsFlags & WHERE_TOP_LIMIT) {
-		explainAppendTerm(pStr, pIndex, pLoop->nTop, j, i, "<");
+		explainAppendTerm(pStr, pIndex, def, pLoop->nTop, j, i, "<");
 	}
 	sqlite3StrAccumAppend(pStr, ")", 1);
 }
@@ -199,13 +218,15 @@ sqlite3WhereExplainOneScan(Parse * pParse,	/* Parse context */
 		}
 		if ((flags & WHERE_IPK) == 0) {
 			const char *zFmt = 0;
-			Index *pIdx;
+			Index *pIdx = pLoop->pIndex;
+			struct index_def *idx_def = pLoop->index_def;
+			if (pIdx == NULL && idx_def == NULL) return 0;
 
-			assert(pLoop->pIndex != 0);
-			pIdx = pLoop->pIndex;
+			assert(pIdx != NULL || idx_def != NULL);
 			assert(!(flags & WHERE_AUTO_INDEX)
 			       || (flags & WHERE_IDX_ONLY));
-			if (IsPrimaryKeyIndex(pIdx)) {
+			if ((pIdx != NULL && IsPrimaryKeyIndex(pIdx)) ||
+			    (idx_def != NULL && idx_def->iid == 0)) {
 				if (isSearch) {
 					zFmt = "PRIMARY KEY";
 				}
@@ -220,7 +241,12 @@ sqlite3WhereExplainOneScan(Parse * pParse,	/* Parse context */
 			}
 			if (zFmt) {
 				sqlite3StrAccumAppend(&str, " USING ", 7);
-				sqlite3XPrintf(&str, zFmt, pIdx->zName);
+				if (pIdx != NULL)
+					sqlite3XPrintf(&str, zFmt, pIdx->zName);
+				else if (idx_def != NULL)
+					sqlite3XPrintf(&str, zFmt, idx_def->name);
+				else
+					sqlite3XPrintf(&str, zFmt, "EPHEMERAL INDEX");
 				explainIndexRange(&str, pLoop);
 			}
 		} else if ((flags & WHERE_IPK) != 0
@@ -675,20 +701,19 @@ codeAllEqualityTerms(Parse * pParse,	/* Parsing context */
 	u16 nEq;		/* The number of == or IN constraints to code */
 	u16 nSkip;		/* Number of left-most columns to skip */
 	Vdbe *v = pParse->pVdbe;	/* The vm under construction */
-	Index *pIdx;		/* The index being used for this loop */
 	WhereTerm *pTerm;	/* A single constraint term */
 	WhereLoop *pLoop;	/* The WhereLoop object */
 	int j;			/* Loop counter */
 	int regBase;		/* Base register */
 	int nReg;		/* Number of registers to allocate */
-	char *zAff;		/* Affinity string to return */
 
 	/* This module is only called on query plans that use an index. */
 	pLoop = pLevel->pWLoop;
 	nEq = pLoop->nEq;
 	nSkip = pLoop->nSkip;
-	pIdx = pLoop->pIndex;
-	assert(pIdx != 0);
+	struct Index *pIdx = pLoop->pIndex;
+	struct index_def *idx_def = pLoop->index_def;
+	assert(pIdx != NULL || idx_def != NULL);
 
 	/* Figure out how many memory cells we will need then allocate them.
 	 */
@@ -696,9 +721,13 @@ codeAllEqualityTerms(Parse * pParse,	/* Parsing context */
 	nReg = pLoop->nEq + nExtraReg;
 	pParse->nMem += nReg;
 
-	zAff =
-	    sqlite3DbStrDup(pParse->db,
-			    sqlite3IndexAffinityStr(pParse->db, pIdx));
+	char *zAff;
+	if (pIdx != NULL) {
+		zAff = sqlite3DbStrDup(pParse->db,
+				       sqlite3IndexAffinityStr(pParse->db, pIdx));
+	} else {
+		zAff = sql_index_affinity_str(pParse->db, idx_def);
+	}
 	assert(zAff != 0 || pParse->db->mallocFailed);
 
 	if (nSkip) {
@@ -1214,7 +1243,6 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about t
 		int endEq;	/* True if range end uses ==, >= or <= */
 		int start_constraints;	/* Start of range is constrained */
 		int nConstraint;	/* Number of constraint terms */
-		Index *pIdx;	/* The index we will be using */
 		int iIdxCur;	/* The VDBE cursor for the index */
 		int nExtraReg = 0;	/* Number of extra registers needed */
 		int op;		/* Instruction opcode */
@@ -1227,7 +1255,9 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about t
 					      * to integer type, used for IPK.
 					      */
 
-		pIdx = pLoop->pIndex;
+		struct Index *pIdx = pLoop->pIndex;
+		struct index_def *idx_def = pLoop->index_def;
+		assert(pIdx != NULL || idx_def != NULL);
 		iIdxCur = pLevel->iIdxCur;
 		assert(nEq >= pLoop->nSkip);
 
@@ -1242,7 +1272,11 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about t
 		assert(pWInfo->pOrderBy == 0
 		       || pWInfo->pOrderBy->nExpr == 1
 		       || (pWInfo->wctrlFlags & WHERE_ORDERBY_MIN) == 0);
-		int nIdxCol = index_column_count(pIdx);
+		int nIdxCol;
+		if (pIdx != NULL)
+			nIdxCol = index_column_count(pIdx);
+		else
+			nIdxCol = idx_def->key_def->part_count;
 		if ((pWInfo->wctrlFlags & WHERE_ORDERBY_MIN) != 0
 		    && pWInfo->nOBSat > 0 && (nIdxCol > nEq)) {
 			j = pIdx->aiColumn[nEq];
@@ -1379,11 +1413,34 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about t
 			startEq = 0;
 			start_constraints = 1;
 		}
-		struct Index *pk = sqlite3PrimaryKeyIndex(pIdx->pTable);
-		assert(pk);
-		int nPkCol = index_column_count(pk);
-		char affinity =
-			pIdx->pTable->def->fields[pk->aiColumn[0]].affinity;
+		struct Index *pk = NULL;
+		struct index_def *idx_pk = NULL;
+		char affinity;
+		if (pIdx == NULL) {
+			struct space *space = space_cache_find(idx_def->space_id);
+			assert(space != NULL);
+			idx_pk = space->index[0]->def;
+			int fieldno = idx_pk->key_def->parts[0].fieldno;
+			affinity = space->def->fields[fieldno].affinity;
+			if (affinity == AFFINITY_UNDEFINED) {
+				if (idx_pk->key_def->part_count == 1 &&
+				    space->def->fields[fieldno].type ==
+				    FIELD_TYPE_INTEGER)
+					affinity = AFFINITY_INTEGER;
+				else
+					affinity = AFFINITY_BLOB;
+			}
+		} else {
+			pk = sqlite3PrimaryKeyIndex(pIdx->pTable);
+			affinity =
+				pIdx->pTable->def->fields[pk->aiColumn[0]].affinity;
+		}
+
+		int nPkCol;
+		if (pk != NULL)
+			nPkCol = index_column_count(pk);
+		else
+			nPkCol = idx_pk->key_def->part_count;
 		if (nPkCol == 1 && affinity == AFFINITY_INTEGER) {
 			/* Right now INTEGER PRIMARY KEY is the only option to
 			 * get Tarantool's INTEGER column type. Need special handling
@@ -1392,7 +1449,11 @@ sqlite3WhereCodeOneLoopStart(WhereInfo * pWInfo,	/* Complete information about t
 			 */
 			int limit = pRangeStart == NULL ? nEq : nEq + 1;
 			for (int i = 0; i < limit; i++) {
-				if (pIdx->aiColumn[i] == pk->aiColumn[0]) {
+				if ((pIdx != NULL && pIdx->aiColumn[i] ==
+				     pk->aiColumn[0]) ||
+				    (idx_pk != NULL &&
+				     idx_def->key_def->parts[i].fieldno ==
+				     idx_pk->key_def->parts[0].fieldno)) {
 					/* Here: we know for sure that table has INTEGER
 					   PRIMARY KEY, single column, and Index we're
 					   trying to use for scan contains this column. */
diff --git a/test/sql-tap/delete1.test.lua b/test/sql-tap/delete1.test.lua
index 810ca8a..d6d4762 100755
--- a/test/sql-tap/delete1.test.lua
+++ b/test/sql-tap/delete1.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(9)
+test:plan(10)
 
 --!./tcltestrunner.lua
 -- ["set","testdir",[["file","dirname",["argv0"]]]]
@@ -132,18 +132,30 @@ test:do_test(
     })
 
 -- Tests for data dictionary integration.
-s = box.schema.create_space('t')
-i = s:create_index('i', {parts={1, 'unsigned'}})
+format = {}
+format[1] = {name = 'id', type = 'scalar'}
+format[2] = {name = 'f', type = 'scalar'}
+s = box.schema.create_space('t', {format = format})
+i = s:create_index('i', {parts = {1, 'scalar'}})
+
 test:do_test(
     "delete1-6.0",
     function()
-	s:replace({1})
-	s:replace({2})
-	s:replace({3})
+	s:replace({1, 4})
+	s:replace({2, 5})
+	s:replace({3, 6})
 	return s:count()
     end,
     3)
 
+test:do_test(
+    "delete1-6.1.1",
+    function()
+        box.sql.execute([[delete from "t" where "id"=2]])
+        return s:count()
+    end,
+    2)
+
 test:do_test(
     "delete1-6.1",
     function()
-- 
2.16.2

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

[Vladislav Shpilevoy]

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

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

[n.pettik]

For some reason this patch got to the trunk without review fixes.
Raw version of this patch contains several bugs, so they led to test fails
(sql-tap/analyze6.test.lua and few sql-tap/tkt*).

I suggest my own fixes. The whole patch is at the end of letter.

Branch: https://github.com/tarantool/tarantool/commits/np/gh-3463-review-fixed-for-point-delete
Issue: https://github.com/tarantool/tarantool/issues/3463

> 
>>  }
>>    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.

Ok, I just replaced all calls of emit_open_cursor() with sql_emit_open_cursor().
The only difference is in doing space lookup outside emit_open_cursor().

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 552578048..8c83a797e 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1181,35 +1181,17 @@ space_checks_expr_list(uint32_t space_id)
 }
 
 int
-emit_open_cursor(struct Parse *parse_context, int cursor, int entity_id)
+sql_emit_open_cursor(struct Parse *parse_context, int cursor, int index_id,
+                    struct space *space)
 {
-       assert(entity_id > 0);
-       struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(entity_id));
        assert(space != NULL);
        struct Vdbe *vdbe = parse_context->pVdbe;
        int space_ptr_reg = ++parse_context->nMem;
        sqlite3VdbeAddOp4(vdbe, OP_LoadPtr, 0, space_ptr_reg, 0, (void*)space,
                          P4_SPACEPTR);
-       return sqlite3VdbeAddOp3(vdbe, OP_OpenWrite, cursor, entity_id,
+       return sqlite3VdbeAddOp3(vdbe, OP_OpenWrite, cursor, index_id,
                                 space_ptr_reg);
 }
-
-int
-sql_emit_open_cursor(struct Parse *parse, int cursor, int index_id, struct space *space)
-{
-       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);
-}

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

Fixed. See code above.

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

Refactored this way:

+++ b/src/box/sql/delete.c
@@ -88,7 +88,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
         * instead of just a Table* parameter.
         */
        struct Table *table = NULL;
-       struct space *space;
+       struct space *space = NULL;
        uint32_t space_id;
        /* Figure out if we have any triggers and if the table
         * being deleted from is a view.
@@ -104,17 +104,28 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
                                                strlen(tab_name));
                if (space_id == BOX_ID_NIL)
                        goto delete_from_cleanup;
+               /*
+                * In this case space has been created via Lua
+                * facilities, so there is no corresponding entry
+                * in table hash. Thus, lets create simple
+                * wrapper around space_def to support interface.
+                */
+               space = space_by_id(space_id);
+               tab_list->a[0].pTab = sql_table_construct_from_space(space);
+               if (tab_list->a[0].pTab == NULL)
+                       goto delete_from_cleanup;
        } else {
                table = sql_list_lookup_table(parse, tab_list);
                if (table == NULL)
                        goto delete_from_cleanup;
                space_id = SQLITE_PAGENO_TO_SPACEID(table->tnum);
+               space = space_by_id(space_id);
+               assert(space != NULL);

@@ -183,16 +194,6 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
                struct NameContext nc;
                memset(&nc, 0, sizeof(nc));
                nc.pParse = parse;
-               if (tab_list->a[0].pTab == NULL) {
-                       struct Table *t = calloc(sizeof(struct Table), 1);
-                       if (t == NULL) {
-                               sqlite3OomFault(parse->db);
-                               goto delete_from_cleanup;
-                       }
-                       assert(space != NULL);
-                       t->def = space->def;
-                       tab_list->a[0].pTab = t;
-               }

+++ b/src/box/sql/resolve.c
@@ -39,6 +39,9 @@
 #include <stdlib.h>
 #include <string.h>
 
+#include "box/box.h"
+#include "box/schema.h"
+
 /*
  * Walk the expression tree pExpr and increase the aggregate function
  * depth (the Expr.op2 field) by N on every TK_AGG_FUNCTION node.
@@ -1432,6 +1435,21 @@ resolveSelectStep(Walker * pWalker, Select * p)
        return WRC_Prune;
 }
 
+struct Table *
+sql_table_construct_from_space(const struct space *space)
+{
+       assert(space != NULL);
+       struct Table *table = calloc(1, sizeof(*table));
+       if (table == NULL) {
+               diag_set(OutOfMemory, sizeof(table), "calloc", "table");
+               return NULL;
+       }
+       table->def = space_def_dup(space->def);
+       if (table->def == NULL)
+               return NULL;
+       return table;
+}
+

+/**
+ * Create wrapper around space_def of the given space.
+ * This routine is required to support.
+ * Both table and space def are allocated on heap.
+ *
+ * @param space Space to be prototype.
+ * @retval New instance of struct Table allocated on malloc,
+ *         or NULL in case of OOM.
+ */
+struct Table *
+sql_table_construct_from_space(const struct space *space);

> 
>>  			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?

Idk, indeed lets always use space->def:

@@ -266,11 +267,7 @@ 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++) {
-                               struct space_def *def;
-                               if (table != NULL)
-                                       def = table->def;
-                               else
-                                       def = space->def;
+                               struct space_def *def = 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?

Idk as well, lets always use conversion from space->def->id.

@@ -342,15 +339,9 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
                        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);
-                       }
+                       int tnum =
+                               SQLITE_PAGENO_FROM_SPACEID_AND_INDEXID(space->def->id,
+                                                                      0);

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

Ok:

@@ -518,7 +509,7 @@ sql_generate_row_delete(struct Parse *parse, struct Table *table,
         * of the DELETE statement is to fire the INSTEAD OF
         * triggers).
         */
-       if (table == NULL || table->pSelect == NULL) {
+       if (table == NULL || !table->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.

Deleted the whole comment - it doesn’t seem to be relevant.
And slightly refactored function:
 
@@ -107,32 +107,22 @@ sqlite3IndexAffinityStr(sqlite3 *db, Index *index)
 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.
-        *
-        * 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);
+       uint32_t column_count = def->key_def->part_count;
+       char *aff = (char *)sqlite3DbMallocRaw(0, column_count + 1);
        if (aff == NULL) {
                sqlite3OomFault(db);
-               return 0;
+               return NULL;
        }
-       int i;
-       struct space *space = space_cache_find(def->space_id);
+       struct space *space = space_by_id(def->space_id);
        assert(space != NULL);
 
-       for (i = 0; i < nColumn; i++) {
+       for (uint32_t i = 0; i < column_count; 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;
+       aff[column_count] = '\0';
 
        return aff;
 }

> 
>> +	 *
>> +	 * 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?

Fixed, see diff above.

> 
>> +	if (aff == NULL) {
>> +		sqlite3OomFault(db);
>> +		return 0;
>> +	}
>> +	int i;
> 
> 10. Can be part of 'for’.

Fixed, see diff above.

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

Indeed, removed:

+++ b/src/box/sql/sqliteInt.h
@@ -2536,8 +2536,6 @@ 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;

> 
>>  		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?

No way:

@@ -371,10 +372,7 @@ whereScanInit(WhereScan * pScan,   /* The WhereScan object being initialized */
        pScan->is_column_seen = false;
        if (pIdx) {
                int j = iColumn;
-               if (j >= pIdx->nColumn)
-                       iColumn = -1;
-               else
-                       iColumn = pIdx->aiColumn[j];
+               iColumn = pIdx->aiColumn[j];

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

Ok, I have refactored this function. See diff at the end of letter.

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

AFAIK Ivan works on this issue, lets wait for his patch.

> 
>>  }
>>    /*
>> @@ -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?

@@ -2668,7 +2650,8 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage)
-				if (pIndex && j < pIndex->nColumn) {
+				if (pIndex != NULL) {


> 
>>  					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?

See patch below.

=======================================================================

From 9ced467a2f33ce02ac48f875db06d3eec7d5561d Mon Sep 17 00:00:00 2001
From: Nikita Pettik <korablev@tarantool.org>
Date: Mon, 18 Jun 2018 00:38:55 +0300
Subject: [PATCH] sql: review fixes for a40287051

---
 src/box/sql/analyze.c   |   6 +-
 src/box/sql/build.c     |  27 +----
 src/box/sql/delete.c    |  45 +++----
 src/box/sql/expr.c      |   9 +-
 src/box/sql/fkey.c      |   7 +-
 src/box/sql/insert.c    |  36 +++---
 src/box/sql/resolve.c   |  18 +++
 src/box/sql/select.c    |   5 +-
 src/box/sql/sqliteInt.h |  42 ++++---
 src/box/sql/where.c     | 309 +++++++++++++++++++++++++-----------------------
 src/box/sql/wherecode.c |   5 +-
 11 files changed, 256 insertions(+), 253 deletions(-)

diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index 9509645c0..28d68a55b 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -176,9 +176,9 @@ openStatTable(Parse * pParse,	/* Parsing context */
 
 	/* Open the sql_stat[134] tables for writing. */
 	for (i = 0; aTable[i]; i++) {
-		int addr = emit_open_cursor(pParse, iStatCur + i, aRoot[i]);
-		v->aOp[addr].p4.key_def = NULL;
-		v->aOp[addr].p4type = P4_KEYDEF;
+		struct space *space =
+			space_by_id(SQLITE_PAGENO_TO_SPACEID(aRoot[i]));
+		sql_emit_open_cursor(pParse, iStatCur + i, aRoot[i], space);
 		sqlite3VdbeChangeP5(v, aCreateTbl[i]);
 		VdbeComment((v, aTable[i]));
 	}
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 552578048..8c83a797e 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -1181,35 +1181,17 @@ space_checks_expr_list(uint32_t space_id)
 }
 
 int
-emit_open_cursor(struct Parse *parse_context, int cursor, int entity_id)
+sql_emit_open_cursor(struct Parse *parse_context, int cursor, int index_id,
+		     struct space *space)
 {
-	assert(entity_id > 0);
-	struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(entity_id));
 	assert(space != NULL);
 	struct Vdbe *vdbe = parse_context->pVdbe;
 	int space_ptr_reg = ++parse_context->nMem;
 	sqlite3VdbeAddOp4(vdbe, OP_LoadPtr, 0, space_ptr_reg, 0, (void*)space,
 			  P4_SPACEPTR);
-	return sqlite3VdbeAddOp3(vdbe, OP_OpenWrite, cursor, entity_id,
+	return sqlite3VdbeAddOp3(vdbe, OP_OpenWrite, cursor, index_id,
 				 space_ptr_reg);
 }
-
-int
-sql_emit_open_cursor(struct Parse *parse, int cursor, int index_id, struct space *space)
-{
-	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);
-}
 /*
  * Generate code that will increment the schema cookie.
  *
@@ -2668,7 +2650,8 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage)
 	if (memRootPage < 0)
 		sqlite3VdbeAddOp2(v, OP_Clear, SQLITE_PAGENO_TO_SPACEID(tnum),
 				  0);
-	emit_open_cursor(pParse, iIdx, tnum);
+	struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(tnum));
+	sql_emit_open_cursor(pParse, iIdx, tnum, space);
 	sqlite3VdbeChangeP5(v,
 			    OPFLAG_BULKCSR | ((memRootPage >= 0) ?
 					      OPFLAG_P2ISREG : 0));
diff --git a/src/box/sql/delete.c b/src/box/sql/delete.c
index 23e2bcc49..1beacec48 100644
--- a/src/box/sql/delete.c
+++ b/src/box/sql/delete.c
@@ -88,7 +88,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 	 * instead of just a Table* parameter.
 	 */
 	struct Table *table = NULL;
-	struct space *space;
+	struct space *space = NULL;
 	uint32_t space_id;
 	/* Figure out if we have any triggers and if the table
 	 * being deleted from is a view.
@@ -104,17 +104,28 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 						strlen(tab_name));
 		if (space_id == BOX_ID_NIL)
 			goto delete_from_cleanup;
+		/*
+		 * In this case space has been created via Lua
+		 * facilities, so there is no corresponding entry
+		 * in table hash. Thus, lets create simple
+		 * wrapper around space_def to support interface.
+		 */
+		space = space_by_id(space_id);
+		tab_list->a[0].pTab = sql_table_construct_from_space(space);
+		if (tab_list->a[0].pTab == NULL)
+			goto delete_from_cleanup;
 	} else {
 		table = sql_list_lookup_table(parse, tab_list);
 		if (table == NULL)
 			goto delete_from_cleanup;
 		space_id = SQLITE_PAGENO_TO_SPACEID(table->tnum);
+		space = space_by_id(space_id);
+		assert(space != NULL);
 		trigger_list =sqlite3TriggersExist(table, TK_DELETE,
 						   NULL, NULL);
 		is_complex = trigger_list != NULL ||
 			     sqlite3FkRequired(table, NULL);
 	}
-	space = space_by_id(space_id);
 	assert(space != NULL);
 
 	bool is_view = space->def->opts.is_view;
@@ -183,16 +194,6 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 		struct NameContext nc;
 		memset(&nc, 0, sizeof(nc));
 		nc.pParse = parse;
-		if (tab_list->a[0].pTab == NULL) {
-			struct Table *t = calloc(sizeof(struct Table), 1);
-			if (t == NULL) {
-				sqlite3OomFault(parse->db);
-				goto delete_from_cleanup;
-			}
-			assert(space != NULL);
-			t->def = space->def;
-			tab_list->a[0].pTab = t;
-		}
 		nc.pSrcList = tab_list;
 		if (sqlite3ResolveExprNames(&nc, where))
 			goto delete_from_cleanup;
@@ -266,11 +267,7 @@ 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++) {
-				struct space_def *def;
-				if (table != NULL)
-					def = table->def;
-				else
-					def = space->def;
+				struct space_def *def = space->def;
 				sqlite3ExprCodeGetColumnOfTable(v, def,
 								tab_cursor,
 								pk_def->parts[i].fieldno,
@@ -342,15 +339,9 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
 			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);
-			}
+			int tnum =
+				SQLITE_PAGENO_FROM_SPACEID_AND_INDEXID(space->def->id,
+								       0);
 			sqlite3VdbeAddOp3(v, OP_OpenWrite, tab_cursor,
 					  tnum, space_ptr_reg);
 			struct key_def *def = key_def_dup(pk_def);
@@ -518,7 +509,7 @@ sql_generate_row_delete(struct Parse *parse, struct Table *table,
 	 * of the DELETE statement is to fire the INSTEAD OF
 	 * triggers).
 	 */
-	if (table == NULL || table->pSelect == NULL) {
+	if (table == NULL || !table->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/expr.c b/src/box/sql/expr.c
index a69d38bd0..1a5e130d0 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -36,6 +36,8 @@
 #include "box/coll_id_cache.h"
 #include "coll.h"
 #include "sqliteInt.h"
+#include "tarantoolInt.h"
+#include "box/schema.h"
 #include "box/session.h"
 
 /* Forward declarations */
@@ -2485,9 +2487,10 @@ sqlite3FindInIndex(Parse * pParse,	/* Parsing context */
 							  "USING INDEX %s FOR IN-OPERATOR",
 							  pIdx->zName),
 							  P4_DYNAMIC);
-					emit_open_cursor(pParse, iTab,
-							 pIdx->tnum);
-					sql_vdbe_set_p4_key_def(pParse, pIdx);
+					struct space *space =
+						space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum));
+					sql_emit_open_cursor(pParse, iTab,
+							     pIdx->tnum, space);
 					VdbeComment((v, "%s", pIdx->zName));
 					assert(IN_INDEX_INDEX_DESC ==
 					       IN_INDEX_INDEX_ASC + 1);
diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
index 6fd29200c..74cf51302 100644
--- a/src/box/sql/fkey.c
+++ b/src/box/sql/fkey.c
@@ -35,6 +35,7 @@
  */
 #include "coll.h"
 #include "sqliteInt.h"
+#include "box/schema.h"
 #include "box/session.h"
 #include "tarantoolInt.h"
 
@@ -439,9 +440,9 @@ fkLookupParent(Parse * pParse,	/* Parse context */
 			int nCol = pFKey->nCol;
 			int regTemp = sqlite3GetTempRange(pParse, nCol);
 			int regRec = sqlite3GetTempReg(pParse);
-
-			emit_open_cursor(pParse, iCur, pIdx->tnum);
-			sql_vdbe_set_p4_key_def(pParse, pIdx);
+			struct space *space =
+				space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum));
+			sql_emit_open_cursor(pParse, iCur, pIdx->tnum, space);
 			for (i = 0; i < nCol; i++) {
 				sqlite3VdbeAddOp2(v, OP_Copy,
 						  aiCol[i] + 1 + regData,
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 7aba83529..0a0b3fc21 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -54,8 +54,8 @@ sqlite3OpenTable(Parse * pParse,	/* Generate code into this VDBE */
 	Index *pPk = sqlite3PrimaryKeyIndex(pTab);
 	assert(pPk != 0);
 	assert(pPk->tnum == pTab->tnum);
-	emit_open_cursor(pParse, iCur, pPk->tnum);
-	sql_vdbe_set_p4_key_def(pParse, pPk);
+	struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pPk->tnum));
+	sql_emit_open_cursor(pParse, iCur, pPk->tnum, space);
 	VdbeComment((v, "%s", pTab->def->name));
 }
 
@@ -107,32 +107,22 @@ sqlite3IndexAffinityStr(sqlite3 *db, Index *index)
 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.
-	 *
-	 * 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);
+	uint32_t column_count = def->key_def->part_count;
+	char *aff = (char *)sqlite3DbMallocRaw(0, column_count + 1);
 	if (aff == NULL) {
 		sqlite3OomFault(db);
-		return 0;
+		return NULL;
 	}
-	int i;
-	struct space *space = space_cache_find(def->space_id);
+	struct space *space = space_by_id(def->space_id);
 	assert(space != NULL);
 
-	for (i = 0; i < nColumn; i++) {
+	for (uint32_t i = 0; i < column_count; 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;
+	aff[column_count] = '\0';
 
 	return aff;
 }
@@ -1951,11 +1941,13 @@ xferOptimization(Parse * pParse,	/* Parser context */
 				break;
 		}
 		assert(pSrcIdx);
-		emit_open_cursor(pParse, iSrc, pSrcIdx->tnum);
-		sql_vdbe_set_p4_key_def(pParse, pSrcIdx);
+		struct space *src_space =
+			space_by_id(SQLITE_PAGENO_TO_SPACEID(pSrcIdx->tnum));
+		sql_emit_open_cursor(pParse, iSrc, pSrcIdx->tnum, src_space);
 		VdbeComment((v, "%s", pSrcIdx->zName));
-		emit_open_cursor(pParse, iDest, pDestIdx->tnum);
-		sql_vdbe_set_p4_key_def(pParse, pDestIdx);
+		struct space *dest_space =
+			space_by_id(SQLITE_PAGENO_TO_SPACEID(pDestIdx->tnum));
+		sql_emit_open_cursor(pParse, iDest, pDestIdx->tnum, dest_space);
 		sqlite3VdbeChangeP5(v, OPFLAG_BULKCSR);
 		VdbeComment((v, "%s", pDestIdx->zName));
 		addr1 = sqlite3VdbeAddOp2(v, OP_Rewind, iSrc, 0);
diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index 23e16189a..e7d9723fa 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -39,6 +39,9 @@
 #include <stdlib.h>
 #include <string.h>
 
+#include "box/box.h"
+#include "box/schema.h"
+
 /*
  * Walk the expression tree pExpr and increase the aggregate function
  * depth (the Expr.op2 field) by N on every TK_AGG_FUNCTION node.
@@ -1432,6 +1435,21 @@ resolveSelectStep(Walker * pWalker, Select * p)
 	return WRC_Prune;
 }
 
+struct Table *
+sql_table_construct_from_space(const struct space *space)
+{
+	assert(space != NULL);
+	struct Table *table = calloc(1, sizeof(*table));
+	if (table == NULL) {
+		diag_set(OutOfMemory, sizeof(table), "calloc", "table");
+		return NULL;
+	}
+	table->def = space_def_dup(space->def);
+	if (table->def == NULL)
+		return NULL;
+	return table;
+}
+
 /*
  * This routine walks an expression tree and resolves references to
  * table columns and result-set columns.  At the same time, do error
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 4b5ba4d3e..9e8ed97e7 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -6107,8 +6107,9 @@ sqlite3Select(Parse * pParse,		/* The parser context */
 				 * Open the cursor, execute the OP_Count,
 				 * close the cursor.
 				 */
-				emit_open_cursor(pParse, cursor,
-						 space->def->id << 10);
+				sql_emit_open_cursor(pParse, cursor,
+						     space->def->id << 10,
+						     space);
 				sqlite3VdbeAddOp2(v, OP_Count, cursor,
 						  sAggInfo.aFunc[0].iMem);
 				sqlite3VdbeAddOp1(v, OP_Close, cursor);
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 878daa8df..b7e7eafa8 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -2536,8 +2536,6 @@ 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;
 		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 */
@@ -3553,20 +3551,6 @@ sql_index_column_sort_order(Index *idx, uint32_t column);
 
 void sqlite3EndTable(Parse *, Token *, Token *, Select *);
 
-/**
- * DEPRECATED. All calls to be replaced w/ sql_emit_open_cursor.
- * Create cursor which will be positioned to the space/index.
- * It makes space lookup and loads pointer to it into register,
- * which is passes to OP_OpenWrite as an argument.
- *
- * @param parse Parse context.
- * @param cursor Number of cursor to be created.
- * @param entity_id Encoded space and index ids.
- * @retval address of last opcode.
- */
-int
-emit_open_cursor(struct Parse *parse, int cursor, int entity_id);
-
 /**
  * Create cursor which will be positioned to the space/index.
  * It makes space lookup and loads pointer to it into register,
@@ -4125,9 +4109,10 @@ int sqlite3VarintLen(u64 v);
 const char *sqlite3IndexAffinityStr(sqlite3 *, Index *);
 
 /**
- * Return a pointer to the column affinity string associated with index
- * pIdx. A column affinity string has one character for each column in
- * the table, according to the affinity of the column:
+ * Return a pointer to the column affinity string associated with
+ * given index. A column affinity string has one character for
+ * each column in the table, according to the affinity of the
+ * column:
  *
  *  Character      Column affinity
  *  ------------------------------
@@ -4138,12 +4123,11 @@ const char *sqlite3IndexAffinityStr(sqlite3 *, Index *);
  *  'F'            REAL
  *
  * Memory for the buffer containing the column index affinity string
- * is managed along with the rest of the Index structure. It will be
- * released when sqlite3DeleteIndex() is called.
+ * is allocated on heap.
  *
  * @param db Database handle.
  * @param def index_def where from affinity to be extracted.
- * @retval Affinity string.
+ * @retval Allocated affinity string, or NULL on OOM.
  */
 char *
 sql_index_affinity_str(struct sqlite3 *db, struct index_def *def);
@@ -4262,6 +4246,20 @@ void
 sqlite3SelectWrongNumTermsError(struct Parse *parse, struct Select *p);
 
 int sqlite3MatchSpanName(const char *, const char *, const char *);
+
+/**
+ * Create wrapper around space_def of the given space.
+ * This routine is required to support original SQLite interface,
+ * which accepts struct Table and struct Index DD arguments.
+ * Both table and space def are allocated on heap.
+ *
+ * @param space Space to be prototype.
+ * @retval New instance of struct Table allocated on malloc,
+ *         or NULL in case of OOM.
+ */
+struct Table *
+sql_table_construct_from_space(const struct space *space);
+
 int sqlite3ResolveExprNames(NameContext *, Expr *);
 int sqlite3ResolveExprListNames(NameContext *, ExprList *);
 void sqlite3ResolveSelectNames(Parse *, Select *, NameContext *);
diff --git a/src/box/sql/where.c b/src/box/sql/where.c
index f017384b5..a3a5bec81 100644
--- a/src/box/sql/where.c
+++ b/src/box/sql/where.c
@@ -42,6 +42,7 @@
 #include "tarantoolInt.h"
 #include "vdbeInt.h"
 #include "whereInt.h"
+#include "box/coll_id_cache.h"
 #include "box/session.h"
 #include "box/schema.h"
 
@@ -371,10 +372,7 @@ whereScanInit(WhereScan * pScan,	/* The WhereScan object being initialized */
 	pScan->is_column_seen = false;
 	if (pIdx) {
 		int j = iColumn;
-		if (j >= pIdx->nColumn)
-			iColumn = -1;
-		else
-			iColumn = pIdx->aiColumn[j];
+		iColumn = pIdx->aiColumn[j];
 		if (iColumn >= 0) {
 			char affinity =
 				pIdx->pTable->def->fields[iColumn].affinity;
@@ -393,12 +391,30 @@ whereScanInit(WhereScan * pScan,	/* The WhereScan object being initialized */
 	return whereScanNext(pScan);
 }
 
+/**
+ * Analogue of whereScanInit() but also can be called for spaces
+ * created via Lua interface. This function doesn't rely on
+ * regular SQLite structures representing data dictionary.
+ *
+ * @param scan The WhereScan object being initialized.
+ * @param clause The WHERE clause to be scanned.
+ * @param cursor Cursor to scan for.
+ * @param column Column to scan for.
+ * @param op_mask Operator(s) to scan for.
+ * @param space_def Def of the space related to WHERE clause.
+ * @param key_def Def of the index to be used to satisfy WHERE
+ *                clause. May be NULL.
+ *
+ * @retval Return a pointer to the first match. Return NULL if
+ *         there are no matches.
+ */
 static WhereTerm *
-sql_where_scan_init(struct WhereScan *scan, struct WhereClause *clause,
+where_scan_init(struct WhereScan *scan, struct WhereClause *clause,
 		    int cursor, int column, uint32_t op_mask,
 		    struct space_def *space_def, struct key_def *key_def)
 {
-	scan->pOrigWC = scan->pWC = clause;
+	scan->pOrigWC = clause;
+	scan->pWC = clause;
 	scan->pIdxExpr = NULL;
 	scan->idxaff = 0;
 	scan->coll = NULL;
@@ -406,11 +422,11 @@ sql_where_scan_init(struct WhereScan *scan, struct WhereClause *clause,
 	if (key_def != NULL) {
 		int j = column;
 		column = key_def->parts[j].fieldno;
-		if (column >= 0) {
-			scan->idxaff = space_def->fields[column].affinity;
-			scan->coll = key_def->parts[column].coll;
-			scan->is_column_seen = true;
-		}
+		scan->idxaff = space_def->fields[column].affinity;
+		uint32_t coll_id = space_def->fields[column].coll_id;
+		struct coll_id *coll = coll_by_id(coll_id);
+		scan->coll = coll != NULL ? coll->coll : NULL;
+		scan->is_column_seen = true;
 	}
 	scan->opMask = op_mask;
 	scan->k = 0;
@@ -472,34 +488,43 @@ sqlite3WhereFindTerm(WhereClause * pWC,	/* The WHERE clause to be searched */
 	return pResult;
 }
 
+/**
+ * Analogue of sqlite3WhereFindTerm() but also can be called
+ * for spaces created via Lua interface. This function doesn't
+ * rely on regular SQLite structures representing data
+ * dictionary.
+ *
+ * @param where_clause The WHERE clause to be examined.
+ * @param cursor Cursor number of LHS.
+ * @param column Column number of LHS
+ * @param is_ready RHS must not overlap with this mask.
+ * @param op Mask of WO_xx values describing operator.
+ * @param space_def Def of the space related to WHERE clause.
+ * @param key_def Def of the index to be used to satisfy WHERE
+ *                clause. May be NULL.
+ *
+ * @retval New struct describing WHERE term.
+ */
 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 */
+sql_where_find_term(WhereClause *where_clause, int cursor, int column,
+		    Bitmask is_ready, u32 op, struct space_def *space_def,
+		    struct key_def *key_def)
 {
-	WhereTerm *pResult = 0;
-	WhereTerm *p;
+	WhereTerm *result = NULL;
 	WhereScan scan;
-
-	p = sql_where_scan_init(&scan, pWC, iCur, iColumn, op, space_def,
-				key_def);
+	WhereTerm *p = where_scan_init(&scan, where_clause, cursor, column,
+					   op, space_def, key_def);
 	op &= WO_EQ;
-	while (p) {
-		if ((p->prereqRight & notReady) == 0) {
-			if (p->prereqRight == 0 && (p->eOperator & op) != 0) {
-				testcase(p->eOperator & WO_IS);
+	while (p != NULL) {
+		if ((p->prereqRight & is_ready) == 0) {
+			if (p->prereqRight == 0 && (p->eOperator & op) != 0)
 				return p;
-			}
-			if (pResult == 0)
-				pResult = p;
+			if (result == NULL)
+				result = p;
 		}
 		p = whereScanNext(&scan);
 	}
-	return pResult;
+	return result;
 }
 
 /*
@@ -3428,7 +3453,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 && j < pIndex->nColumn) {
+				if (pIndex != NULL) {
 					iColumn = pIndex->aiColumn[j];
 					revIdx = sql_index_column_sort_order(pIndex,
 									     j);
@@ -4096,135 +4121,125 @@ wherePathSolver(WhereInfo * pWInfo, LogEst nRowEst)
 	return SQLITE_OK;
 }
 
-/*
- * Most queries use only a single table (they are not joins) and have
- * simple == constraints against indexed fields.  This routine attempts
- * to plan those simple cases using much less ceremony than the
- * general-purpose query planner, and thereby yield faster sqlite3_prepare()
- * times for the common case.
+/**
+ * Attempt at finding appropriate terms in WHERE clause.
  *
- * Return non-zero on success, if this query can be handled by this
- * no-frills query planner.  Return zero if this query needs the
- * general-purpose query planner.
+ * @param loop The loop @where belongs to.
+ * @param where The WHERE clause to be examined..
+ * @param cursor Cursor number of LHS.
+ * @param space_def Def of the space related to WHERE clause.
+ * @param index_def Def of the index to be used to satisfy WHERE
+ *                  clause. May be NULL.
+ * @retval 0 on success, -1 otherwise.
  */
 static int
-sql_where_shortcut(struct WhereLoopBuilder *builder)
+where_assign_loop_terms(struct WhereLoop *loop, struct WhereClause *where,
+			int cursor, struct space_def *space_def,
+			struct index_def *idx_def)
 {
-	WhereInfo *pWInfo;
-	struct SrcList_item *pItem;
-	WhereClause *pWC;
-	WhereTerm *pTerm;
-	WhereLoop *pLoop;
-	int iCur;
-	int j;
+	uint32_t column_count = idx_def != NULL ? idx_def->key_def->part_count :
+				space_def->field_count;
+	if (column_count > ArraySize(loop->aLTermSpace))
+		return -1;
+	uint32_t i;
+	for (i = 0; i < column_count; ++i) {
+		struct WhereTerm *term =
+			sql_where_find_term(where, cursor, i, 0, WO_EQ,
+					    space_def, idx_def != NULL ?
+					    idx_def->key_def : NULL);
+		if (term == NULL)
+			break;
+		testcase(pTerm->eOperator & WO_IS);
+		loop->aLTerm[i] = term;
+	}
+	if (i != column_count)
+		return -1;
+	loop->wsFlags = WHERE_COLUMN_EQ | WHERE_ONEROW | WHERE_INDEXED |
+			WHERE_IDX_ONLY;
+	loop->nLTerm = i;
+	loop->nEq = i;
+	loop->index_def = idx_def;
+	/* TUNING: Cost of a unique index lookup is 15. */
+	assert(39 == sqlite3LogEst(15));
+	loop->rRun = 39;
+	return 0;
+}
 
-	pWInfo = builder->pWInfo;
-	if (pWInfo->wctrlFlags & WHERE_OR_SUBCLAUSE)
+/**
+ * Most queries use only a single table (they are not joins) and
+ * have simple == constraints against indexed fields. This
+ * routine attempts to plan those simple cases using much less
+ * ceremony than the general-purpose query planner, and thereby
+ * yield faster sqlite3_prepare() times for the common case.
+ *
+ * @param builder Where-Loop Builder.
+ * @retval Return non-zero on success, i.e. if this query can be
+ *         handled by this no-frills query planner. Return zero
+ *         if this query needs the general-purpose query planner.
+ */
+static int
+sql_where_shortcut(struct WhereLoopBuilder *builder)
+{
+	struct WhereInfo *where_info = builder->pWInfo;
+	if (where_info->wctrlFlags & WHERE_OR_SUBCLAUSE)
 		return 0;
-	assert(pWInfo->pTabList->nSrc >= 1);
-	pItem = pWInfo->pTabList->a;
-	struct space_def *space_def = pItem->pTab->def;
+	assert(where_info->pTabList->nSrc >= 1);
+	struct SrcList_item *item = where_info->pTabList->a;
+	struct space_def *space_def = item->pTab->def;
 	assert(space_def != NULL);
-
-	if (pItem->fg.isIndexedBy)
+	if (item->fg.isIndexedBy)
 		return 0;
-	iCur = pItem->iCursor;
-	pWC = &pWInfo->sWC;
-	pLoop = builder->pNew;
-	pLoop->wsFlags = 0;
-	pLoop->nSkip = 0;
-	pTerm = sqlite3WhereFindTerm(pWC, iCur, -1, 0, WO_EQ, 0);
-	if (pTerm) {
-		pLoop->wsFlags = WHERE_COLUMN_EQ | WHERE_IPK | WHERE_ONEROW;
-		pLoop->aLTerm[0] = pTerm;
-		pLoop->nLTerm = 1;
-		pLoop->nEq = 1;
-		/* TUNING: Cost of a PK lookup is 10 */
-		pLoop->rRun = 33;	/* 33==sqlite3LogEst(10) */
+	int cursor = item->iCursor;
+	struct WhereClause *clause = &where_info->sWC;
+	struct WhereLoop *loop = builder->pNew;
+	loop->wsFlags = 0;
+	loop->nSkip = 0;
+	loop->pIndex = NULL;
+	struct WhereTerm *term = sqlite3WhereFindTerm(clause, cursor, -1, 0,
+						      WO_EQ, 0);
+	if (term != NULL) {
+		loop->wsFlags = WHERE_COLUMN_EQ | WHERE_IPK | WHERE_ONEROW;
+		loop->aLTerm[0] = term;
+		loop->nLTerm = 1;
+		loop->nEq = 1;
+		/* TUNING: Cost of a PK lookup is 10. */
+		assert(33 == sqlite3LogEst(10));
+		loop->rRun = 33;
 	} else {
-		struct space *space = space_cache_find(space_def->id);
+		assert(loop->aLTermSpace == loop->aLTerm);
+		struct space *space = space_by_id(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)
-					)
+				struct index_def *idx_def =
+					space->index[i]->def;
+				if (!idx_def->opts.is_unique)
 					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;
+				if (where_assign_loop_terms(loop, clause,
+							    cursor, space_def,
+							    idx_def) == 0)
+					break;
 			}
 		} else {
-			/* Space is ephemeral.  */
+			/* Space is ephemeral. */
 			assert(space_def->id == 0);
-			int opMask;
-			int nIdxCol = space_def->field_count;
-			assert(pLoop->aLTermSpace == pLoop->aLTerm);
-			if ( nIdxCol > ArraySize(pLoop->aLTermSpace))
-				return 0;
-			opMask = WO_EQ;
-			for (j = 0; j < nIdxCol; j++) {
-				pTerm = sql_where_find_term(pWC, iCur,
-							    j, 0,
-							    opMask,
-							    space_def,
-							    NULL);
-				if (pTerm == NULL)
-					break;
-				pLoop->aLTerm[j] = pTerm;
-			}
-			if (j != nIdxCol)
-				return 0;
-			pLoop->wsFlags = WHERE_COLUMN_EQ | WHERE_ONEROW |
-					 WHERE_INDEXED | WHERE_IDX_ONLY;
-			pLoop->nLTerm = j;
-			pLoop->nEq = j;
-			pLoop->pIndex = NULL;
-			pLoop->index_def = NULL;
-			/* TUNING: Cost of a unique index lookup is 15 */
-			pLoop->rRun = 39;	/* 39==sqlite3LogEst(15) */
+			where_assign_loop_terms(loop, clause, cursor,
+						space_def, NULL);
 		}
 	}
-	if (pLoop->wsFlags) {
-		pLoop->nOut = (LogEst) 1;
-		pWInfo->a[0].pWLoop = pLoop;
-		pLoop->maskSelf = sqlite3WhereGetMask(&pWInfo->sMaskSet, iCur);
-		pWInfo->a[0].iTabCur = iCur;
-		pWInfo->nRowOut = 1;
-		if (pWInfo->pOrderBy)
-			pWInfo->nOBSat = pWInfo->pOrderBy->nExpr;
-		if (pWInfo->wctrlFlags & WHERE_WANT_DISTINCT) {
-			pWInfo->eDistinct = WHERE_DISTINCT_UNIQUE;
+	if (loop->wsFlags) {
+		loop->nOut = (LogEst) 1;
+		where_info->a[0].pWLoop = loop;
+		loop->maskSelf = sqlite3WhereGetMask(&where_info->sMaskSet,
+						     cursor);
+		where_info->a[0].iTabCur = cursor;
+		where_info->nRowOut = 1;
+		if (where_info->pOrderBy)
+			where_info->nOBSat = where_info->pOrderBy->nExpr;
+		if (where_info->wctrlFlags & WHERE_WANT_DISTINCT) {
+			where_info->eDistinct = WHERE_DISTINCT_UNIQUE;
 		}
 #ifdef SQLITE_DEBUG
-		pLoop->cId = '0';
+		loop->cId = '0';
 #endif
 		return 1;
 	}
@@ -4704,8 +4719,7 @@ sqlite3WhereBegin(Parse * pParse,	/* The parser context */
 			 */
 			if (idx_def == NULL && pIx == NULL) continue;
 			bool is_primary = (pIx != NULL && IsPrimaryKeyIndex(pIx)) ||
-					   (idx_def != NULL && (idx_def->iid == 0)) |
-				(idx_def == NULL && pIx == NULL);
+					   (idx_def != NULL && (idx_def->iid == 0));
 			if (is_primary
 			    && (wctrlFlags & WHERE_OR_SUBCLAUSE) != 0) {
 				/* This is one term of an OR-optimization using
@@ -4752,9 +4766,10 @@ sqlite3WhereBegin(Parse * pParse,	/* The parser context */
 			assert(iIndexCur >= 0);
 			if (op) {
 				if (pIx != NULL) {
-					emit_open_cursor(pParse, iIndexCur,
-							 pIx->tnum);
-					sql_vdbe_set_p4_key_def(pParse, pIx);
+					struct space *space =
+						space_by_id(SQLITE_PAGENO_TO_SPACEID(pIx->tnum));
+					sql_emit_open_cursor(pParse, iIndexCur,
+							     pIx->tnum, space);
 				} else {
 					sql_emit_open_cursor(pParse, iIndexCur,
 							     idx_def->iid,
diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c
index 60e0ac7e4..eaab0b657 100644
--- a/src/box/sql/wherecode.c
+++ b/src/box/sql/wherecode.c
@@ -87,7 +87,7 @@ explainAppendTerm(StrAccum * pStr,	/* The text expression being built */
 			assert(def != NULL);
                         struct space *space = space_cache_find(def->space_id);
                         assert(space != NULL);
-                        name = space->def->fields[i].name;
+                        name = space->def->fields[i + iTerm].name;
 		}
 		sqlite3StrAccumAppendAll(pStr, name);
 	}
@@ -143,7 +143,8 @@ explainIndexRange(StrAccum * pStr, WhereLoop * pLoop)
 		} else {
 			struct space *space = space_cache_find(def->space_id);
 			assert(space != NULL);
-			z = space->def->fields[i].name;
+			uint32_t fieldno = def->key_def->parts[i].fieldno;
+			z = space->def->fields[fieldno].name;
 		}
 		if (i)
 			sqlite3StrAccumAppend(pStr, " AND ", 5);
-- 
2.15.1

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

[Vladislav Shpilevoy]

Hello. Thanks for the patch!

See 6 comments below.

On 18/06/2018 05:11, n.pettik wrote:
> For some reason this patch got to the trunk without review fixes.
> Raw version of this patch contains several bugs, so they led to test fails
> (sql-tap/analyze6.test.lua and few sql-tap/tkt*).
> 
> I suggest my own fixes. The whole patch is at the end of letter.
> 
> Branch: https://github.com/tarantool/tarantool/commits/np/gh-3463-review-fixed-for-point-delete
> Issue: https://github.com/tarantool/tarantool/issues/3463
> 
>>
>>>   }
>>>     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.
> 
> Ok, I just replaced all calls of emit_open_cursor() with sql_emit_open_cursor().
> The only difference is in doing space lookup outside emit_open_cursor().

1. How about rename this function to parser_emit_open_cursor?
According to Tarantool naming convention a method name should
be called as 'object_action_subject'.

Now we have too many different prefixes of parser methods:
sqlite3, sql, parse, parser, <no_prefix>. Lets use 'parser'.

>>
>>> +}
>>> 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.
> 
> Refactored this way:
> 
> +++ b/src/box/sql/delete.c
> @@ -104,17 +104,28 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>                                                  strlen(tab_name));
>                  if (space_id == BOX_ID_NIL)
>                          goto delete_from_cleanup;
> +               /*
> +                * In this case space has been created via Lua
> +                * facilities, so there is no corresponding entry
> +                * in table hash. Thus, lets create simple
> +                * wrapper around space_def to support interface.
> +                */
> +               space = space_by_id(space_id);
> +               tab_list->a[0].pTab = sql_table_construct_from_space(space);
> +               if (tab_list->a[0].pTab == NULL)
> +                       goto delete_from_cleanup;

2. As I proposed in the review, why can not we use stack struct Table here?
Just declare it at the beginning of the function and here save its pointer.


> @@ -107,32 +107,22 @@ sqlite3IndexAffinityStr(sqlite3 *db, Index *index)
>   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.
> -        *
> -        * 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);
> +       uint32_t column_count = def->key_def->part_count;
> +       char *aff = (char *)sqlite3DbMallocRaw(0, column_count + 1);
>          if (aff == NULL) {
>                  sqlite3OomFault(db);
> -               return 0;
> +               return NULL;
>          }

3. Why not just pass db to sqlite3DbMallocRaw? It does OomFault for us.

As I see in codeAllEqualityTerms() (where this function is used)
affinity is either allocated as sqlite3DbStrDup(pParse->db) or
in sql_index_affinity_str() using sqlite3DbMallocRaw(NULL).

Different ways of allocating (with db or not) and freeing leads
to very surprising assertions sometimes.

> -       int i;
> -       struct space *space = space_cache_find(def->space_id);
> +       struct space *space = space_by_id(def->space_id);
>          assert(space != NULL);
>   
> -       for (i = 0; i < nColumn; i++) {
> +       for (uint32_t i = 0; i < column_count; 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;
> +       aff[column_count] = '\0';
>   
>          return aff;
>   }
> 
> =======================================================================
> 
>  From 9ced467a2f33ce02ac48f875db06d3eec7d5561d Mon Sep 17 00:00:00 2001
> From: Nikita Pettik <korablev@tarantool.org>
> Date: Mon, 18 Jun 2018 00:38:55 +0300
> Subject: [PATCH] sql: review fixes for a40287051
> 
> ---
>   src/box/sql/analyze.c   |   6 +-
>   src/box/sql/build.c     |  27 +----
>   src/box/sql/delete.c    |  45 +++----
>   src/box/sql/expr.c      |   9 +-
>   src/box/sql/fkey.c      |   7 +-
>   src/box/sql/insert.c    |  36 +++---
>   src/box/sql/resolve.c   |  18 +++
>   src/box/sql/select.c    |   5 +-
>   src/box/sql/sqliteInt.h |  42 ++++---
>   src/box/sql/where.c     | 309 +++++++++++++++++++++++++-----------------------
>   src/box/sql/wherecode.c |   5 +-
>   11 files changed, 256 insertions(+), 253 deletions(-)
>> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
> index 23e16189a..e7d9723fa 100644
> --- a/src/box/sql/resolve.c
> +++ b/src/box/sql/resolve.c
> @@ -39,6 +39,9 @@
>   #include <stdlib.h>
>   #include <string.h>
>   
> +#include "box/box.h"
> +#include "box/schema.h"

4. Why do you need these headers?

> +
>   /*
>    * Walk the expression tree pExpr and increase the aggregate function
>    * depth (the Expr.op2 field) by N on every TK_AGG_FUNCTION node.
> diff --git a/src/box/sql/where.c b/src/box/sql/where.c
> index f017384b5..a3a5bec81 100644
> --- a/src/box/sql/where.c
> +++ b/src/box/sql/where.c
> @@ -4096,135 +4121,125 @@ wherePathSolver(WhereInfo * pWInfo, LogEst nRowEst)
>   	return SQLITE_OK;
>   }
>   
> -/*
> - * Most queries use only a single table (they are not joins) and have
> - * simple == constraints against indexed fields.  This routine attempts
> - * to plan those simple cases using much less ceremony than the
> - * general-purpose query planner, and thereby yield faster sqlite3_prepare()
> - * times for the common case.
> +/**
> + * Attempt at finding appropriate terms in WHERE clause.
>    *
> - * Return non-zero on success, if this query can be handled by this
> - * no-frills query planner.  Return zero if this query needs the
> - * general-purpose query planner.
> + * @param loop The loop @where belongs to.
> + * @param where The WHERE clause to be examined..
> + * @param cursor Cursor number of LHS.
> + * @param space_def Def of the space related to WHERE clause.
> + * @param index_def Def of the index to be used to satisfy WHERE
> + *                  clause. May be NULL.
> + * @retval 0 on success, -1 otherwise.
>    */
>   static int
> -sql_where_shortcut(struct WhereLoopBuilder *builder)
> +where_assign_loop_terms(struct WhereLoop *loop, struct WhereClause *where,
> +			int cursor, struct space_def *space_def,
> +			struct index_def *idx_def)

5. Why WhereClause new method has prefix 'sql_where_', but WhereLoop method has prefix
'where_'? Lets make it 'where_clause_find_term' and 'where_loop_assign_terms'.

> +/**
> + * Most queries use only a single table (they are not joins) and
> + * have simple == constraints against indexed fields. This
> + * routine attempts to plan those simple cases using much less
> + * ceremony than the general-purpose query planner, and thereby
> + * yield faster sqlite3_prepare() times for the common case.
> + *
> + * @param builder Where-Loop Builder.
> + * @retval Return non-zero on success, i.e. if this query can be
> + *         handled by this no-frills query planner. Return zero
> + *         if this query needs the general-purpose query planner.
> + */
> +static int
> +sql_where_shortcut(struct WhereLoopBuilder *builder)

6. Same. For example, 'where_loop_builder_shortcut'.

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

[Vladislav Shpilevoy]

Forgot to say: I have pushed some
other minor fixes on the branch.

On 18/06/2018 13:44, Vladislav Shpilevoy wrote:
> Hello. Thanks for the patch!
> 
> See 6 comments below.
> 
> On 18/06/2018 05:11, n.pettik wrote:
>> For some reason this patch got to the trunk without review fixes.
>> Raw version of this patch contains several bugs, so they led to test fails
>> (sql-tap/analyze6.test.lua and few sql-tap/tkt*).
>>
>> I suggest my own fixes. The whole patch is at the end of letter.
>>
>> Branch: https://github.com/tarantool/tarantool/commits/np/gh-3463-review-fixed-for-point-delete
>> Issue: https://github.com/tarantool/tarantool/issues/3463
>>
>>>
>>>>   }
>>>>     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.
>>
>> Ok, I just replaced all calls of emit_open_cursor() with sql_emit_open_cursor().
>> The only difference is in doing space lookup outside emit_open_cursor().
> 
> 1. How about rename this function to parser_emit_open_cursor?
> According to Tarantool naming convention a method name should
> be called as 'object_action_subject'.
> 
> Now we have too many different prefixes of parser methods:
> sqlite3, sql, parse, parser, <no_prefix>. Lets use 'parser'.
> 
>>>
>>>> +}
>>>> 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.
>>
>> Refactored this way:
>>
>> +++ b/src/box/sql/delete.c
>> @@ -104,17 +104,28 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>>                                                  strlen(tab_name));
>>                  if (space_id == BOX_ID_NIL)
>>                          goto delete_from_cleanup;
>> +               /*
>> +                * In this case space has been created via Lua
>> +                * facilities, so there is no corresponding entry
>> +                * in table hash. Thus, lets create simple
>> +                * wrapper around space_def to support interface.
>> +                */
>> +               space = space_by_id(space_id);
>> +               tab_list->a[0].pTab = sql_table_construct_from_space(space);
>> +               if (tab_list->a[0].pTab == NULL)
>> +                       goto delete_from_cleanup;
> 
> 2. As I proposed in the review, why can not we use stack struct Table here?
> Just declare it at the beginning of the function and here save its pointer.
> 
> 
>> @@ -107,32 +107,22 @@ sqlite3IndexAffinityStr(sqlite3 *db, Index *index)
>>   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.
>> -        *
>> -        * 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);
>> +       uint32_t column_count = def->key_def->part_count;
>> +       char *aff = (char *)sqlite3DbMallocRaw(0, column_count + 1);
>>          if (aff == NULL) {
>>                  sqlite3OomFault(db);
>> -               return 0;
>> +               return NULL;
>>          }
> 
> 3. Why not just pass db to sqlite3DbMallocRaw? It does OomFault for us.
> 
> As I see in codeAllEqualityTerms() (where this function is used)
> affinity is either allocated as sqlite3DbStrDup(pParse->db) or
> in sql_index_affinity_str() using sqlite3DbMallocRaw(NULL).
> 
> Different ways of allocating (with db or not) and freeing leads
> to very surprising assertions sometimes.
> 
>> -       int i;
>> -       struct space *space = space_cache_find(def->space_id);
>> +       struct space *space = space_by_id(def->space_id);
>>          assert(space != NULL);
>> -       for (i = 0; i < nColumn; i++) {
>> +       for (uint32_t i = 0; i < column_count; 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;
>> +       aff[column_count] = '\0';
>>          return aff;
>>   }
>>
>> =======================================================================
>>
>>  From 9ced467a2f33ce02ac48f875db06d3eec7d5561d Mon Sep 17 00:00:00 2001
>> From: Nikita Pettik <korablev@tarantool.org>
>> Date: Mon, 18 Jun 2018 00:38:55 +0300
>> Subject: [PATCH] sql: review fixes for a40287051
>>
>> ---
>>   src/box/sql/analyze.c   |   6 +-
>>   src/box/sql/build.c     |  27 +----
>>   src/box/sql/delete.c    |  45 +++----
>>   src/box/sql/expr.c      |   9 +-
>>   src/box/sql/fkey.c      |   7 +-
>>   src/box/sql/insert.c    |  36 +++---
>>   src/box/sql/resolve.c   |  18 +++
>>   src/box/sql/select.c    |   5 +-
>>   src/box/sql/sqliteInt.h |  42 ++++---
>>   src/box/sql/where.c     | 309 +++++++++++++++++++++++++-----------------------
>>   src/box/sql/wherecode.c |   5 +-
>>   11 files changed, 256 insertions(+), 253 deletions(-)
>>> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
>> index 23e16189a..e7d9723fa 100644
>> --- a/src/box/sql/resolve.c
>> +++ b/src/box/sql/resolve.c
>> @@ -39,6 +39,9 @@
>>   #include <stdlib.h>
>>   #include <string.h>
>> +#include "box/box.h"
>> +#include "box/schema.h"
> 
> 4. Why do you need these headers?
> 
>> +
>>   /*
>>    * Walk the expression tree pExpr and increase the aggregate function
>>    * depth (the Expr.op2 field) by N on every TK_AGG_FUNCTION node.
>> diff --git a/src/box/sql/where.c b/src/box/sql/where.c
>> index f017384b5..a3a5bec81 100644
>> --- a/src/box/sql/where.c
>> +++ b/src/box/sql/where.c
>> @@ -4096,135 +4121,125 @@ wherePathSolver(WhereInfo * pWInfo, LogEst nRowEst)
>>       return SQLITE_OK;
>>   }
>> -/*
>> - * Most queries use only a single table (they are not joins) and have
>> - * simple == constraints against indexed fields.  This routine attempts
>> - * to plan those simple cases using much less ceremony than the
>> - * general-purpose query planner, and thereby yield faster sqlite3_prepare()
>> - * times for the common case.
>> +/**
>> + * Attempt at finding appropriate terms in WHERE clause.
>>    *
>> - * Return non-zero on success, if this query can be handled by this
>> - * no-frills query planner.  Return zero if this query needs the
>> - * general-purpose query planner.
>> + * @param loop The loop @where belongs to.
>> + * @param where The WHERE clause to be examined..
>> + * @param cursor Cursor number of LHS.
>> + * @param space_def Def of the space related to WHERE clause.
>> + * @param index_def Def of the index to be used to satisfy WHERE
>> + *                  clause. May be NULL.
>> + * @retval 0 on success, -1 otherwise.
>>    */
>>   static int
>> -sql_where_shortcut(struct WhereLoopBuilder *builder)
>> +where_assign_loop_terms(struct WhereLoop *loop, struct WhereClause *where,
>> +            int cursor, struct space_def *space_def,
>> +            struct index_def *idx_def)
> 
> 5. Why WhereClause new method has prefix 'sql_where_', but WhereLoop method has prefix
> 'where_'? Lets make it 'where_clause_find_term' and 'where_loop_assign_terms'.
> 
>> +/**
>> + * Most queries use only a single table (they are not joins) and
>> + * have simple == constraints against indexed fields. This
>> + * routine attempts to plan those simple cases using much less
>> + * ceremony than the general-purpose query planner, and thereby
>> + * yield faster sqlite3_prepare() times for the common case.
>> + *
>> + * @param builder Where-Loop Builder.
>> + * @retval Return non-zero on success, i.e. if this query can be
>> + *         handled by this no-frills query planner. Return zero
>> + *         if this query needs the general-purpose query planner.
>> + */
>> +static int
>> +sql_where_shortcut(struct WhereLoopBuilder *builder)
> 
> 6. Same. For example, 'where_loop_builder_shortcut'.
> 
> 

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

[n.pettik]

Thanks for fixes, I have squashed them.

> 1. How about rename this function to parser_emit_open_cursor?
> According to Tarantool naming convention a method name should
> be called as 'object_action_subject'.
> 
> Now we have too many different prefixes of parser methods:
> sqlite3, sql, parse, parser, <no_prefix>. Lets use 'parser’.

Ok, but I decided to call it vdbe_emit_open_cursor() since all staff connected
with code generation rather belongs to vdbe:

+++ b/src/box/sql/analyze.c
@@ -178,7 +178,7 @@ openStatTable(Parse * pParse,       /* Parsing context */
        for (i = 0; aTable[i]; i++) {
                struct space *space =
                        space_by_id(SQLITE_PAGENO_TO_SPACEID(aRoot[i]));
-               sql_emit_open_cursor(pParse, iStatCur + i, aRoot[i], space);
+               vdbe_emit_open_cursor(pParse, iStatCur + i, aRoot[i], space);

+++ b/src/box/sql/build.c
@@ -1181,8 +1181,8 @@ space_checks_expr_list(uint32_t space_id)
 }
 
 int
-sql_emit_open_cursor(struct Parse *parse_context, int cursor, int index_id,
-                    struct space *space)
+vdbe_emit_open_cursor(struct Parse *parse_context, int cursor, int index_id,
+                     struct space *space)
 {
        assert(space != NULL);
        struct Vdbe *vdbe = parse_context->pVdbe;
@@ -2651,7 +2651,7 @@ sqlite3RefillIndex(Parse * pParse, Index * pIndex, int memRootPage)
                sqlite3VdbeAddOp2(v, OP_Clear, SQLITE_PAGENO_TO_SPACEID(tnum),
                                  0);
        struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(tnum));
-       sql_emit_open_cursor(pParse, iIdx, tnum, space);
+       vdbe_emit_open_cursor(pParse, iIdx, tnum, space);


+++ b/src/box/sql/expr.c
@@ -2489,8 +2489,8 @@ sqlite3FindInIndex(Parse * pParse,        /* Parsing context */
                                                          P4_DYNAMIC);
                                        struct space *space =
                                                space_by_id(SQLITE_PAGENO_TO_SPACEID(pIdx->tnum));
-                                       sql_emit_open_cursor(pParse, iTab,
-                                                            pIdx->tnum, space);
+                                       vdbe_emit_open_cursor(pParse, iTab,
+                                                             pIdx->tnum, space);

+++ b/src/box/sql/insert.c
@@ -55,7 +55,7 @@ sqlite3OpenTable(Parse * pParse,      /* Generate code into this VDBE */
        assert(pPk != 0);
        assert(pPk->tnum == pTab->tnum);
        struct space *space = space_by_id(SQLITE_PAGENO_TO_SPACEID(pPk->tnum));
-       sql_emit_open_cursor(pParse, iCur, pPk->tnum, space);
+       vdbe_emit_open_cursor(pParse, iCur, pPk->tnum, space);

@@ -1943,11 +1941,11 @@ xferOptimization(Parse * pParse,        /* Parser context */
                assert(pSrcIdx);
                struct space *src_space =
                        space_by_id(SQLITE_PAGENO_TO_SPACEID(pSrcIdx->tnum));
-               sql_emit_open_cursor(pParse, iSrc, pSrcIdx->tnum, src_space);
+               vdbe_emit_open_cursor(pParse, iSrc, pSrcIdx->tnum, src_space);
                VdbeComment((v, "%s", pSrcIdx->zName));
                struct space *dest_space =
                        space_by_id(SQLITE_PAGENO_TO_SPACEID(pDestIdx->tnum));
-               sql_emit_open_cursor(pParse, iDest, pDestIdx->tnum, dest_space);
+               vdbe_emit_open_cursor(pParse, iDest, pDestIdx->tnum, dest_space);

+++ b/src/box/sql/select.c
@@ -6107,9 +6107,9 @@ sqlite3Select(Parse * pParse,             /* The parser context */
                                 * Open the cursor, execute the OP_Count,
                                 * close the cursor.
                                 */
-                               sql_emit_open_cursor(pParse, cursor,
-                                                    space->def->id << 10,
-                                                    space);
+                               vdbe_emit_open_cursor(pParse, cursor,
+                                                     space->def->id << 10,
+                                                     space);

+++ b/src/box/sql/sqliteInt.h
@@ -3564,8 +3564,8 @@ void sqlite3EndTable(Parse *, Token *, Token *, Select *);
  * @retval address of last opcode.
  */
 int
-sql_emit_open_cursor(struct Parse *parse, int cursor, int index_id,
-                    struct space *space);
+vdbe_emit_open_cursor(struct Parse *parse, int cursor, int index_id,
+                     struct space *space);


>>> 
>>>> +}
>>>> 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.
>> Refactored this way:
>> +++ b/src/box/sql/delete.c
>> @@ -104,17 +104,28 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>>                                                 strlen(tab_name));
>>                 if (space_id == BOX_ID_NIL)
>>                         goto delete_from_cleanup;
>> +               /*
>> +                * In this case space has been created via Lua
>> +                * facilities, so there is no corresponding entry
>> +                * in table hash. Thus, lets create simple
>> +                * wrapper around space_def to support interface.
>> +                */
>> +               space = space_by_id(space_id);
>> +               tab_list->a[0].pTab = sql_table_construct_from_space(space);
>> +               if (tab_list->a[0].pTab == NULL)
>> +                       goto delete_from_cleanup;
> 
> 2. As I proposed in the review, why can not we use stack struct Table here?
> Just declare it at the beginning of the function and here save its pointer.

Well, OK it is possible:

+++ b/src/box/sql/delete.c
@@ -111,9 +111,12 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
                 * wrapper around space_def to support interface.
                 */
                space = space_by_id(space_id);
-               tab_list->a[0].pTab = sql_table_construct_from_space(space);
-               if (tab_list->a[0].pTab == NULL)
-                       goto delete_from_cleanup;
+               struct Table tmp_tab;
+               memset(&tmp_tab, 0, sizeof(tmp_tab));
+               tmp_tab.def = space->def;
+               /* Prevent from freeing memory in DeleteTable. */
+               tmp_tab.nTabRef = 2;
+               tab_list->a[0].pTab = &tmp_tab;

-struct Table *
-sql_table_construct_from_space(const struct space *space)
-{
-       assert(space != NULL);
-       struct Table *table = calloc(1, sizeof(*table));
-       if (table == NULL) {
-               diag_set(OutOfMemory, sizeof(table), "calloc", "table");
-               return NULL;
-       }
-       table->def = space_def_dup(space->def);
-       if (table->def == NULL)
-               return NULL;
-       return table;
-}
-

-
-/**
- * Create wrapper around space_def of the given space.
- * This routine is required to support original SQLite interface,
- * which accepts struct Table and struct Index DD arguments.
- * Both table and space def are allocated on heap.
- *
- * @param space Space to be prototype.
- * @retval New instance of struct Table allocated on malloc,
- *         or NULL in case of OOM.
- */
-struct Table *
-sql_table_construct_from_space(const struct space *space);
-

> 
> 
>> @@ -107,32 +107,22 @@ sqlite3IndexAffinityStr(sqlite3 *db, Index *index)
>>  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.
>> -        *
>> -        * 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);
>> +       uint32_t column_count = def->key_def->part_count;
>> +       char *aff = (char *)sqlite3DbMallocRaw(0, column_count + 1);
>>         if (aff == NULL) {
>>                 sqlite3OomFault(db);
>> -               return 0;
>> +               return NULL;
>>         }
> 
> 3. Why not just pass db to sqlite3DbMallocRaw? It does OomFault for us.

Ok:

@@ -108,11 +108,9 @@ char *
 sql_index_affinity_str(struct sqlite3 *db, struct index_def *def)
 {
        uint32_t column_count = def->key_def->part_count;
-       char *aff = (char *)sqlite3DbMallocRaw(0, column_count + 1);
-       if (aff == NULL) {
-               sqlite3OomFault(db);
+       char *aff = (char *)sqlite3DbMallocRaw(db, column_count + 1);
+       if (aff == NULL)
                return NULL;
-       }

>>  11 files changed, 256 insertions(+), 253 deletions(-)
>>> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
>> index 23e16189a..e7d9723fa 100644
>> --- a/src/box/sql/resolve.c
>> +++ b/src/box/sql/resolve.c
>> @@ -39,6 +39,9 @@
>>  #include <stdlib.h>
>>  #include <string.h>
>>  +#include "box/box.h"
>> +#include "box/schema.h"
> 
> 4. Why do you need these headers?

Looks like I forgot to remove them. Anyway, in your fixes you removed them. Thx.

>>   */
>>  static int
>> -sql_where_shortcut(struct WhereLoopBuilder *builder)
>> +where_assign_loop_terms(struct WhereLoop *loop, struct WhereClause *where,
>> +			int cursor, struct space_def *space_def,
>> +			struct index_def *idx_def)
> 
> 5. Why WhereClause new method has prefix 'sql_where_', but WhereLoop method has prefix
> 'where_'? Lets make it 'where_clause_find_term' and 'where_loop_assign_terms’.

-sql_where_find_term(struct WhereClause *where_clause, int cursor, int column,
-                   Bitmask is_ready, u32 op, struct space_def *space_def,
-                   struct key_def *key_def)
+where_clase_find_term(struct WhereClause *where_clause, int cursor, int column,
+               Bitmask is_ready, u32 op, struct space_def *space_def,
+               struct key_def *key_def)

@@ -4133,7 +4133,7 @@ wherePathSolver(WhereInfo * pWInfo, LogEst nRowEst)
  * @retval 0 on success, -1 otherwise.
  */
 static int
-where_assign_loop_terms(struct WhereLoop *loop, struct WhereClause *where,
+where_loop_assign_terms(struct WhereLoop *loop, struct WhereClause *where,
                        int cursor, struct space_def *space_def,
                        struct index_def *idx_def)

@@ -4144,9 +4144,9 @@ where_assign_loop_terms(struct WhereLoop *loop, struct WhereClause *where,
        uint32_t i;
        for (i = 0; i < column_count; ++i) {
                struct WhereTerm *term =
-                       sql_where_find_term(where, cursor, i, 0, WO_EQ,
-                                           space_def, idx_def != NULL ?
-                                           idx_def->key_def : NULL);
+                       where_clause_find_term(where, cursor, i, 0, WO_EQ,
+                                              space_def, idx_def != NULL ?
+                                              idx_def->key_def : NULL);

@@ -4178,7 +4178,7 @@ where_assign_loop_terms(struct WhereLoop *loop, struct WhereClause *where,
  *         if this query needs the general-purpose query planner.
  */
 static int
-sql_where_shortcut(struct WhereLoopBuilder *builder)
+where_loop_builder_shortcut(struct WhereLoopBuilder *builder)
 {
        struct WhereInfo *where_info = builder->pWInfo;
        if (where_info->wctrlFlags & WHERE_OR_SUBCLAUSE)
@@ -4214,7 +4214,7 @@ sql_where_shortcut(struct WhereLoopBuilder *builder)
                                        space->index[i]->def;
                                if (!idx_def->opts.is_unique)
                                        continue;
-                               if (where_assign_loop_terms(loop, clause,
+                               if (where_loop_assign_terms(loop, clause,
                                                            cursor, space_def,
                                                            idx_def) == 0)
                                        break;
@@ -4222,7 +4222,7 @@ sql_where_shortcut(struct WhereLoopBuilder *builder)
                } else {
                        /* Space is ephemeral. */
                        assert(space_def->id == 0);
-                       where_assign_loop_terms(loop, clause, cursor,
+                       where_loop_assign_terms(loop, clause, cursor,
                                                space_def, NULL);

@@ -4537,7 +4537,7 @@ sqlite3WhereBegin(Parse * pParse, /* The parser context */
        }
 #endif
 
-       if (nTabList != 1 || sql_where_shortcut(&sWLB) == 0) {
+       if (nTabList != 1 || where_loop_builder_shortcut(&sWLB) == 0) {

> 
>> +/**
>> + * Most queries use only a single table (they are not joins) and
>> + * have simple == constraints against indexed fields. This
>> + * routine attempts to plan those simple cases using much less
>> + * ceremony than the general-purpose query planner, and thereby
>> + * yield faster sqlite3_prepare() times for the common case.
>> + *
>> + * @param builder Where-Loop Builder.
>> + * @retval Return non-zero on success, i.e. if this query can be
>> + *         handled by this no-frills query planner. Return zero
>> + *         if this query needs the general-purpose query planner.
>> + */
>> +static int
>> +sql_where_shortcut(struct WhereLoopBuilder *builder)
> 
> 6. Same. For example, 'where_loop_builder_shortcut’.

Fixed. See code above.

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

[Vladislav Shpilevoy]

Thanks for the fixes! See 2 comments below.

>> 2. As I proposed in the review, why can not we use stack struct Table here?
>> Just declare it at the beginning of the function and here save its pointer.
> 
> Well, OK it is possible:
> 
> +++ b/src/box/sql/delete.c
> @@ -111,9 +111,12 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>                   * wrapper around space_def to support interface.
>                   */
>                  space = space_by_id(space_id);
> -               tab_list->a[0].pTab = sql_table_construct_from_space(space);
> -               if (tab_list->a[0].pTab == NULL)
> -                       goto delete_from_cleanup;
> +               struct Table tmp_tab;
> +               memset(&tmp_tab, 0, sizeof(tmp_tab));
> +               tmp_tab.def = space->def;
> +               /* Prevent from freeing memory in DeleteTable. */
> +               tmp_tab.nTabRef = 2;
> +               tab_list->a[0].pTab = &tmp_tab;

1. Are you sure you can save the pointer to the stack value and then
use it out of the declaration context?

Here tmp_tab is declared inside {} block, but pointer on it is
used out the former.
>>>   11 files changed, 256 insertions(+), 253 deletions(-)
>>>> diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
>>> index 23e16189a..e7d9723fa 100644
>>> --- a/src/box/sql/resolve.c
>>> +++ b/src/box/sql/resolve.c
>>> @@ -39,6 +39,9 @@
>>>   #include <stdlib.h>
>>>   #include <string.h>
>>>   +#include "box/box.h"
>>> +#include "box/schema.h"
>>
>> 4. Why do you need these headers?
> 
> Looks like I forgot to remove them. Anyway, in your fixes you removed them. Thx.
> 
>>>    */
>>>   static int
>>> -sql_where_shortcut(struct WhereLoopBuilder *builder)
>>> +where_assign_loop_terms(struct WhereLoop *loop, struct WhereClause *where,
>>> +			int cursor, struct space_def *space_def,
>>> +			struct index_def *idx_def)
>>
>> 5. Why WhereClause new method has prefix 'sql_where_', but WhereLoop method has prefix
>> 'where_'? Lets make it 'where_clause_find_term' and 'where_loop_assign_terms’.
> 
> -sql_where_find_term(struct WhereClause *where_clause, int cursor, int column,
> -                   Bitmask is_ready, u32 op, struct space_def *space_def,
> -                   struct key_def *key_def)
> +where_clase_find_term(struct WhereClause *where_clause, int cursor, int column,

2. clase - clause. Else the patch can not be built.

> +               Bitmask is_ready, u32 op, struct space_def *space_def,
> +               struct key_def *key_def)
> 

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

[n.pettik]

> On 18 Jun 2018, at 15:40, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Thanks for the fixes! See 2 comments below.
> 
>>> 2. As I proposed in the review, why can not we use stack struct Table here?
>>> Just declare it at the beginning of the function and here save its pointer.
>> Well, OK it is possible:
>> +++ b/src/box/sql/delete.c
>> @@ -111,9 +111,12 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>>                  * wrapper around space_def to support interface.
>>                  */
>>                 space = space_by_id(space_id);
>> -               tab_list->a[0].pTab = sql_table_construct_from_space(space);
>> -               if (tab_list->a[0].pTab == NULL)
>> -                       goto delete_from_cleanup;
>> +               struct Table tmp_tab;
>> +               memset(&tmp_tab, 0, sizeof(tmp_tab));
>> +               tmp_tab.def = space->def;
>> +               /* Prevent from freeing memory in DeleteTable. */
>> +               tmp_tab.nTabRef = 2;
>> +               tab_list->a[0].pTab = &tmp_tab;
> 
> 1. Are you sure you can save the pointer to the stack value and then
> use it out of the declaration context?
> 
> Here tmp_tab is declared inside {} block, but pointer on it is
> used out the former.

Well,  I'm not so sure about that..

@@ -99,6 +99,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
         */
        bool is_complex = false;
        const char *tab_name = tab_list->a->zName;
+       struct Table tmp_tab;
        if (sqlite3LocateTable(parse, LOCATE_NOERR, tab_name) == NULL) {
                space_id = box_space_id_by_name(tab_name,
                                                strlen(tab_name));
@@ -111,7 +112,6 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
                 * wrapper around space_def to support interface.
                 */
                space = space_by_id(space_id);
-               struct Table tmp_tab;

>>> 5. Why WhereClause new method has prefix 'sql_where_', but WhereLoop method has prefix
>>> 'where_'? Lets make it 'where_clause_find_term' and 'where_loop_assign_terms’.
>> -sql_where_find_term(struct WhereClause *where_clause, int cursor, int column,
>> -                   Bitmask is_ready, u32 op, struct space_def *space_def,
>> -                   struct key_def *key_def)
>> +where_clase_find_term(struct WhereClause *where_clause, int cursor, int column,
> 
> 2. clase - clause. Else the patch can not be built.

Oops, I am sorry:

@@ -506,9 +506,9 @@ sqlite3WhereFindTerm(WhereClause * pWC,     /* The WHERE clause to be searched */
  * @retval New struct describing WHERE term.
  */
 static inline struct WhereTerm *
-where_clase_find_term(struct WhereClause *where_clause, int cursor, int column,
-               Bitmask is_ready, u32 op, struct space_def *space_def,
-               struct key_def *key_def)
+where_clause_find_term(struct WhereClause *where_clause, int cursor, int column,
+                      Bitmask is_ready, u32 op, struct space_def *space_def,
+                      struct key_def *key_def)

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

[Vladislav Shpilevoy]

Now LGTM.

On 18/06/2018 17:00, n.pettik wrote:
> 
>> On 18 Jun 2018, at 15:40, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>>
>> Thanks for the fixes! See 2 comments below.
>>
>>>> 2. As I proposed in the review, why can not we use stack struct Table here?
>>>> Just declare it at the beginning of the function and here save its pointer.
>>> Well, OK it is possible:
>>> +++ b/src/box/sql/delete.c
>>> @@ -111,9 +111,12 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>>>                   * wrapper around space_def to support interface.
>>>                   */
>>>                  space = space_by_id(space_id);
>>> -               tab_list->a[0].pTab = sql_table_construct_from_space(space);
>>> -               if (tab_list->a[0].pTab == NULL)
>>> -                       goto delete_from_cleanup;
>>> +               struct Table tmp_tab;
>>> +               memset(&tmp_tab, 0, sizeof(tmp_tab));
>>> +               tmp_tab.def = space->def;
>>> +               /* Prevent from freeing memory in DeleteTable. */
>>> +               tmp_tab.nTabRef = 2;
>>> +               tab_list->a[0].pTab = &tmp_tab;
>>
>> 1. Are you sure you can save the pointer to the stack value and then
>> use it out of the declaration context?
>>
>> Here tmp_tab is declared inside {} block, but pointer on it is
>> used out the former.
> 
> Well,  I'm not so sure about that..
> 
> @@ -99,6 +99,7 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>           */
>          bool is_complex = false;
>          const char *tab_name = tab_list->a->zName;
> +       struct Table tmp_tab;
>          if (sqlite3LocateTable(parse, LOCATE_NOERR, tab_name) == NULL) {
>                  space_id = box_space_id_by_name(tab_name,
>                                                  strlen(tab_name));
> @@ -111,7 +112,6 @@ sql_table_delete_from(struct Parse *parse, struct SrcList *tab_list,
>                   * wrapper around space_def to support interface.
>                   */
>                  space = space_by_id(space_id);
> -               struct Table tmp_tab;
> 
>>>> 5. Why WhereClause new method has prefix 'sql_where_', but WhereLoop method has prefix
>>>> 'where_'? Lets make it 'where_clause_find_term' and 'where_loop_assign_terms’.
>>> -sql_where_find_term(struct WhereClause *where_clause, int cursor, int column,
>>> -                   Bitmask is_ready, u32 op, struct space_def *space_def,
>>> -                   struct key_def *key_def)
>>> +where_clase_find_term(struct WhereClause *where_clause, int cursor, int column,
>>
>> 2. clase - clause. Else the patch can not be built.
> 
> Oops, I am sorry:
> 
> @@ -506,9 +506,9 @@ sqlite3WhereFindTerm(WhereClause * pWC,     /* The WHERE clause to be searched */
>    * @retval New struct describing WHERE term.
>    */
>   static inline struct WhereTerm *
> -where_clase_find_term(struct WhereClause *where_clause, int cursor, int column,
> -               Bitmask is_ready, u32 op, struct space_def *space_def,
> -               struct key_def *key_def)
> +where_clause_find_term(struct WhereClause *where_clause, int cursor, int column,
> +                      Bitmask is_ready, u32 op, struct space_def *space_def,
> +                      struct key_def *key_def)
> 
> 

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

[Kirill Yukhin]

Hello Nikita, Vlad,
On 18 июн 17:17, Vladislav Shpilevoy wrote:
> Now LGTM.
Patch checked into 2.0 branch.

--
Regards, Kirill Yukhin

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

[Kirill Yukhin]

Hello Vlad,
On 01 июн 18:16, Kirill Yukhin wrote:
> This patches consists of two preparational patches and the
> main patch which enables DELETE's from spaces created w/ Lua.
Thanks for your inputs and even patches. I've squashed your
review changes (w/ minor fixes) into main patch-set and committed
them to 2.0 branch.

--
Regards, Kirill Yukhin