[Tarantool-patches] [PATCH] sql: remove expmask from prepared statement

[Tarantool-patches] [PATCH] sql: remove expmask from prepared statement

[Nikita Pettik]

expmask indicated necessity to recompile statement after parameter was
bound: it might turn out that parameter can affect query plan. However,
part of this mechanism has been removed long ago as a SQLite's legacy.
In its current state expmask is likely to be useless and assertions
involving it are obviously unsuitable. This patch completely removes
expmask and related routines.

Closes #4566
---
Branch: https://github.com/tarantool/tarantool/tree/np/gh-4566-remove-exp-mask
Issue: https://github.com/tarantool/tarantool/issues/4566

 src/box/sql/vdbe.h      |  1 -
 src/box/sql/vdbeInt.h   |  1 -
 src/box/sql/vdbeapi.c   | 19 -------------------
 src/box/sql/vdbeaux.c   | 16 ----------------
 src/box/sql/vdbemem.c   |  1 -
 src/box/sql/whereexpr.c |  2 --
 test/sql/bind.result    | 32 ++++++++++++++++++++++++++++++++
 test/sql/bind.test.lua  |  9 +++++++++
 8 files changed, 41 insertions(+), 40 deletions(-)

diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index 29ff99867..582d48a1f 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -255,7 +255,6 @@ void sqlVdbeSetSql(Vdbe *, const char *z, int n, int);
 void sqlVdbeSwap(Vdbe *, Vdbe *);
 VdbeOp *sqlVdbeTakeOpArray(Vdbe *, int *, int *);
 sql_value *sqlVdbeGetBoundValue(Vdbe *, int, u8);
-void sqlVdbeSetVarmask(Vdbe *, int);
 char *sqlVdbeExpandSql(Vdbe *, const char *);
 int sqlMemCompare(const Mem *, const Mem *, const struct coll *);
 
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index 104a05613..0f32b4cd6 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -428,7 +428,6 @@ struct Vdbe {
 	VdbeFrame *pFrame;	/* Parent frame */
 	VdbeFrame *pDelFrame;	/* List of frame objects to free on VM reset */
 	int nFrame;		/* Number of frames in pFrame list */
-	u32 expmask;		/* Binding to these vars invalidates VM */
 	SubProgram *pProgram;	/* Linked list of all sub-programs used by VM */
 	/** Parser flags with which this object was built. */
 	uint32_t sql_flags;
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index bf40b44fa..b91d16a9d 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -112,9 +112,6 @@ sql_clear_bindings(sql_stmt * pStmt)
 		sqlVdbeMemRelease(&p->aVar[i]);
 		p->aVar[i].flags = MEM_Null;
 	}
-	if (p->isPrepareV2 && p->expmask) {
-		p->expired = 1;
-	}
 	return rc;
 }
 
@@ -826,22 +823,6 @@ vdbeUnbind(Vdbe * p, int i)
 	sqlVdbeMemRelease(pVar);
 	pVar->flags = MEM_Null;
 	pVar->field_type = field_type_MAX;
-
-	/* If the bit corresponding to this variable in Vdbe.expmask is set, then
-	 * binding a new value to this variable invalidates the current query plan.
-	 *
-	 * IMPLEMENTATION-OF: R-48440-37595 If the specific value bound to host
-	 * parameter in the WHERE clause might influence the choice of query plan
-	 * for a statement, then the statement will be automatically recompiled,
-	 * as if there had been a schema change, on the first sql_step() call
-	 * following any change to the bindings of that parameter.
-	 */
-	if (p->isPrepareV2 &&
-	    ((i < 32 && p->expmask & ((u32) 1 << i))
-	     || p->expmask == 0xffffffff)
-	    ) {
-		p->expired = 1;
-	}
 	return 0;
 }
 
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 40918e7e7..a1d658648 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2988,22 +2988,6 @@ sqlVdbeGetBoundValue(Vdbe * v, int iVar, u8 aff)
 	return 0;
 }
 
-/*
- * Configure SQL variable iVar so that binding a new value to it signals
- * to sql_reoptimize() that re-preparing the statement may result
- * in a better query plan.
- */
-void
-sqlVdbeSetVarmask(Vdbe * v, int iVar)
-{
-	assert(iVar > 0);
-	if (iVar > 32) {
-		v->expmask = 0xffffffff;
-	} else {
-		v->expmask |= ((u32) 1 << (iVar - 1));
-	}
-}
-
 int
 sqlVdbeCompareMsgpack(const char **key1,
 			  struct UnpackedRecord *unpacked, int key2_idx)
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 15276c224..dcfc59820 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -1596,7 +1596,6 @@ stat4ValueFromExpr(Parse * pParse,	/* Parse context */
 	    ) {
 		Vdbe *v;
 		int iBindVar = pExpr->iColumn;
-		sqlVdbeSetVarmask(pParse->pVdbe, iBindVar);
 		if ((v = pParse->pReprepare) != 0) {
 			pVal = valueNew(db, pAlloc);
 			if (pVal) {
diff --git a/src/box/sql/whereexpr.c b/src/box/sql/whereexpr.c
index 98615b118..d9b5c78f5 100644
--- a/src/box/sql/whereexpr.c
+++ b/src/box/sql/whereexpr.c
@@ -314,7 +314,6 @@ like_optimization_is_valid(Parse *pParse, Expr *pExpr, Expr **ppPrefix,
 		if (pVal && sql_value_type(pVal) == MP_STR) {
 			z = (char *)sql_value_text(pVal);
 		}
-		sqlVdbeSetVarmask(pParse->pVdbe, iCol);
 		assert(pRight->op == TK_VARIABLE || pRight->op == TK_REGISTER);
 	} else if (op == TK_STRING) {
 		z = pRight->u.zToken;
@@ -336,7 +335,6 @@ like_optimization_is_valid(Parse *pParse, Expr *pExpr, Expr **ppPrefix,
 			*ppPrefix = pPrefix;
 			if (op == TK_VARIABLE) {
 				Vdbe *v = pParse->pVdbe;
-				sqlVdbeSetVarmask(v, pRight->iColumn);
 				if (*pisComplete && pRight->u.zToken[1]) {
 					/* If the rhs of the LIKE expression is a variable, and the current
 					 * value of the variable means there is no need to invoke the LIKE
diff --git a/test/sql/bind.result b/test/sql/bind.result
index 86126e86f..82a058c90 100644
--- a/test/sql/bind.result
+++ b/test/sql/bind.result
@@ -337,3 +337,35 @@ box.execute('SELECT $2', {1, 2, 3})
 - null
 - 'Failed to execute SQL statement: The number of parameters is too large'
 ...
+-- gh-4566: bind variable to LIKE argument resulted to crash.
+--
+box.execute("CREATE TABLE t (id INT PRIMARY KEY, a TEXT);")
+---
+- row_count: 1
+...
+box.execute("SELECT * FROM t WHERE a LIKE ?;", {'a%'});
+---
+- metadata:
+  - name: ID
+    type: integer
+  - name: A
+    type: string
+  rows: []
+...
+box.execute("INSERT INTO t VALUES (1, 'aA'), (2, 'Ba'), (3, 'A');")
+---
+- row_count: 3
+...
+box.execute("SELECT * FROM t WHERE a LIKE ?;", {'a%'});
+---
+- metadata:
+  - name: ID
+    type: integer
+  - name: A
+    type: string
+  rows:
+  - [1, 'aA']
+...
+box.space.T:drop()
+---
+...
diff --git a/test/sql/bind.test.lua b/test/sql/bind.test.lua
index 29d71edcd..2ced7775a 100644
--- a/test/sql/bind.test.lua
+++ b/test/sql/bind.test.lua
@@ -124,3 +124,12 @@ box.execute('DROP TABLE test')
 
 box.execute('SELECT ?', {1, 2})
 box.execute('SELECT $2', {1, 2, 3})
+
+-- gh-4566: bind variable to LIKE argument resulted to crash.
+--
+box.execute("CREATE TABLE t (id INT PRIMARY KEY, a TEXT);")
+box.execute("SELECT * FROM t WHERE a LIKE ?;", {'a%'});
+box.execute("INSERT INTO t VALUES (1, 'aA'), (2, 'Ba'), (3, 'A');")
+box.execute("SELECT * FROM t WHERE a LIKE ?;", {'a%'});
+
+box.space.T:drop()
-- 
2.15.1

Re: [Tarantool-patches] [tarantool-patches] [PATCH] sql: remove expmask from prepared statement

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

LGTM.

On 18/10/2019 15:47, Nikita Pettik wrote:
> expmask indicated necessity to recompile statement after parameter was
> bound: it might turn out that parameter can affect query plan. However,
> part of this mechanism has been removed long ago as a SQLite's legacy.
> In its current state expmask is likely to be useless and assertions
> involving it are obviously unsuitable. This patch completely removes
> expmask and related routines.
> 
> Closes #4566
> ---
> Branch: https://github.com/tarantool/tarantool/tree/np/gh-4566-remove-exp-mask
> Issue: https://github.com/tarantool/tarantool/issues/4566
> 
>  src/box/sql/vdbe.h      |  1 -
>  src/box/sql/vdbeInt.h   |  1 -
>  src/box/sql/vdbeapi.c   | 19 -------------------
>  src/box/sql/vdbeaux.c   | 16 ----------------
>  src/box/sql/vdbemem.c   |  1 -
>  src/box/sql/whereexpr.c |  2 --
>  test/sql/bind.result    | 32 ++++++++++++++++++++++++++++++++
>  test/sql/bind.test.lua  |  9 +++++++++
>  8 files changed, 41 insertions(+), 40 deletions(-)
> 

Re: [Tarantool-patches] [tarantool-patches] [PATCH] sql: remove expmask from prepared statement

[Nikita Pettik]

On 19 Oct 15:33, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> LGTM.

Pushed to master as trivial.
 
> On 18/10/2019 15:47, Nikita Pettik wrote:
> > expmask indicated necessity to recompile statement after parameter was
> > bound: it might turn out that parameter can affect query plan. However,
> > part of this mechanism has been removed long ago as a SQLite's legacy.
> > In its current state expmask is likely to be useless and assertions
> > involving it are obviously unsuitable. This patch completely removes
> > expmask and related routines.
> > 
> > Closes #4566
> > ---
> > Branch: https://github.com/tarantool/tarantool/tree/np/gh-4566-remove-exp-mask
> > Issue: https://github.com/tarantool/tarantool/issues/4566
> > 
> >  src/box/sql/vdbe.h      |  1 -
> >  src/box/sql/vdbeInt.h   |  1 -
> >  src/box/sql/vdbeapi.c   | 19 -------------------
> >  src/box/sql/vdbeaux.c   | 16 ----------------
> >  src/box/sql/vdbemem.c   |  1 -
> >  src/box/sql/whereexpr.c |  2 --
> >  test/sql/bind.result    | 32 ++++++++++++++++++++++++++++++++
> >  test/sql/bind.test.lua  |  9 +++++++++
> >  8 files changed, 41 insertions(+), 40 deletions(-)
> >