Tarantool development patches archive
 help / color / mirror / Atom feed
From: Mergen Imeev via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: v.shpilevoy@tarantool.org
Cc: tarantool-patches@dev.tarantool.org
Subject: [Tarantool-patches] [PATCH v4 04/16] sql: remove AggStep0 and OP_BuiltinFunction0
Date: Fri,  1 Oct 2021 15:48:37 +0300	[thread overview]
Message-ID: <4ee4802cb803a27b76e3102f6accb4df3a1dde4f.1633092363.git.imeevma@gmail.com> (raw)
In-Reply-To: <cover.1633092363.git.imeevma@gmail.com>

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  | 33 ++++++++++++++++--
 src/box/sql/sqlInt.h  |  6 ++++
 src/box/sql/vdbe.c    | 81 ++-----------------------------------------
 src/box/sql/vdbeaux.c | 12 ++-----
 5 files changed, 51 insertions(+), 93 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..5b57f57a3 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,27 @@ 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;
+}
+
+void
+sql_context_delete(struct sql_context *ctx)
+{
+	sqlDbFree(sql_get(), ctx);
+}
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index eac6bcec8..b944b357b 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -4127,6 +4127,12 @@ 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);
+
+void
+sql_context_delete(struct sql_context *ctx);
+
 /*
  * 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 af97b0499..11e418e53 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
@@ -4170,17 +4132,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])
  *
@@ -4191,34 +4142,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;
@@ -4228,6 +4152,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..4c2bd11ba 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -590,19 +590,13 @@ sqlVdbeJumpHere(Vdbe * p, int addr)
 
 static void vdbeFreeOpArray(sql *, Op *, int);
 
-static SQL_NOINLINE void
-freeP4FuncCtx(sql * db, sql_context * p)
-{
-	sqlDbFree(db, p);
-}
-
 static void
 freeP4(sql * db, int p4type, void *p4)
 {
 	assert(db);
 	switch (p4type) {
 	case P4_FUNCCTX:{
-			freeP4FuncCtx(db, (sql_context *) p4);
+			sql_context_delete(p4);
 			break;
 		}
 	case P4_REAL:
@@ -1068,14 +1062,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.func;
+			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;
-- 
2.25.1


  parent reply	other threads:[~2021-10-01 12:50 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-01 12:48 [Tarantool-patches] [PATCH v4 00/16] sql: refactor aggregate functions Mergen Imeev via Tarantool-patches
2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 01/16] sql: remove MEM_Zero flag from struct MEM Mergen Imeev via Tarantool-patches
2021-10-04 21:51   ` Vladislav Shpilevoy via Tarantool-patches
2021-10-05  8:46     ` Mergen Imeev via Tarantool-patches
2021-10-05  9:42       ` Mergen Imeev via Tarantool-patches
2021-10-05 12:28         ` Mergen Imeev via Tarantool-patches
2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 02/16] sql: fix possible undefined behavior during cast Mergen Imeev via Tarantool-patches
2021-10-04 21:52   ` Vladislav Shpilevoy via Tarantool-patches
2021-10-05  9:00     ` Mergen Imeev via Tarantool-patches
2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 03/16] sql: use register P1 for number of arguments Mergen Imeev via Tarantool-patches
2021-10-01 12:48 ` Mergen Imeev via Tarantool-patches [this message]
2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 05/16] sql: move collation to struct sql_context Mergen Imeev via Tarantool-patches
2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 06/16] sql: introduce mem_append() Mergen Imeev via Tarantool-patches
2021-10-04 21:52   ` Vladislav Shpilevoy via Tarantool-patches
2021-10-05  9:32     ` Mergen Imeev via Tarantool-patches
2021-10-11 21:50       ` Vladislav Shpilevoy via Tarantool-patches
2021-10-19 10:49         ` Mergen Imeev via Tarantool-patches
2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 07/16] sql: remove sql_vdbemem_finalize() Mergen Imeev via Tarantool-patches
2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 08/16] sql: refactor SUM() function Mergen Imeev via Tarantool-patches
2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 09/16] sql: refactor TOTAL() function Mergen Imeev via Tarantool-patches
2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 10/16] sql: refactor AVG() function Mergen Imeev via Tarantool-patches
2021-10-04 21:53   ` Vladislav Shpilevoy via Tarantool-patches
2021-10-05  9:48     ` Mergen Imeev via Tarantool-patches
2021-10-11 21:50       ` Vladislav Shpilevoy via Tarantool-patches
2021-10-19 11:14         ` Mergen Imeev via Tarantool-patches
2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 11/16] sql: refactor COUNT() function Mergen Imeev via Tarantool-patches
2021-10-04 21:53   ` Vladislav Shpilevoy via Tarantool-patches
2021-10-05  9:55     ` Mergen Imeev via Tarantool-patches
2021-10-11 21:51       ` Vladislav Shpilevoy via Tarantool-patches
2021-10-19 11:17         ` Mergen Imeev via Tarantool-patches
2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 12/16] sql: refactor MIN() and MAX() functions Mergen Imeev via Tarantool-patches
2021-10-04 21:54   ` Vladislav Shpilevoy via Tarantool-patches
2021-10-05 10:07     ` Mergen Imeev via Tarantool-patches
2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 13/16] sql: refactor GROUP_CONCAT() function Mergen Imeev via Tarantool-patches
2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 14/16] sql: remove copying of result in finalizers Mergen Imeev via Tarantool-patches
2021-10-01 12:48 ` [Tarantool-patches] [PATCH v4 15/16] sql: remove MEM_TYPE_AGG Mergen Imeev via Tarantool-patches
2021-10-01 12:49 ` [Tarantool-patches] [PATCH v4 16/16] sql: remove field argv from struct sql_context Mergen Imeev via Tarantool-patches
2021-10-25 20:58 ` [Tarantool-patches] [PATCH v4 00/16] sql: refactor aggregate functions Vladislav Shpilevoy via Tarantool-patches
2021-10-26 10:34 Mergen Imeev via Tarantool-patches
2021-10-26 10:34 ` [Tarantool-patches] [PATCH v4 04/16] sql: remove AggStep0 and OP_BuiltinFunction0 Mergen Imeev via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4ee4802cb803a27b76e3102f6accb4df3a1dde4f.1633092363.git.imeevma@gmail.com \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=imeevma@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v4 04/16] sql: remove AggStep0 and OP_BuiltinFunction0' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox