[Tarantool-patches] [PATCH v1 02/13] sql: remove AggStep0 and OP_BuiltinFunction0
Mergen Imeev
imeevma at tarantool.org
Fri Sep 10 19:27:05 MSK 2021
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 at 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 at 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 at 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 at 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
>
More information about the Tarantool-patches
mailing list