[Tarantool-patches] [PATCH v2 06/15] sql: remove sql_vdbemem_finalize()
Mergen Imeev
imeevma at tarantool.org
Sat Sep 25 14:13:54 MSK 2021
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 at 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;
}
More information about the Tarantool-patches
mailing list