[tarantool-patches] Re: [PATCH v1 1/1] sql: refactor vdbe_emit_open_cursor calls

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

Hello. Thanks for the patch!

On 09/07/2018 15:42, Kirill Shcherbatov wrote:
> Made vdbe_emit_open_cursor calls consistent:
> now it uses index id everywhere.
> This required to change a way to detect that
> VDBE has openned Read cursor to specified table
> in vdbe_has_table_read to write result of insert
> in temp table if required.
> ---

Where are the issue and the branch?

> https://github.com/tarantool/tarantool/compare/kshch/kshch/vdbe_emit_open_cursor-refactor

This is not branch and by this link I see message:

     "There isn’t anything to compare."

> over branch
> https://github.com/tarantool/tarantool/issues/2847

How is this linked? The issue was found under working with this
issue, but it has been actual before and remains actual after.

See 7 comments below.

And I have pushed my own patch for LoadPtr removal on the top
of your branch. Please, fix my comments below, and fix the patch
I have pushed (most likely it does not pass tests now).

> 
>   src/box/sql/analyze.c |  3 +-
>   src/box/sql/build.c   |  4 ++-
>   src/box/sql/expr.c    |  5 ++-
>   src/box/sql/fkey.c    |  3 +-
>   src/box/sql/insert.c  | 95 ++++++++++++++++++++++++++++++---------------------
>   src/box/sql/select.c  |  4 +--
>   src/box/sql/where.c   | 10 ++++--
>   7 files changed, 75 insertions(+), 49 deletions(-)
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 1c00842..82447dc 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -1108,6 +1108,7 @@ vdbe_emit_open_cursor(struct Parse *parse_context, int cursor, int index_id,
>   		      struct space *space)
>   {
>   	assert(space != NULL);
> +	assert(index_id == SQLITE_PAGENO_TO_INDEXID(index_id));

1. What the hell? Why?

>   	struct Vdbe *vdbe = parse_context->pVdbe;
>   	int space_ptr_reg = ++parse_context->nMem;
>   	sqlite3VdbeAddOp4(vdbe, OP_LoadPtr, 0, space_ptr_reg, 0, (void*)space,
>   	addr1 = sqlite3VdbeAddOp2(v, OP_SorterSort, iSorter, 0);

2. vdbe_emit_open_cursor has outdated comment in the header.

> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 58e159c..77567e7 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -179,41 +179,50 @@ sqlite3TableAffinity(Vdbe * v, Table * pTab, int iReg)
>   	}
>   }
>   
> -/*
> - * Return non-zero if the table pTab in database or any of its indices
> - * have been opened at any point in the VDBE program. This is used to see if
> - * a statement of the form  "INSERT INTO <pTab> SELECT ..." can
> - * run for the results of the SELECT.
> +/**
> + * Return true if the table pTab in database or any of its

3. No pTab.

> + * indices have been opened at any point in the VDBE program.
> + * This is used to see if a statement of the form
> + * "INSERT INTO <pTab> SELECT ..." can run for the results of the
> + * SELECT.
> + *
> + * @param parser Parse context.
> + * @param table Table AST object.
> + * @retval true or false

4. Unhelpful comment. Obviously bool is either true or false.

>    */
> -static int
> -readsTable(Parse * p, Table * pTab)
> +static bool
> +vdbe_has_table_read(struct Parse *parser, struct Table *table)

5. You can make table be const struct Table *.

>   {
> -	Vdbe *v = sqlite3GetVdbe(p);
> -	int i;
> -	int iEnd = sqlite3VdbeCurrentAddr(v);
> -
> -	for (i = 1; i < iEnd; i++) {
> -		VdbeOp *pOp = sqlite3VdbeGetOp(v, i);
> -		assert(pOp != 0);
> -		/* Currently, there is no difference between
> -		 * Read and Write cursors.
> +	struct Vdbe *v = sqlite3GetVdbe(parser);
> +	int last_instr = sqlite3VdbeCurrentAddr(v);
> +	for (int i = 1; i < last_instr; i++) {
> +		struct VdbeOp *pOp = sqlite3VdbeGetOp(v, i);

6. When a function is refactored completely, we should use
Tarantool code style for variable names.

> +		assert(pOp != NULL);
> +		/*
> +		 * Currently, there is no difference between Read
> +		 * and Write cursors.
>   		 */
> -		if (pOp->opcode == OP_OpenRead ||
> -		    pOp->opcode == OP_OpenWrite) {
> -			Index *pIndex;
> -			int tnum = pOp->p2;
> -			if (tnum == pTab->tnum) {
> -				return 1;
> -			}
> -			for (pIndex = pTab->pIndex; pIndex;
> -			     pIndex = pIndex->pNext) {
> -				if (tnum == pIndex->tnum) {
> -					return 1;
> -				}
> +		if (pOp->opcode == OP_OpenRead || pOp->opcode == OP_OpenWrite) {
> +			assert(i > 1);
> +			struct VdbeOp *space_var_op =
> +				sqlite3VdbeGetOp(v, i - 1);
> +			assert(space_var_op != NULL);
> +			assert(space_var_op->opcode == OP_LoadPtr);
> +			struct space *space = space_var_op->p4.space;
> +
> +			if (space == space_by_id(table->def->id))

7. Why do you need space_by_id()? Why not just space->id == table->def->id?

> +				return true;
> +
> +			int idx_id = pOp->p2;
> +			for (struct Index *pIndex = table->pIndex;
> +				pIndex != NULL; pIndex = pIndex->pNext) {
> +				if (idx_id ==
> +				    SQLITE_PAGENO_TO_INDEXID(pIndex->tnum))
> +					return true;
>   			}
>   		}
>   	}
> -	return 0;
> +	return false;
>   }
>   
>