[tarantool-patches] [PATCH 1/1] sql: fix out of time auto commit mode detection

[tarantool-patches] [PATCH 1/1] sql: fix out of time auto commit mode detection

[Vladislav Shpilevoy]

Autocommit is a mode making a Vdbe programm either do not call
box_txn_begin/commit and use Tarantool autocommit mode, or do
box_txn_begin + commit hidden from a user.

But before the patch it is detected during parsing that is very
bad idea.
1. Vdbe conceptually must not depend on any parser time detected
constants like autocommit. Vdbe can be executed multiple times,
and in an environment different from the parsing time one. For
example, parser can be run out of transaction, and Vdbe is
executed inside a one.

2. When autocommit is detected to be false, parser creates
things on a region hoping they will not be turned into garbage
before Vdbe is executed. It is wrong too - region can be
truncated between parsing end and Vdbe start (in future). Or Vdbe
can be executed in another fiber.

Lets detect autocommit mode in runtime, not in compile time. It
allows to use region for temporary allocations during parsing,
and makes us closer to reusable Vdbe objects.

The detection is done once on OP_Init in the top-frame program.
Child Vdbe programs (triggers, for example) must not touch any
transaction things. Only top parent frame can make such
decisions.

Thanks to @kshcherbatov for finding the bug out.
---
Branch: https://github.com/tarantool/tarantool/tree/fix-sql-auto-commit

 src/box/sql/select.c  |  4 ----
 src/box/sql/vdbe.c    | 13 +++++++++++++
 src/box/sql/vdbeaux.c |  2 +-
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 29075d5c2..d26d74625 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -1937,10 +1937,6 @@ allocVdbe(Parse * pParse)
 	    ) {
 		pParse->okConstFactor = 1;
 	}
-	if (sql_vdbe_prepare(v) != 0) {
-		sqlite3DbFree(pParse->db, v);
-		return NULL;
-	}
 	return v;
 }
 
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 3940550d6..5a4024efc 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -5351,6 +5351,19 @@ case OP_Init: {          /* jump */
 	 */
 	assert(pOp->p4.z==0 || strncmp(pOp->p4.z, "-" "- ", 3)==0);
 	assert(pOp==p->aOp);  /* Always instruction 0 */
+	/*
+	 * Once per execution time prepare the programm: detect
+	 * autocommit, create SQL specific transaction things. To
+	 * guarantee the single call of this function the
+	 * preparation is done in the parent frame only. Child
+	 * programs like triggers must use the information
+	 * received from the parent.
+	 */
+	if (p->pFrame == NULL && sql_vdbe_prepare(p) != 0) {
+		sqlite3DbFree(db, p);
+		rc = SQL_TARANTOOL_ERROR;
+		break;
+	}
 
 #ifndef SQLITE_OMIT_TRACE
 	if ((db->mTrace & SQLITE_TRACE_STMT)!=0
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 734433640..4a12d69ae 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -55,7 +55,7 @@ sqlite3VdbeCreate(Parse * pParse)
 	p = sqlite3DbMallocRawNN(db, sizeof(Vdbe));
 	if (p == 0)
 		return 0;
-	memset(&p->aOp, 0, sizeof(Vdbe) - offsetof(Vdbe, aOp));
+	memset(p, 0, sizeof(Vdbe));
 	p->db = db;
 	if (db->pVdbe) {
 		db->pVdbe->pPrev = p;
-- 
2.15.1 (Apple Git-101)

[tarantool-patches] Re: [tarantool-patches] [PATCH 1/1] sql: fix out of time auto commit mode detection

[Nikita Pettik]

[-- Attachment #1: Type: text/plain, Size: 108 bytes --]


>+	 * Once per execution time prepare the programm: detect
Nitpicking: 'program'.

LGTM.
-- 
Nikita Pettik

[-- Attachment #2: Type: text/html, Size: 407 bytes --]

[tarantool-patches] Re: [PATCH 1/1] sql: fix out of time auto commit mode detection

[Vladislav Shpilevoy]

Hello. Thanks for review!

On 22/05/2018 00:45, Nikita Pettik wrote:
>     + * Once per execution time prepare the programm: detect
> 
> Nitpicking: 'program'.

@@ -5352,7 +5352,7 @@ case OP_Init: {          /* jump */
         assert(pOp->p4.z==0 || strncmp(pOp->p4.z, "-" "- ", 3)==0);
         assert(pOp==p->aOp);  /* Always instruction 0 */
         /*
-        * Once per execution time prepare the programm: detect
+        * Once per execution time prepare the program: detect
          * autocommit, create SQL specific transaction things. To


> 
> LGTM.
> 
> -- 
> Nikita Pettik
> 

[tarantool-patches] Re: [PATCH 1/1] sql: fix out of time auto commit mode detection

[Kirill Yukhin]

Hello Vlad,
On 22 мая 00:46, Vladislav Shpilevoy wrote:
> Hello. Thanks for review!
> 
> On 22/05/2018 00:45, Nikita Pettik wrote:
> >     + * Once per execution time prepare the programm: detect
> > 
> > Nitpicking: 'program'.
> 
> @@ -5352,7 +5352,7 @@ case OP_Init: {          /* jump */
>         assert(pOp->p4.z==0 || strncmp(pOp->p4.z, "-" "- ", 3)==0);
>         assert(pOp==p->aOp);  /* Always instruction 0 */
>         /*
> -        * Once per execution time prepare the programm: detect
> +        * Once per execution time prepare the program: detect
>          * autocommit, create SQL specific transaction things. To
> > 
> > LGTM.

I've checked you patch into 2.0 branch.
In future, could you pls name your branches according to SOP?
Please, see chapter `Submitting a patch` for details.

> > -- 
> > Nikita Pettik

--
Regards, Kirill Yukhin