Re: [Tarantool-patches] [PATCH v1 02/13] sql: remove AggStep0 and OP_BuiltinFunction0

[Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi! Sorry, I found that there is one failing test in release built. I make some
changes, diff:


diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index e2dcc72ab..b7e44a386 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -1068,14 +1068,12 @@ displayP4(Op * pOp, char *zTemp, int nTemp)
 				   func->def->param_count);
 			break;
 		}
-#if defined(SQL_DEBUG) || defined(VDBE_PROFILE)
 	case P4_FUNCCTX:{
 			struct func *func = pOp->p4.pCtx->func;
 			sqlXPrintf(&x, "%s(%d)", func->def->name,
 				   func->def->param_count);
 			break;
 		}
-#endif
 	case P4_BOOL:
 			sqlXPrintf(&x, "%d", pOp->p4.b);
 			break;

On Fri, Sep 10, 2021 at 07:01:52PM +0300, Mergen Imeev via Tarantool-patches wrote:
> 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 may change during execution of VDBE, which could lead to various
> problems.
> 
> Needed for #4145
> ---
>  src/box/sql/expr.c    | 12 +++++--
>  src/box/sql/select.c  | 27 +++++++++++++--
>  src/box/sql/sqlInt.h  |  3 ++
>  src/box/sql/vdbe.c    | 81 ++-----------------------------------------
>  src/box/sql/vdbeaux.c |  2 +-
>  5 files changed, 41 insertions(+), 84 deletions(-)
> 
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 3b5df5292..6446ef091 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -4107,9 +4107,15 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
>  						  (char *)coll, P4_COLLSEQ);
>  			}
>  			if (func->def->language == FUNC_LANGUAGE_SQL_BUILTIN) {
> -				sqlVdbeAddOp4(v, OP_BuiltinFunction0, nFarg,
> -					      r1, target, (char *)func,
> -					      P4_FUNC);
> +				struct sql_context *ctx =
> +					sql_context_new(v, func, nFarg);
> +				if (ctx == NULL) {
> +					pParse->is_aborted = true;
> +					return -1;
> +				}
> +				sqlVdbeAddOp4(v, OP_BuiltinFunction, nFarg,
> +					      r1, target, (char *)ctx,
> +					      P4_FUNCCTX);
>  			} else {
>  				sqlVdbeAddOp4(v, OP_FunctionByName, nFarg,
>  					      r1, target,
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 92e40aef6..e27dcb336 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -5639,8 +5639,13 @@ updateAccumulator(Parse * pParse, AggInfo * pAggInfo)
>  			sqlVdbeAddOp4(v, OP_CollSeq, regHit, 0, 0,
>  					  (char *)coll, P4_COLLSEQ);
>  		}
> -		sqlVdbeAddOp3(v, OP_AggStep0, nArg, regAgg, pF->iMem);
> -		sqlVdbeAppendP4(v, pF->func, P4_FUNC);
> +		struct sql_context *ctx = sql_context_new(v, pF->func, nArg);
> +		if (ctx == NULL) {
> +			pParse->is_aborted = true;
> +			return;
> +		}
> +		sqlVdbeAddOp3(v, OP_AggStep, nArg, regAgg, pF->iMem);
> +		sqlVdbeAppendP4(v, ctx, P4_FUNCCTX);
>  		sql_expr_type_cache_change(pParse, regAgg, nArg);
>  		sqlReleaseTempRange(pParse, regAgg, nArg);
>  		if (addrNext) {
> @@ -6744,3 +6749,21 @@ sql_expr_extract_select(struct Parse *parser, struct Select *select)
>  	parser->parsed_ast.expr =
>  		sqlExprDup(parser->db, expr_list->a->pExpr, 0);
>  }
> +
> +struct sql_context *
> +sql_context_new(struct Vdbe *vdbe, struct func *func, uint32_t argc)
> +{
> +	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 NULL;
> +	ctx->pOut = NULL;
> +	ctx->func = func;
> +	ctx->is_aborted = false;
> +	ctx->skipFlag = 0;
> +	ctx->pVdbe = vdbe;
> +	ctx->iOp = 0;
> +	return ctx;
> +}
> diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
> index 2e250dc29..c2701dbde 100644
> --- a/src/box/sql/sqlInt.h
> +++ b/src/box/sql/sqlInt.h
> @@ -4141,6 +4141,9 @@ char *sqlStrAccumFinish(StrAccum *);
>  void sqlStrAccumReset(StrAccum *);
>  void sqlSelectDestInit(SelectDest *, int, int, int);
>  
> +struct sql_context *
> +sql_context_new(struct Vdbe *vdbe, struct func *func, uint32_t argc);
> +
>  /*
>   * Create an expression to load @a column from datasource
>   * @a src_idx in @a src_list.
> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index a0f1be454..2ff7ce8f4 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1182,16 +1182,6 @@ case OP_CollSeq: {
>  	break;
>  }
>  
> -/* Opcode: BuiltinFunction0 P1 P2 P3 P4 *
> - * Synopsis: r[P3]=func(r[P2@P1])
> - *
> - * Invoke a user function (P4 is a pointer to a FuncDef object that
> - * defines the function) with P1 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.
> - *
> - * See also: BuiltinFunction, AggStep, AggFinal
> - */
>  /* Opcode: BuiltinFunction P1 P2 P3 P4 *
>   * Synopsis: r[P3]=func(r[P2@P1])
>   *
> @@ -1200,37 +1190,8 @@ case OP_CollSeq: {
>   * 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.
>   *
> - * 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->p1;
> -	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;
> -	pOp->p4type = P4_FUNCCTX;
> -	pOp->p4.pCtx = pCtx;
> -	pOp->opcode = OP_BuiltinFunction;
> -	/* Fall through into OP_BuiltinFunction */
> -	FALLTHROUGH;
> -}
>  case OP_BuiltinFunction: {
>  	int i;
>  	int argc = pOp->p1;
> @@ -1238,6 +1199,7 @@ case OP_BuiltinFunction: {
>  
>  	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
> @@ -4178,17 +4140,6 @@ case OP_DecrJumpZero: {      /* jump, in1 */
>  }
>  
>  
> -/* Opcode: AggStep0 P1 P2 P3 P4 *
> - * Synopsis: accum=r[P3] step(r[P2@P1])
> - *
> - * Execute the step function for an aggregate.  The
> - * function has P1 arguments.   P4 is a pointer to the FuncDef
> - * structure that specifies the function.  Register P3 is the
> - * accumulator.
> - *
> - * The P1 arguments are taken from register P2 and its
> - * successors.
> - */
>  /* Opcode: AggStep P1 P2 P3 P4 *
>   * Synopsis: accum=r[P3] step(r[P2@P1])
>   *
> @@ -4199,34 +4150,7 @@ case OP_DecrJumpZero: {      /* jump, in1 */
>   *
>   * The P1 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;
> -	pOp->p4type = P4_FUNCCTX;
> -	pOp->p4.pCtx = pCtx;
> -	pOp->opcode = OP_AggStep;
> -	/* Fall through into OP_AggStep */
> -	FALLTHROUGH;
> -}
>  case OP_AggStep: {
>  	int i;
>  	int argc = pOp->p1;
> @@ -4236,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/vdbeaux.c b/src/box/sql/vdbeaux.c
> index 8148ed8b0..e2dcc72ab 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
>