[tarantool-patches] [PATCH] sql: remove redundant goto from VDBE prologue

[tarantool-patches] [PATCH] sql: remove redundant goto from VDBE prologue

[Nikita Pettik]

Structure of VDBE prologue:

    0: OP_Init 0 N (address to start) 0 --|
|-> 1: ...                                |
|      ...                                |
|   N: OP_Transaction         <------------
|   N+1: (Constant expressions to be saved in registers)
|      ...
|-- M: OP_Goto 0 1 0

However, last opcode in VDBE program (i.e. OP_Goto) is generated always,
despite the existence of OP_Transaction or constant expressions.
Thus, VDBE program for queries like <SELECT * FROM table;> features
redundant jump. Such useless jump wastes exectuion time (although it is
executed once) and may affect jump prediction.

This patch adds conditional branch for generating jump opcode finishing
VDBE program: it is appended only if we need to start transaction or
code constrant expressions.

Closes #3231
---
Branch: https://github.com/tarantool/tarantool/commits/np/gh-3231-remove-goto-from-vdbe-prologue
Issue: https://github.com/tarantool/tarantool/issues/3231

 src/box/sql/build.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 62d687b17..66bef404c 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -94,9 +94,6 @@ sqlite3FinishCoding(Parse * pParse)
 			sqlite3VdbeJumpHere(v, 0);
 			if (pParse->initiateTTrans)
 				sqlite3VdbeAddOp0(v, OP_TTransaction);
-			if (db->init.busy == 0)
-				sqlite3VdbeChangeP5(v, 1);
-
 			/* Code constant expressions that where factored out of inner loops */
 			if (pParse->pConstExpr) {
 				ExprList *pEL = pParse->pConstExpr;
@@ -107,10 +104,22 @@ sqlite3FinishCoding(Parse * pParse)
 							iConstExprReg);
 				}
 			}
-
-			/* Finally, jump back to the beginning of the executable code. */
-			sqlite3VdbeGoto(v, 1);
 		}
+		/*
+		 * Finally, jump back to the beginning of
+		 * the executable code. In fact, it is required
+		 * only if some additional opcodes are generated.
+		 * Otherwise, it would be useless jump:
+		 *
+		 * 0:        OP_Init 0 vdbe_end ...
+		 * 1: ...
+		 *    ...
+		 * vdbe_end: OP_Goto 0 1 ...
+		 */
+		if (pParse->pConstExpr != NULL || pParse->initiateTTrans)
+			sqlite3VdbeGoto(v, 1);
+		else
+			sqlite3VdbeChangeP2(v, 0, 1);
 	}
 
 	/* Get the VDBE program ready for execution
-- 
2.15.1

[tarantool-patches] Re: [PATCH] sql: remove redundant goto from VDBE prologue

[Vladislav Shpilevoy]

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

On 12/06/2018 21:26, Nikita Pettik wrote:
> Structure of VDBE prologue:
> 
>      0: OP_Init 0 N (address to start) 0 --|
> |-> 1: ...                                |
> |      ...                                |
> |   N: OP_Transaction         <------------
> |   N+1: (Constant expressions to be saved in registers)
> |      ...
> |-- M: OP_Goto 0 1 0
> 
> However, last opcode in VDBE program (i.e. OP_Goto) is generated always,
> despite the existence of OP_Transaction or constant expressions.
> Thus, VDBE program for queries like <SELECT * FROM table;> features
> redundant jump. Such useless jump wastes exectuion time (although it is
> executed once) and may affect jump prediction.
> 
> This patch adds conditional branch for generating jump opcode finishing
> VDBE program: it is appended only if we need to start transaction or
> code constrant expressions.
> 
> Closes #3231
> ---
> Branch: https://github.com/tarantool/tarantool/commits/np/gh-3231-remove-goto-from-vdbe-prologue
> Issue: https://github.com/tarantool/tarantool/issues/3231
> 
>   src/box/sql/build.c | 21 +++++++++++++++------
>   1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 62d687b17..66bef404c 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -94,9 +94,6 @@ sqlite3FinishCoding(Parse * pParse)

1. This function looks small. Lets refactor it to Tarantool
code style. If we don't, I am afraid this commit will be lost
in 'git blame', when this function finally will be refactored
completely.

>   			sqlite3VdbeJumpHere(v, 0);
>   			if (pParse->initiateTTrans)
>   				sqlite3VdbeAddOp0(v, OP_TTransaction);
> -			if (db->init.busy == 0)
> -				sqlite3VdbeChangeP5(v, 1);

2. Why is it removed? I do not see this code moved below.

> -
>   			/* Code constant expressions that where factored out of inner loops */
>   			if (pParse->pConstExpr) {
>   				ExprList *pEL = pParse->pConstExpr;
> @@ -107,10 +104,22 @@ sqlite3FinishCoding(Parse * pParse)
>   							iConstExprReg);
>   				}
>   			}
> -
> -			/* Finally, jump back to the beginning of the executable code. */
> -			sqlite3VdbeGoto(v, 1);
>   		}
> +		/*
> +		 * Finally, jump back to the beginning of
> +		 * the executable code. In fact, it is required
> +		 * only if some additional opcodes are generated.
> +		 * Otherwise, it would be useless jump:
> +		 *
> +		 * 0:        OP_Init 0 vdbe_end ...
> +		 * 1: ...
> +		 *    ...
> +		 * vdbe_end: OP_Goto 0 1 ...
> +		 */
> +		if (pParse->pConstExpr != NULL || pParse->initiateTTrans)
> +			sqlite3VdbeGoto(v, 1);
> +		else
> +			sqlite3VdbeChangeP2(v, 0, 1);

3. As far as I see, P2 in OP_Init is 1 already when we are here. It is
not? See allocVdbe function. P2 == 1 by default, and here it can be changed to
goto to ttrans.

>   	}
>   
>   	/* Get the VDBE program ready for execution
> 

4. Can we test the new VDBE plan using EXPLAIN? I am wondering why all
plan changes are not tested using EXPLAIN.

[tarantool-patches] Re: [PATCH] sql: remove redundant goto from VDBE prologue

[n.pettik]

> 1. This function looks small. Lets refactor it to Tarantool
> code style. If we don't, I am afraid this commit will be lost
> in 'git blame', when this function finally will be refactored
> completely.
> 

Ok. See diff at the end of letter.

>>  			sqlite3VdbeJumpHere(v, 0);
>>  			if (pParse->initiateTTrans)
>>  				sqlite3VdbeAddOp0(v, OP_TTransaction);
>> -			if (db->init.busy == 0)
>> -				sqlite3VdbeChangeP5(v, 1);
> 
> 2. Why is it removed? I do not see this code moved below.

Actually, this code looks strange. If OP_TTransaction is added,
then setting fifth param for it makes no sense (this opcode takes
no args). If this change is related to OP_Halt, then it is also useless,
since the last OP_Halt must be with signature 0 0 0 (comment from vdbe.c):

* There is an implied "Halt 0 0 0" instruction inserted at the very end of
* every program.  So a jump past the last instruction of the program
* is the same as executing Halt.

That is why I removed this code.

> 
>> -
>>  			/* Code constant expressions that where factored out of inner loops */
>>  			if (pParse->pConstExpr) {
>>  				ExprList *pEL = pParse->pConstExpr;
>> @@ -107,10 +104,22 @@ sqlite3FinishCoding(Parse * pParse)
>>  							iConstExprReg);
>>  				}
>>  			}
>> -
>> -			/* Finally, jump back to the beginning of the executable code. */
>> -			sqlite3VdbeGoto(v, 1);
>>  		}
>> +		/*
>> +		 * Finally, jump back to the beginning of
>> +		 * the executable code. In fact, it is required
>> +		 * only if some additional opcodes are generated.
>> +		 * Otherwise, it would be useless jump:
>> +		 *
>> +		 * 0:        OP_Init 0 vdbe_end ...
>> +		 * 1: ...
>> +		 *    ...
>> +		 * vdbe_end: OP_Goto 0 1 ...
>> +		 */
>> +		if (pParse->pConstExpr != NULL || pParse->initiateTTrans)
>> +			sqlite3VdbeGoto(v, 1);
>> +		else
>> +			sqlite3VdbeChangeP2(v, 0, 1);
> 
> 3. As far as I see, P2 in OP_Init is 1 already when we are here. It is
> not? See allocVdbe function. P2 == 1 by default, and here it can be changed to
> goto to ttrans.

In fact, it is changed by sqlite3VdbeJumpHere(v, 0); 
Thus, we have to again set its value to 1, in case of omitting jump.

> 
>>  	}
>>    	/* Get the VDBE program ready for execution
> 
> 4. Can we test the new VDBE plan using EXPLAIN? I am wondering why all
> plan changes are not tested using EXPLAIN.

Well, it it is quite complicated to test EXPLAIN command since any change to query
planner or code generator at all would result in rewriting such tests. Hence, once we
decided to avoid using tests with EXPLAIN until we get stable code generation.

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

Subject: [PATCH] sql: remove redundant goto from VDBE prologue

Structure of VDBE prologue:

    0: OP_Init 0 N (address to start) 0 --|
|-> 1: ...                                |
|      ...                                |
|   N: OP_Transaction         <------------
|   N+1: (Constant expressions to be saved in registers)
|      ...
|-- M: OP_Goto 0 1 0

However, last opcode in VDBE program (i.e. OP_Goto) is generated always,
despite the existence of OP_Transaction or constant expressions.
Thus, VDBE program for queries like <SELECT * FROM table;> features
redundant jump. Such useless jump wastes exectuion time (although it is
executed once) and may affect jump prediction.

This patch adds conditional branch for generating jump opcode finishing
VDBE program: it is appended only if we need to start transaction or
code constrant expressions.

Closes #3231
---
 src/box/sql/build.c     | 105 +++++++++++++++++++++++-------------------------
 src/box/sql/parse.y     |   2 +-
 src/box/sql/sqliteInt.h |  17 +++++++-
 3 files changed, 68 insertions(+), 56 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 62d687b17..1c972c977 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -54,76 +54,73 @@
 #include "box/tuple_format.h"
 #include "box/coll_id_cache.h"
 
-/*
- * This routine is called after a single SQL statement has been
- * parsed and a VDBE program to execute that statement has been
- * prepared.  This routine puts the finishing touches on the
- * VDBE program and resets the pParse structure for the next
- * parse.
- *
- * Note that if an error occurred, it might be the case that
- * no VDBE code was generated.
- */
 void
-sqlite3FinishCoding(Parse * pParse)
+sql_finish_coding(struct Parse *parse_context)
 {
-	sqlite3 *db;
-	Vdbe *v;
-
-	assert(pParse->pToplevel == 0);
-	db = pParse->db;
-	if (pParse->nested)
+	if (parse_context->nested)
 		return;
-	if (db->mallocFailed || pParse->nErr) {
-		if (pParse->rc == SQLITE_OK)
-			pParse->rc = SQLITE_ERROR;
+	assert(parse_context->pToplevel == NULL);
+	struct sqlite3 *db = parse_context->db;
+	if (db->mallocFailed || parse_context->nErr != 0) {
+		if (parse_context->rc == SQLITE_OK)
+			parse_context->rc = SQLITE_ERROR;
 		return;
 	}
-
-	/* Begin by generating some termination code at the end of the
-	 * vdbe program
+	/*
+	 * Begin by generating some termination code at the end
+	 * of the vdbe program
 	 */
-	v = sqlite3GetVdbe(pParse);
-	assert(!pParse->isMultiWrite
-	       || sqlite3VdbeAssertMayAbort(v, pParse->mayAbort));
-	if (v) {
+	struct Vdbe *v = sqlite3GetVdbe(parse_context);
+	assert(!parse_context->isMultiWrite ||
+	       sqlite3VdbeAssertMayAbort(v, parse_context->mayAbort));
+	if (v != NULL) {
 		sqlite3VdbeAddOp0(v, OP_Halt);
-		if (db->mallocFailed == 0 || pParse->pConstExpr) {
-			int i;
+		if (db->mallocFailed == 0 ||
+		    parse_context->pConstExpr != NULL) {
 			assert(sqlite3VdbeGetOp(v, 0)->opcode == OP_Init);
 			sqlite3VdbeJumpHere(v, 0);
-			if (pParse->initiateTTrans)
+			if (parse_context->initiateTTrans)
 				sqlite3VdbeAddOp0(v, OP_TTransaction);
-			if (db->init.busy == 0)
-				sqlite3VdbeChangeP5(v, 1);
-
-			/* Code constant expressions that where factored out of inner loops */
-			if (pParse->pConstExpr) {
-				ExprList *pEL = pParse->pConstExpr;
-				pParse->okConstFactor = 0;
-				for (i = 0; i < pEL->nExpr; i++) {
-					sqlite3ExprCode(pParse, pEL->a[i].pExpr,
-							pEL->a[i].u.
+			/*
+			 * Code constant expressions that where
+			 * factored out of inner loops.
+			 */
+			if (parse_context->pConstExpr != NULL) {
+				struct ExprList *exprs =
+					parse_context->pConstExpr;
+				parse_context->okConstFactor = 0;
+				for (int i = 0; i < exprs->nExpr; ++i) {
+					sqlite3ExprCode(parse_context,
+							exprs->a[i].pExpr,
+							exprs->a[i].u.
 							iConstExprReg);
 				}
 			}
-
-			/* Finally, jump back to the beginning of the executable code. */
-			sqlite3VdbeGoto(v, 1);
 		}
-	}
-
-	/* Get the VDBE program ready for execution
-	 */
-	if (v && pParse->nErr == 0 && !db->mallocFailed) {
-		assert(pParse->iCacheLevel == 0);	/* Disables and re-enables match */
-		/* A minimum of one cursor is required if autoincrement is used
-		 *  See ticket [a696379c1f08866]
+		/*
+		 * Finally, jump back to the beginning of
+		 * the executable code. In fact, it is required
+		 * only if some additional opcodes are generated.
+		 * Otherwise, it would be useless jump:
+		 *
+		 * 0:        OP_Init 0 vdbe_end ...
+		 * 1: ...
+		 *    ...
+		 * vdbe_end: OP_Goto 0 1 ...
 		 */
-		sqlite3VdbeMakeReady(v, pParse);
-		pParse->rc = SQLITE_DONE;
+		if (parse_context->pConstExpr != NULL ||
+		    parse_context->initiateTTrans)
+			sqlite3VdbeGoto(v, 1);
+		else
+			sqlite3VdbeChangeP2(v, 0, 1);
+	}
+	/* Get the VDBE program ready for execution. */
+	if (v != NULL && parse_context->nErr == 0 && !db->mallocFailed) {
+		assert(parse_context->iCacheLevel == 0);
+		sqlite3VdbeMakeReady(v, parse_context);
+		parse_context->rc = SQLITE_DONE;
 	} else {
-		pParse->rc = SQLITE_ERROR;
+		parse_context->rc = SQLITE_ERROR;
 	}
 }
 
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 2e5f349c8..58f54e05f 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -110,7 +110,7 @@ static void disableLookaside(Parse *pParse){
 input ::= ecmd.
 ecmd ::= explain cmdx SEMI. {
 	if (!pParse->parse_only)
-		sqlite3FinishCoding(pParse);
+		sql_finish_coding(pParse);
 }
 ecmd ::= SEMI. {
   sqlite3ErrorMsg(pParse, "syntax error: empty request");
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 01351a183..4d2bcafbc 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -3442,7 +3442,22 @@ void sqlite3NormalizeName(char *z);
 void sqlite3TokenInit(Token *, char *);
 int sqlite3KeywordCode(const unsigned char *, int);
 int sqlite3RunParser(Parse *, const char *, char **);
-void sqlite3FinishCoding(Parse *);
+
+/**
+ * This routine is called after a single SQL statement has been
+ * parsed and a VDBE program to execute that statement has been
+ * prepared.  This routine puts the finishing touches on the
+ * VDBE program and resets the pParse structure for the next
+ * parse.
+ *
+ * Note that if an error occurred, it might be the case that
+ * no VDBE code was generated.
+ *
+ * @param parse_context Current parsing context.
+ */
+void
+sql_finish_coding(struct Parse *parse_context);
+
 int sqlite3GetTempReg(Parse *);
 void sqlite3ReleaseTempReg(Parse *, int);
 int sqlite3GetTempRange(Parse *, int);
-- 
2.15.1

[tarantool-patches] Re: [PATCH] sql: remove redundant goto from VDBE prologue

[Vladislav Shpilevoy]

>>
>> 3. As far as I see, P2 in OP_Init is 1 already when we are here. It is
>> not? See allocVdbe function. P2 == 1 by default, and here it can be changed to
>> goto to ttrans.
> 
> In fact, it is changed by sqlite3VdbeJumpHere(v, 0);
> Thus, we have to again set its value to 1, in case of omitting jump.

It is done in the same function few lines above. How about
to do not do this jump + not jump? I have slightly refactored the
code to do not this jump. Please, see the separate commit on
the branch.

(I did not check the tests).

> 
>>
>>>   	}
>>>     	/* Get the VDBE program ready for execution
>>
>> 4. Can we test the new VDBE plan using EXPLAIN? I am wondering why all
>> plan changes are not tested using EXPLAIN.
> 
> Well, it it is quite complicated to test EXPLAIN command since any change to query
> planner or code generator at all would result in rewriting such tests. Hence, once we
> decided to avoid using tests with EXPLAIN until we get stable code generation.

I think, that we should start test plans. At least, here it is possible to call
EXPLAIN, test first result line only and ignore others. Then this test will not
fail on any plan change.

[tarantool-patches] Re: [PATCH] sql: remove redundant goto from VDBE prologue

[n.pettik]

> On 18 Jun 2018, at 14:06, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> 
>>> 
>>> 3. As far as I see, P2 in OP_Init is 1 already when we are here. It is
>>> not? See allocVdbe function. P2 == 1 by default, and here it can be changed to
>>> goto to ttrans.
>> In fact, it is changed by sqlite3VdbeJumpHere(v, 0);
>> Thus, we have to again set its value to 1, in case of omitting jump.
> 
> It is done in the same function few lines above. How about
> to do not do this jump + not jump? I have slightly refactored the
> code to do not this jump. Please, see the separate commit on
> the branch.
> 
> (I did not check the tests).

Thx for refactoring, but it wouldn’t work this way:
We must firstly make lable to jump (sqlite3VdbeJumpHere(v, 0);),
then test on emitting OP_TTransaction and const exprs (or both). If we need to
add such opcodes, then we should jump back. Otherwise, dismiss initial jump.

+++ b/src/box/sql/build.c
@@ -74,11 +74,11 @@ sql_finish_coding(struct Parse *parse_context)
         */
        assert(!parse_context->isMultiWrite ||
               sqlite3VdbeAssertMayAbort(v, parse_context->mayAbort));
+       sqlite3VdbeJumpHere(v, 0);
+       if (parse_context->initiateTTrans)
+               sqlite3VdbeAddOp0(v, OP_TTransaction);
        if (parse_context->pConstExpr != NULL) {
                assert(sqlite3VdbeGetOp(v, 0)->opcode == OP_Init);
-               sqlite3VdbeJumpHere(v, 0);
-               if (parse_context->initiateTTrans)
-                       sqlite3VdbeAddOp0(v, OP_TTransaction);
                /*
                 * Code constant expressions that where
                 * factored out of inner loops.
@@ -89,21 +89,22 @@ sql_finish_coding(struct Parse *parse_context)
                        sqlite3ExprCode(parse_context, exprs->a[i].pExpr,
                                        exprs->a[i].u. iConstExprReg);
                }
-               /*
-                * Finally, jump back to the beginning of
-                * the executable code. In fact, it is required
-                * only if some additional opcodes are generated.
-                * Otherwise, it would be useless jump:
-                *
-                * 0:        OP_Init 0 vdbe_end ...
-                * 1: ...
-                *    ...
-                * vdbe_end: OP_Goto 0 1 ...
-                */
-               sqlite3VdbeGoto(v, 1);
-       } else if (parse_context->initiateTTrans) {
-               sqlite3VdbeGoto(v, 1);
        }
+       /*
+        * Finally, jump back to the beginning of
+        * the executable code. In fact, it is required
+        * only if some additional opcodes are generated.
+        * Otherwise, it would be useless jump:
+        *
+        * 0:        OP_Init 0 vdbe_end ...
+        * 1: ...
+        *    ...
+        * vdbe_end: OP_Goto 0 1 ...
+        */
+       if (parse_context->initiateTTrans || parse_context->pConstExpr != NULL)
+               sqlite3VdbeGoto(v, 1);
+       else
+               sqlite3VdbeChangeP2(v, 0, 1);

> 
>>> 
>>>>  	}
>>>>    	/* Get the VDBE program ready for execution
>>> 
>>> 4. Can we test the new VDBE plan using EXPLAIN? I am wondering why all
>>> plan changes are not tested using EXPLAIN.
>> Well, it it is quite complicated to test EXPLAIN command since any change to query
>> planner or code generator at all would result in rewriting such tests. Hence, once we
>> decided to avoid using tests with EXPLAIN until we get stable code generation.
> 
> I think, that we should start test plans. At least, here it is possible to call
> EXPLAIN, test first result line only and ignore others. Then this test will not
> fail on any plan change.

Ok, I added a couple of simple tests:

+++ b/test/sql-tap/explain.test.lua
@@ -0,0 +1,43 @@
+#!/usr/bin/env tarantool
+test = require("sqltester")
+test:plan(3)
+
+-- gh-3231: make sure that there is no redundant OP_Goto at the
+-- start of VDBE program. In other words OP_Init jumps exactly to
+-- the next opcode (i.e. opcode with address 1).
+--
+test:do_execsql_test(
+    "explain-1.0",
+    [[
+        CREATE TABLE t1(id INTEGER PRIMARY KEY, a INT);
+        INSERT INTO t1 VALUES(1, 2), (3, 4), (5, 6);
+        SELECT * FROM t1;
+    ]], {
+        -- <explain-1.0>
+        1, 2, 3, 4, 5, 6
+        -- </explain-1.0>
+    })
+
+test:do_test(
+    "explain-1.1",
+    function()
+        opcodes = test:execsql("EXPLAIN SELECT * FROM t1;")
+        return opcodes[1]
+    end,
+        -- <explain-1.1>
+        0, 'Init', 0, 1, 0, '', '00', 'Start at 1'
+        -- </explain-1.1>
+    )
+
+test:do_test(
+    "explain-1.2",
+    function()
+        opcodes = test:execsql("EXPLAIN SELECT a + 1 FROM t1 WHERE id = 4 OR id = 5;")
+        return opcodes[1]
+    end,
+        -- <explain-1.2>
+        0, 'Init', 0, 1, 0, '', '00', 'Start at 1'
+        -- </explain-1.2>
+    )
+
+test:finish_test()

[tarantool-patches] Re: [PATCH] sql: remove redundant goto from VDBE prologue

[Vladislav Shpilevoy]

Thanks for the fixes!

On 18/06/2018 20:45, n.pettik wrote:
> 
>> On 18 Jun 2018, at 14:06, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>>
>>
>>>>
>>>> 3. As far as I see, P2 in OP_Init is 1 already when we are here. It is
>>>> not? See allocVdbe function. P2 == 1 by default, and here it can be changed to
>>>> goto to ttrans.
>>> In fact, it is changed by sqlite3VdbeJumpHere(v, 0);
>>> Thus, we have to again set its value to 1, in case of omitting jump.
>>
>> It is done in the same function few lines above. How about
>> to do not do this jump + not jump? I have slightly refactored the
>> code to do not this jump. Please, see the separate commit on
>> the branch.
>>
>> (I did not check the tests).
> 
> Thx for refactoring, but it wouldn’t work this way:
> We must firstly make lable to jump (sqlite3VdbeJumpHere(v, 0);),
> then test on emitting OP_TTransaction and const exprs (or both). If we need to
> add such opcodes, then we should jump back. Otherwise, dismiss initial jump.

We do not need to change P2 until it is really needed. That is what I
mean.

Please, consider this diff:

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index a7b9d20b3..3a361cfad 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -74,7 +74,7 @@ sql_finish_coding(struct Parse *parse_context)
  	 */
  	assert(!parse_context->isMultiWrite ||
  	       sqlite3VdbeAssertMayAbort(v, parse_context->mayAbort));
-	sqlite3VdbeJumpHere(v, 0);
+	int last_instruction = v->nOp;
  	if (parse_context->initiateTTrans)
  		sqlite3VdbeAddOp0(v, OP_TTransaction);
  	if (parse_context->pConstExpr != NULL) {
@@ -101,10 +101,10 @@ sql_finish_coding(struct Parse *parse_context)
  	 *    ...
  	 * vdbe_end: OP_Goto 0 1 ...
  	 */
-	if (parse_context->initiateTTrans || parse_context->pConstExpr != NULL)
+	if (parse_context->initiateTTrans || parse_context->pConstExpr != NULL) {
+		sqlite3VdbeChangeP2(v, 0, last_instruction);
  		sqlite3VdbeGoto(v, 1);
-	else
-		sqlite3VdbeChangeP2(v, 0, 1);
+	}
  	/* Get the VDBE program ready for execution. */
  	if (parse_context->nErr == 0 && !db->mallocFailed) {
  		assert(parse_context->iCacheLevel == 0);

The tests pass ok.

[tarantool-patches] Re: [PATCH] sql: remove redundant goto from VDBE prologue

[n.pettik]

>> Thx for refactoring, but it wouldn’t work this way:
>> We must firstly make lable to jump (sqlite3VdbeJumpHere(v, 0);),
>> then test on emitting OP_TTransaction and const exprs (or both). If we need to
>> add such opcodes, then we should jump back. Otherwise, dismiss initial jump.
> 
> We do not need to change P2 until it is really needed. That is what I
> mean.

Ok, my apologies for misunderstanding. I pushed your changes.

[tarantool-patches] Re: [PATCH] sql: remove redundant goto from VDBE prologue

[Vladislav Shpilevoy]

Thanks. Now LGTM.

On 18/06/2018 23:55, n.pettik wrote:
> 
>>> Thx for refactoring, but it wouldn’t work this way:
>>> We must firstly make lable to jump (sqlite3VdbeJumpHere(v, 0);),
>>> then test on emitting OP_TTransaction and const exprs (or both). If we need to
>>> add such opcodes, then we should jump back. Otherwise, dismiss initial jump.
>>
>> We do not need to change P2 until it is really needed. That is what I
>> mean.
> 
> Ok, my apologies for misunderstanding. I pushed your changes.
> 

[tarantool-patches] Re: [PATCH] sql: remove redundant goto from VDBE prologue

[Kirill Yukhin]

Hello Nikita, Vlad,
On 19 июн 00:03, Vladislav Shpilevoy wrote:
> Thanks. Now LGTM.
I've checked the patch into 2.0 branch.

--
Regards, Kirill Yukhin