From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id A9BAA6EC55; Fri, 10 Sep 2021 19:27:09 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A9BAA6EC55 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1631291229; bh=u3lyzPs6LWPoInMZv/6C1FdG0TC+dSNlptQHWON57uo=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=h7mSeMwiHgi1p2DL6ZHU12FH+zNhCiTvWCjrfTvVKeZrKz33IYF7pbMW6MB3NfY2k cOZXxJQDdT9nncZc94U9GJh8he5Se/O5MOps0bHKpRtHlO3Xyd7uiqJt7Hj1ssX40z cNWcMM0FXk8Hm0DVgoqK7Tt6Vf83ctNjekoGAHG0= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id C8F526EC55 for ; Fri, 10 Sep 2021 19:27:07 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C8F526EC55 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1mOjMo-0006mH-UP; Fri, 10 Sep 2021 19:27:07 +0300 Date: Fri, 10 Sep 2021 19:27:05 +0300 To: v.shpilevoy@tarantool.org, tarantool-patches@dev.tarantool.org Message-ID: <20210910162705.GA356901@tarantool.org> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD91AE02D33A9C88A2F250B8A21226859B08342E9AB5F4ECC3300894C459B0CD1B99D35D838DE779B7C4930E1CC892A966500FA8D3CB3048F8D8ED064EC5171B301 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE79173C6E970493712EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637A6A20D80F0832BC78638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8BAAD271C43BF0778E8A8298B970815D1117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8B8C7ADC89C2F0B2A5A471835C12D1D977C4224003CC8364762BB6847A3DEAEFB0F43C7A68FF6260569E8FC8737B5C2249EC8D19AE6D49635B68655334FD4449CB9ECD01F8117BC8BEAAAE862A0553A39223F8577A6DFFEA7CD1D040B6C1ECEA3F43847C11F186F3C59DAA53EE0834AAEE X-C1DE0DAB: 0D63561A33F958A57DC3FBC8650AC0228F63ED61D49AA567A7C6B4CA05D7314CD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75E3127721F5A72C97410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34C264B329661203DA8173871E700C78B344D7858768B1B0899D84BB79E356B1CF399DD7E37D17C3261D7E09C32AA3244CB2EF498719347D82D3321360222DDD9551E887DA02A9F7BF729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojFfmM42hctyRtLui84Zwylw== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5D8C77B02E3F338D4F80403CF0543F720283D72C36FC87018B9F80AB2734326CD2FB559BB5D741EB96352A0ABBE4FDA4210A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v1 02/13] sql: remove AggStep0 and OP_BuiltinFunction0 X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Mergen Imeev via Tarantool-patches Reply-To: Mergen Imeev Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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->p3p2 || 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->p3p2 || 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 >