[Tarantool-patches] [PATCH v5 13/52] sql: introduce mem_copy()
imeevma at tarantool.org
imeevma at tarantool.org
Fri Apr 9 20:36:56 MSK 2021
Thank you for the review! My answers and new patch below.
On 30.03.2021 02:01, Vladislav Shpilevoy wrote:
> I appreciate the work you did here!
>
> See 2 comments below.
>
> On 23.03.2021 10:35, Mergen Imeev via Tarantool-patches wrote:
>> This patch introduces mem_copy(). This function copies value from source
>> MEM to destination MEM. In case value is string or binary and have not
>> static allocation type, it is copied to newly allocated memory.
>>
>> Part of #5818
>> ---
>> src/box/sql/func.c | 4 ++--
>> src/box/sql/mem.c | 54 +++++++++++++++++++++++++------------------
>> src/box/sql/mem.h | 9 +++++++-
>> src/box/sql/vdbeapi.c | 2 +-
>> src/box/sql/vdbeaux.c | 2 +-
>> 5 files changed, 44 insertions(+), 27 deletions(-)
>>
>> diff --git a/src/box/sql/func.c b/src/box/sql/func.c
>> index 81b537d9b..6b6081150 100644
>> --- a/src/box/sql/func.c
>> +++ b/src/box/sql/func.c
>> @@ -2079,13 +2079,13 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv)
>> bool is_max = (func->flags & SQL_FUNC_MAX) != 0;
>> cmp = sqlMemCompare(pBest, pArg, pColl);
>> if ((is_max && cmp < 0) || (!is_max && cmp > 0)) {
>> - sqlVdbeMemCopy(pBest, pArg);
>
> 1. Still is "used" in stat4ValueFromExpr().
>
Fixed.
>> + mem_copy(pBest, pArg);
>> } else {
>> sqlSkipAccumulatorLoad(context);
>> }
>> } else {
>> pBest->db = sql_context_db_handle(context);
>> - sqlVdbeMemCopy(pBest, pArg);
>> + mem_copy(pBest, pArg);
>> }
>> }
>>
>> diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
>> index abc9291ef..f12441d7c 100644
>> --- a/src/box/sql/mem.c
>> +++ b/src/box/sql/mem.c
>> @@ -257,6 +257,38 @@ mem_destroy(struct Mem *mem)
>> mem->zMalloc = NULL;
>> }
>>
>> +int
>> +mem_copy(struct Mem *to, const struct Mem *from)
>> +{
>> + mem_clear(to);
>> + to->u = from->u;
>> + to->flags = from->flags;
>> + to->subtype = from->subtype;
>> + to->field_type = from->field_type;
>> + to->n = from->n;
>> + to->z = from->z;
>> + if ((to->flags & (MEM_Str | MEM_Blob)) == 0)
>> + return 0;
>> + if ((to->flags & MEM_Static) != 0)
>> + return 0;
>> + if ((to->flags & (MEM_Zero | MEM_Blob)) == (MEM_Zero | MEM_Blob)) {
>> + if (sqlVdbeMemExpandBlob(to) != 0)
>> + return -1;
>> + return 0;
>
> 2. You can make `returnsqlVdbeMemExpandBlob(to);`, no need to check its result.
>
Fixed.
> Also what was wrong with sqlVdbeMemCopy's way of using sqlVdbeMemMakeWriteable?
>
I see that this as a hack. It changes dynamic or allocated type (only type!) to
ephemeral and then calls sqlVdbeMemMakeWriteable(), which converts ephemeral
value to allocated value. Isn't it better to just directly copy?
>> + }
>> + if (to->szMalloc == 0)
>> + to->zMalloc = sqlDbMallocRaw(to->db, to->n);
>> + else
>> + to->zMalloc = sqlDbReallocOrFree(to->db, to->zMalloc, to->n);
>> + if (to->zMalloc == NULL)
>> + return -1;
>> + to->szMalloc = sqlDbMallocSize(to->db, to->zMalloc);
>> + memcpy(to->zMalloc, to->z, to->n);
>> + to->z = to->zMalloc;
>> + to->flags &= (MEM_Str | MEM_Blob | MEM_Term | MEM_Subtype);
>> + return 0;
>> +}
>> +
>> static inline bool
>> mem_has_msgpack_subtype(struct Mem *mem)
>> {
New patch:
commit 14bb43fcd1f34bef5c92279ae6c274d1e437cc86
Author: Mergen Imeev <imeevma at gmail.com>
Date: Tue Mar 23 00:53:03 2021 +0300
sql: introduce mem_copy()
This patch introduces mem_copy(). This function copies value from source
MEM to destination MEM. In case value is string or binary and have not
static allocation type, it is copied to newly allocated memory.
Part of #5818
diff --git a/src/box/sql/func.c b/src/box/sql/func.c
index a0108220f..0b85bf365 100644
--- a/src/box/sql/func.c
+++ b/src/box/sql/func.c
@@ -1769,13 +1769,13 @@ minmaxStep(sql_context * context, int NotUsed, sql_value ** argv)
bool is_max = (func->flags & SQL_FUNC_MAX) != 0;
cmp = sqlMemCompare(pBest, pArg, pColl);
if ((is_max && cmp < 0) || (!is_max && cmp > 0)) {
- sqlVdbeMemCopy(pBest, pArg);
+ mem_copy(pBest, pArg);
} else {
sqlSkipAccumulatorLoad(context);
}
} else {
pBest->db = sql_context_db_handle(context);
- sqlVdbeMemCopy(pBest, pArg);
+ mem_copy(pBest, pArg);
}
}
diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c
index 25b2e75ee..ea3917fe3 100644
--- a/src/box/sql/mem.c
+++ b/src/box/sql/mem.c
@@ -267,6 +267,35 @@ mem_destroy(struct Mem *mem)
mem->zMalloc = NULL;
}
+int
+mem_copy(struct Mem *to, const struct Mem *from)
+{
+ mem_clear(to);
+ to->u = from->u;
+ to->flags = from->flags;
+ to->subtype = from->subtype;
+ to->field_type = from->field_type;
+ to->n = from->n;
+ to->z = from->z;
+ if ((to->flags & (MEM_Str | MEM_Blob)) == 0)
+ return 0;
+ if ((to->flags & MEM_Static) != 0)
+ return 0;
+ if ((to->flags & (MEM_Zero | MEM_Blob)) == (MEM_Zero | MEM_Blob))
+ return sqlVdbeMemExpandBlob(to);
+ if (to->szMalloc == 0)
+ to->zMalloc = sqlDbMallocRaw(to->db, to->n);
+ else
+ to->zMalloc = sqlDbReallocOrFree(to->db, to->zMalloc, to->n);
+ if (to->zMalloc == NULL)
+ return -1;
+ to->szMalloc = sqlDbMallocSize(to->db, to->zMalloc);
+ memcpy(to->zMalloc, to->z, to->n);
+ to->z = to->zMalloc;
+ to->flags &= (MEM_Str | MEM_Blob | MEM_Term | MEM_Subtype);
+ return 0;
+}
+
static inline bool
mem_has_msgpack_subtype(struct Mem *mem)
{
@@ -1960,28 +1989,6 @@ vdbe_mem_alloc_blob_region(struct Mem *vdbe_mem, uint32_t size)
return 0;
}
-/*
- * Make a full copy of pFrom into pTo. Prior contents of pTo are
- * freed before the copy is made.
- */
-int
-sqlVdbeMemCopy(Mem * pTo, const Mem * pFrom)
-{
- int rc = 0;
-
- mem_clear(pTo);
- memcpy(pTo, pFrom, MEMCELLSIZE);
- pTo->flags &= ~MEM_Dyn;
- if (pTo->flags & (MEM_Str | MEM_Blob)) {
- if (0 == (pFrom->flags & MEM_Static)) {
- pTo->flags |= MEM_Ephem;
- rc = sqlVdbeMemMakeWriteable(pTo);
- }
- }
-
- return rc;
-}
-
void
sqlVdbeMemShallowCopy(Mem * pTo, const Mem * pFrom, int srcType)
{
diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h
index e4e586d4d..af36f31a2 100644
--- a/src/box/sql/mem.h
+++ b/src/box/sql/mem.h
@@ -168,6 +168,14 @@ mem_create(struct Mem *mem);
void
mem_destroy(struct Mem *mem);
+/**
+ * Copy content of MEM from one MEM to another. In case source MEM contains
+ * string or binary and allocation type is not STATIC, this value is copied to
+ * newly allocated by destination MEM memory.
+ */
+int
+mem_copy(struct Mem *to, const struct Mem *from);
+
/* One or more of the following flags are set to indicate the validOK
* representations of the value stored in the Mem struct.
*
@@ -467,7 +475,6 @@ mem_is_type_compatible(struct Mem *mem, enum field_type type);
int
vdbe_mem_alloc_blob_region(struct Mem *vdbe_mem, uint32_t size);
-int sqlVdbeMemCopy(Mem *, const Mem *);
void sqlVdbeMemShallowCopy(Mem *, const Mem *, int);
void sqlVdbeMemMove(Mem *, Mem *);
int sqlVdbeMemMakeWriteable(Mem *);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index f054a0f43..abd49b9bb 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1008,11 +1008,7 @@ case OP_Copy: {
pOut = &aMem[pOp->p2];
assert(pOut!=pIn1);
while( 1) {
- sqlVdbeMemShallowCopy(pOut, pIn1, MEM_Ephem);
- Deephemeralize(pOut);
-#ifdef SQL_DEBUG
- pOut->pScopyFrom = 0;
-#endif
+ mem_copy(pOut, pIn1);
REGISTER_TRACE(p, pOp->p2+pOp->p3-n, pOut);
if ((n--)==0) break;
pOut++;
diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c
index 2a3561d42..7951996ea 100644
--- a/src/box/sql/vdbeapi.c
+++ b/src/box/sql/vdbeapi.c
@@ -229,7 +229,7 @@ sql_result_text64(sql_context * pCtx,
void
sql_result_value(sql_context * pCtx, sql_value * pValue)
{
- sqlVdbeMemCopy(pCtx->pOut, pValue);
+ mem_copy(pCtx->pOut, pValue);
}
void
diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c
index 52e100454..bec8a532a 100644
--- a/src/box/sql/vdbeaux.c
+++ b/src/box/sql/vdbeaux.c
@@ -2333,7 +2333,7 @@ sqlVdbeGetBoundValue(Vdbe * v, int iVar, u8 aff)
if (!mem_is_null(pMem)) {
sql_value *pRet = sqlValueNew(v->db);
if (pRet) {
- sqlVdbeMemCopy((Mem *) pRet, pMem);
+ mem_copy(pRet, pMem);
sql_value_apply_type(pRet, aff);
}
return pRet;
diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index bb87bb902..91cba9962 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -415,8 +415,7 @@ stat4ValueFromExpr(Parse * pParse, /* Parse context */
if ((v = pParse->pReprepare) != 0) {
pVal = valueNew(db, pAlloc);
if (pVal) {
- rc = sqlVdbeMemCopy((Mem *) pVal,
- &v->aVar[iBindVar - 1]);
+ rc = mem_copy(pVal, &v->aVar[iBindVar - 1]);
if (rc == 0)
sql_value_apply_type(pVal, type);
pVal->db = pParse->db;
More information about the Tarantool-patches
mailing list