[Tarantool-patches] [PATCH v2 1/5] sql: remove OP_BuiltinFunction0 and OP_AggStep0

imeevma at tarantool.org imeevma at tarantool.org
Thu Aug 19 08:31:26 MSK 2021


This patch moves the initialization of sql_context out of the VDBE. This
allows us to remove the opcodes OP_BuiltinFunction0 and OP_AggStep0,
which work in a rather strange way. Moreover, due to the changes these
opcodes make to the VDBEs, it is possible that the estimated size of the
VDBE could become non-constant, which could lead to various problems.

Part of #6105
---
 src/box/sql/analyze.c |  6 +--
 src/box/sql/expr.c    |  2 +-
 src/box/sql/func.c    | 13 ++++++-
 src/box/sql/select.c  |  2 +-
 src/box/sql/vdbe.c    | 87 ++-----------------------------------------
 src/box/sql/vdbeInt.h |  2 +-
 src/box/sql/vdbeaux.c |  2 +-
 7 files changed, 22 insertions(+), 92 deletions(-)

diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index fb4ad2a81..b9b6f5da8 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -725,7 +725,7 @@ callStatGet(Vdbe * v, int regStat4, int iParam, int regOut)
 	 */
 	struct func *func = sql_func_by_signature("_sql_stat_get", 2);
 	assert(func != NULL);
-	sqlVdbeAddOp4(v, OP_BuiltinFunction0, 0, regStat4, regOut,
+	sqlVdbeAddOp4(v, OP_BuiltinFunction, 0, regStat4, regOut,
 		      (char *)func, P4_FUNC);
 	sqlVdbeChangeP5(v, 2);
 }
@@ -869,7 +869,7 @@ vdbe_emit_analyze_space(struct Parse *parse, struct space *space)
 		struct func *init_func =
 			sql_func_by_signature("_sql_stat_init", 3);
 		assert(init_func != NULL);
-		sqlVdbeAddOp4(v, OP_BuiltinFunction0, 0, stat4_reg + 1,
+		sqlVdbeAddOp4(v, OP_BuiltinFunction, 0, stat4_reg + 1,
 			      stat4_reg, (char *)init_func, P4_FUNC);
 		sqlVdbeChangeP5(v, 3);
 		/*
@@ -974,7 +974,7 @@ vdbe_emit_analyze_space(struct Parse *parse, struct space *space)
 		struct func *push_func =
 			sql_func_by_signature("_sql_stat_push", 3);
 		assert(push_func != NULL);
-		sqlVdbeAddOp4(v, OP_BuiltinFunction0, 1, stat4_reg, tmp_reg,
+		sqlVdbeAddOp4(v, OP_BuiltinFunction, 1, stat4_reg, tmp_reg,
 			      (char *)push_func, P4_FUNC);
 		sqlVdbeChangeP5(v, 3);
 		sqlVdbeAddOp2(v, OP_Next, idx_cursor, next_row_addr);
diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 50dc98a94..d0bdee4fd 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -4168,7 +4168,7 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 				sqlVdbeAddOp4(v, OP_CollSeq, 0, 0, 0,
 						  (char *)coll, P4_COLLSEQ);
 			}
-			if (sql_emit_func_call(v, pExpr, OP_BuiltinFunction0,
+			if (sql_emit_func_call(v, pExpr, OP_BuiltinFunction,
 					       constMask, r1, target,
 					       nFarg) != 0) {
 				pParse->is_aborted = true;
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index ea3b44472..4a0d2d097 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -2889,7 +2889,18 @@ sql_emit_func_call(struct Vdbe *vdbe, struct Expr *expr, int op, int mask,
 	struct func *func = sql_func_find(expr);
 	if (func == NULL)
 		return -1;
-	sqlVdbeAddOp4(vdbe, op, mask, r1, r2, (char *)func, P4_FUNC);
+	uint32_t size = sizeof(struct sql_context);
+	if (argc > 1)
+		size += (argc - 1) * sizeof(struct Mem);
+	struct sql_context *ctx = sqlDbMallocRawNN(sql_get(), size);
+	if (ctx == NULL)
+		return -1;
+	ctx->pOut = NULL;
+	ctx->func = func;
+	ctx->iOp = 0;
+	ctx->pVdbe = vdbe;
+	ctx->argc = argc;
+	sqlVdbeAddOp4(vdbe, op, mask, r1, r2, (char *)ctx, P4_FUNCCTX);
 	sqlVdbeChangeP5(vdbe, argc);
 	return 0;
 }
diff --git a/src/box/sql/select.c b/src/box/sql/select.c
index 87f2012f1..cb92f2ed0 100644
--- a/src/box/sql/select.c
+++ b/src/box/sql/select.c
@@ -5632,7 +5632,7 @@ updateAccumulator(Parse * pParse, AggInfo * pAggInfo)
 			sqlVdbeAddOp4(v, OP_CollSeq, regHit, 0, 0,
 					  (char *)coll, P4_COLLSEQ);
 		}
-		sql_emit_func_call(v, pF->pExpr, OP_AggStep0, 0, regAgg,
+		sql_emit_func_call(v, pF->pExpr, OP_AggStep, 0, regAgg,
 				   pF->iMem, nArg);
 		sql_expr_type_cache_change(pParse, regAgg, nArg);
 		sqlReleaseTempRange(pParse, regAgg, nArg);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 7f86fa7b3..98ea37c67 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1181,20 +1181,6 @@ case OP_CollSeq: {
 	break;
 }
 
-/* Opcode: BuiltinFunction0 P1 P2 P3 P4 P5
- * Synopsis: r[P3]=func(r[P2 at P5])
- *
- * Invoke a user function (P4 is a pointer to a FuncDef object that
- * defines the function) with P5 arguments taken from register P2 and
- * successors.  The result of the function is stored in register P3.
- * Register P3 must not be one of the function inputs.
- *
- * P1 is a 32-bit bitmask indicating whether or not each argument to the
- * function was determined to be constant at compile time. If the first
- * argument was constant then bit 0 of P1 is set.
- *
- * See also: BuiltinFunction, AggStep, AggFinal
- */
 /* Opcode: BuiltinFunction P1 P2 P3 P4 P5
  * Synopsis: r[P3]=func(r[P2 at P5])
  *
@@ -1207,44 +1193,15 @@ case OP_CollSeq: {
  * function was determined to be constant at compile time. If the first
  * argument was constant then bit 0 of P1 is set.
  *
- * SQL functions are initially coded as OP_BuiltinFunction0 with
- * P4 pointing to a FuncDef object.  But on first evaluation,
- * the P4 operand is automatically converted into an sql_context
- * object and the operation changed to this OP_BuiltinFunction
- * opcode.  In this way, the initialization of the sql_context
- * object occurs only once, rather than once for each evaluation
- * of the function.
- *
- * See also: BuiltinFunction0, AggStep, AggFinal
+ * See also: AggStep, AggFinal
  */
-case OP_BuiltinFunction0: {
-	int n;
-	sql_context *pCtx;
-
-	assert(pOp->p4type == P4_FUNC);
-	n = pOp->p5;
-	assert(pOp->p3>0 && pOp->p3<=(p->nMem+1 - p->nCursor));
-	assert(n==0 || (pOp->p2>0 && pOp->p2+n<=(p->nMem+1 - p->nCursor)+1));
-	assert(pOp->p3<pOp->p2 || pOp->p3>=pOp->p2+n);
-	pCtx = sqlDbMallocRawNN(db, sizeof(*pCtx) + (n-1)*sizeof(sql_value*));
-	if (pCtx==0) goto no_mem;
-	pCtx->pOut = 0;
-	pCtx->func = pOp->p4.func;
-	pCtx->iOp = (int)(pOp - aOp);
-	pCtx->pVdbe = p;
-	pCtx->argc = n;
-	pOp->p4type = P4_FUNCCTX;
-	pOp->p4.pCtx = pCtx;
-	pOp->opcode = OP_BuiltinFunction;
-	/* Fall through into OP_BuiltinFunction */
-	FALLTHROUGH;
-}
 case OP_BuiltinFunction: {
 	int i;
 	sql_context *pCtx;
 
 	assert(pOp->p4type==P4_FUNCCTX);
 	pCtx = pOp->p4.pCtx;
+	pCtx->iOp = (int)(pOp - aOp);
 
 	/* If this function is inside of a trigger, the register array in aMem[]
 	 * might change from one evaluation to the next.  The next block of code
@@ -4184,17 +4141,6 @@ case OP_DecrJumpZero: {      /* jump, in1 */
 }
 
 
-/* Opcode: AggStep0 * P2 P3 P4 P5
- * Synopsis: accum=r[P3] step(r[P2 at P5])
- *
- * Execute the step function for an aggregate.  The
- * function has P5 arguments.   P4 is a pointer to the FuncDef
- * structure that specifies the function.  Register P3 is the
- * accumulator.
- *
- * The P5 arguments are taken from register P2 and its
- * successors.
- */
 /* Opcode: AggStep * P2 P3 P4 P5
  * Synopsis: accum=r[P3] step(r[P2 at P5])
  *
@@ -4205,35 +4151,7 @@ case OP_DecrJumpZero: {      /* jump, in1 */
  *
  * The P5 arguments are taken from register P2 and its
  * successors.
- *
- * This opcode is initially coded as OP_AggStep0.  On first evaluation,
- * the FuncDef stored in P4 is converted into an sql_context and
- * the opcode is changed.  In this way, the initialization of the
- * sql_context only happens once, instead of on each call to the
- * step function.
  */
-case OP_AggStep0: {
-	int n;
-	sql_context *pCtx;
-
-	assert(pOp->p4type == P4_FUNC);
-	n = pOp->p5;
-	assert(pOp->p3>0 && pOp->p3<=(p->nMem+1 - p->nCursor));
-	assert(n==0 || (pOp->p2>0 && pOp->p2+n<=(p->nMem+1 - p->nCursor)+1));
-	assert(pOp->p3<pOp->p2 || pOp->p3>=pOp->p2+n);
-	pCtx = sqlDbMallocRawNN(db, sizeof(*pCtx) + (n-1)*sizeof(sql_value*));
-	if (pCtx==0) goto no_mem;
-	pCtx->pMem = 0;
-	pCtx->func = pOp->p4.func;
-	pCtx->iOp = (int)(pOp - aOp);
-	pCtx->pVdbe = p;
-	pCtx->argc = n;
-	pOp->p4type = P4_FUNCCTX;
-	pOp->p4.pCtx = pCtx;
-	pOp->opcode = OP_AggStep;
-	/* Fall through into OP_AggStep */
-	FALLTHROUGH;
-}
 case OP_AggStep: {
 	int i;
 	sql_context *pCtx;
@@ -4242,6 +4160,7 @@ case OP_AggStep: {
 
 	assert(pOp->p4type==P4_FUNCCTX);
 	pCtx = pOp->p4.pCtx;
+	pCtx->iOp = (int)(pOp - aOp);
 	pMem = &aMem[pOp->p3];
 
 	/* If this function is inside of a trigger, the register array in aMem[]
diff --git a/src/box/sql/vdbeInt.h b/src/box/sql/vdbeInt.h
index cfe743b94..073ac0b97 100644
--- a/src/box/sql/vdbeInt.h
+++ b/src/box/sql/vdbeInt.h
@@ -174,7 +174,7 @@ struct sql_context {
 	struct func *func;
 	Mem *pMem;		/* Memory cell used to store aggregate context */
 	Vdbe *pVdbe;		/* The VM that owns this context */
-	/** Instruction number of OP_BuiltinFunction0. */
+	/** Instruction number of OP_BuiltinFunction or OP_AggStep. */
 	int iOp;
 	/*
 	 * True, if an error occurred during the execution of the
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 2d7800b17..662fbbf81 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1070,7 +1070,7 @@ displayP4(Op * pOp, char *zTemp, int nTemp)
 		}
 #if defined(SQL_DEBUG) || defined(VDBE_PROFILE)
 	case P4_FUNCCTX:{
-			struct func *func = pOp->p4.func;
+			struct func *func = pOp->p4.pCtx->func;
 			sqlXPrintf(&x, "%s(%d)", func->def->name,
 				   func->def->param_count);
 			break;
-- 
2.25.1



More information about the Tarantool-patches mailing list