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

[n.pettik]

> On 18 Jun 2018, at 14:06, Vladislav Shpilevoy <v.shpilevoy at 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()