Re: [Tarantool-patches] [PATCH v2 06/15] sql: remove sql_vdbemem_finalize()

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

Thank you for the review! My answers, diff and new patch below.

On 23.09.2021 01:47, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
>
>>>> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
>>>> index 12dc9126b..dfab86c50 100644
>>>> --- a/src/box/sql/vdbe.c
>>>> +++ b/src/box/sql/vdbe.c
>>>> @@ -4204,16 +4204,32 @@ case OP_AggStep: {
>>>>   * the step function was not previously called.
>>>>   */
>>>>  case OP_AggFinal: {
>>>> -	Mem *pMem;
>>>>  	assert(pOp->p1>0 && pOp->p1<=(p->nMem+1 - p->nCursor));
>>>> -	pMem = &aMem[pOp->p1];
>>>> -	assert(mem_is_null(pMem) || mem_is_agg(pMem));
>>>> -	if (sql_vdbemem_finalize(pMem, pOp->p4.func) != 0)
>>>> +	struct func_sql_builtin *func = (struct func_sql_builtin *)pOp->p4.func;
>>>> +	struct Mem *pIn1 = &aMem[pOp->p1];
>>>> +	assert(mem_is_null(pIn1) || mem_is_agg(pIn1));
>>>> +
>>>> +	struct sql_context ctx;
>>>> +	memset(&ctx, 0, sizeof(ctx));
>>>> +	struct Mem t;
>>>
>>> Please, use mem_create(). Also for the context I would propose to add
>>> sql_context_create(). So as not to touch internals of the struct here
>>> directly.
>>>
>> I suggest to leave it as it is for now, since I plan to completely drop
>> struct sql_context from finalize functions in a few patches in this patch-set.
>> What do you think?
>
> mem_create() would be just -3 and +1 line in the patch. I suggest to
> use it. Previously memset was tolerated because it was done in mem.c,
> where it is allowed to touch Mem internals directly.
>
Fixed.

On Thu, Sep 23, 2021 at 12:47:45AM +0200, Vladislav Shpilevoy wrote:
> Thanks for the fixes!
> 
> > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
> > index a9011fc63..fb304d868 100644
> > --- a/src/box/sql/mem.c
> > +++ b/src/box/sql/mem.c
> > @@ -217,11 +217,7 @@ mem_create(struct Mem *mem)
> >  static inline void
> >  mem_clear(struct Mem *mem)
> >  {
> > -	if ((mem->type & (MEM_TYPE_AGG | MEM_TYPE_FRAME)) != 0 ||
> > -	    (mem->flags & MEM_Dyn) != 0) {
> > -		if (mem->type == MEM_TYPE_AGG)
> > -			sql_vdbemem_finalize(mem, mem->u.func);
> > -		assert(mem->type != MEM_TYPE_AGG);
> > +	if ((mem->type & MEM_TYPE_FRAME) != 0 || (mem->flags & MEM_Dyn) != 0) {
> 
> It is a single type now, you can use type == MEM_TYPE_FRAME. Also a few lines
> below you again check both the conditions:
> 
> 	if ((mem->flags & MEM_Dyn) != 0) {
> 		...
> 	} else if (mem->type == MEM_TYPE_FRAME) {
> 		...
> 	}
> 
> The 'else' don't need to check type == FRAME. It is true
> anyway if this was true:
> 
> 	if ((mem->type & MEM_TYPE_FRAME) != 0 || (mem->flags & MEM_Dyn) != 0)
> 
> and "(mem->flags & MEM_Dyn) != 0" wasn't.
Fixed. In addition, I believe that by the end of the refactoring of all the SQL
built-in functions, we will drop MEM_Dyn as it is mostly, if not only, used in
functions.



Diff:

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index bb26da60a..e59935672 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -217,11 +217,11 @@ mem_create(struct Mem *mem)
 static inline void
 mem_clear(struct Mem *mem)
 {
-	if ((mem->type & MEM_TYPE_FRAME) != 0 || (mem->flags & MEM_Dyn) != 0) {
+	if (mem->type == MEM_TYPE_FRAME || (mem->flags & MEM_Dyn) != 0) {
 		if ((mem->flags & MEM_Dyn) != 0) {
 			assert(mem->xDel != SQL_DYNAMIC && mem->xDel != NULL);
 			mem->xDel((void *)mem->z);
-		} else if (mem->type == MEM_TYPE_FRAME) {
+		} else {
 			struct VdbeFrame *frame = mem->u.pFrame;
 			frame->pParent = frame->v->pDelFrame;
 			frame->v->pDelFrame = frame;
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index dfab86c50..b101fc6c2 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4212,10 +4212,7 @@ case OP_AggFinal: {
 	struct sql_context ctx;
 	memset(&ctx, 0, sizeof(ctx));
 	struct Mem t;
-	memset(&t, 0, sizeof(t));
-	t.type = MEM_TYPE_NULL;
-	assert(t.flags == 0);
-	t.db = pIn1->db;
+	mem_create(&t);
 	ctx.pOut = &t;
 	ctx.pMem = pIn1;
 	ctx.func = pOp->p4.func;


New patch:

commit d30d6b78e5c9486fdc1986e74e6340e11a960f7f
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Thu Sep 9 17:19:03 2021 +0300

    sql: remove sql_vdbemem_finalize()
    
    We don't need this function, since it is easier to call finalizers
    directly. This patch also allows us to make further simplifications.
    
    Needed for #4145

diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index f64cbe10a..e59935672 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -217,15 +217,11 @@ mem_create(struct Mem *mem)
 static inline void
 mem_clear(struct Mem *mem)
 {
-	if ((mem->type & (MEM_TYPE_AGG | MEM_TYPE_FRAME)) != 0 ||
-	    (mem->flags & MEM_Dyn) != 0) {
-		if (mem->type == MEM_TYPE_AGG)
-			sql_vdbemem_finalize(mem, mem->u.func);
-		assert(mem->type != MEM_TYPE_AGG);
+	if (mem->type == MEM_TYPE_FRAME || (mem->flags & MEM_Dyn) != 0) {
 		if ((mem->flags & MEM_Dyn) != 0) {
 			assert(mem->xDel != SQL_DYNAMIC && mem->xDel != NULL);
 			mem->xDel((void *)mem->z);
-		} else if (mem->type == MEM_TYPE_FRAME) {
+		} else {
 			struct VdbeFrame *frame = mem->u.pFrame;
 			frame->pParent = frame->v->pDelFrame;
 			frame->v->pDelFrame = frame;
@@ -3067,31 +3063,6 @@ sqlVdbeMemTooBig(Mem * p)
 	return 0;
 }
 
-int
-sql_vdbemem_finalize(struct Mem *mem, struct func *func)
-{
-	assert(func != NULL);
-	assert(func->def->language == FUNC_LANGUAGE_SQL_BUILTIN);
-	assert(func->def->aggregate == FUNC_AGGREGATE_GROUP);
-	assert(mem->type == MEM_TYPE_NULL || func == mem->u.func);
-	sql_context ctx;
-	memset(&ctx, 0, sizeof(ctx));
-	Mem t;
-	memset(&t, 0, sizeof(t));
-	t.type = MEM_TYPE_NULL;
-	assert(t.flags == 0);
-	t.db = mem->db;
-	ctx.pOut = &t;
-	ctx.pMem = mem;
-	ctx.func = func;
-	((struct func_sql_builtin *)func)->finalize(&ctx);
-	assert((mem->flags & MEM_Dyn) == 0);
-	if (mem->szMalloc > 0)
-		sqlDbFree(mem->db, mem->zMalloc);
-	memcpy(mem, &t, sizeof(t));
-	return ctx.is_aborted ? -1 : 0;
-}
-
 int
 sqlVdbeRecordCompareMsgpack(const void *key1,
 				struct UnpackedRecord *key2)
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index 54a1e931b..c0993e573 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -969,18 +969,6 @@ int sqlVdbeMemTooBig(Mem *);
 #define VdbeMemDynamic(X) (((X)->flags & MEM_Dyn) != 0 ||\
 			   ((X)->type & (MEM_TYPE_AGG | MEM_TYPE_FRAME)) != 0)
 
-/** MEM manipulate functions. */
-
-/**
- * Memory cell mem contains the context of an aggregate function.
- * This routine calls the finalize method for that function. The
- * result of the aggregate is stored back into mem.
- *
- * Returns -1 if the finalizer reports an error. 0 otherwise.
- */
-int
-sql_vdbemem_finalize(struct Mem *mem, struct func *func);
-
 /**
  * Perform comparison of two tuples: unpacked (key1) and packed (key2)
  *
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 12dc9126b..b101fc6c2 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -4204,16 +4204,29 @@ case OP_AggStep: {
  * the step function was not previously called.
  */
 case OP_AggFinal: {
-	Mem *pMem;
 	assert(pOp->p1>0 && pOp->p1<=(p->nMem+1 - p->nCursor));
-	pMem = &aMem[pOp->p1];
-	assert(mem_is_null(pMem) || mem_is_agg(pMem));
-	if (sql_vdbemem_finalize(pMem, pOp->p4.func) != 0)
+	struct func_sql_builtin *func = (struct func_sql_builtin *)pOp->p4.func;
+	struct Mem *pIn1 = &aMem[pOp->p1];
+	assert(mem_is_null(pIn1) || mem_is_agg(pIn1));
+
+	struct sql_context ctx;
+	memset(&ctx, 0, sizeof(ctx));
+	struct Mem t;
+	mem_create(&t);
+	ctx.pOut = &t;
+	ctx.pMem = pIn1;
+	ctx.func = pOp->p4.func;
+	func->finalize(&ctx);
+	assert((pIn1->flags & MEM_Dyn) == 0);
+	if (pIn1->szMalloc > 0)
+		sqlDbFree(pIn1->db, pIn1->zMalloc);
+	memcpy(pIn1, &t, sizeof(t));
+
+	if (ctx.is_aborted)
 		goto abort_due_to_error;
-	UPDATE_MAX_BLOBSIZE(pMem);
-	if (sqlVdbeMemTooBig(pMem)) {
+	UPDATE_MAX_BLOBSIZE(pIn1);
+	if (sqlVdbeMemTooBig(pIn1) != 0)
 		goto too_big;
-	}
 	break;
 }