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 77E9A6EC5F; Fri, 9 Apr 2021 20:37:54 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 77E9A6EC5F DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1617989874; bh=JnB1Zbg52USKtMGAGkUqL9SvYWWXy7ovXq85it6rlro=; h=To:Cc:Date:In-Reply-To:References:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=mjG94z87i05AkeL6yRpdfSLx53s8SjuLw2vXDCDMz8YWYiFERmwy9J+Hxh+HweyHD qikh8rRGQjWYKR4weqxL4uFtdaeI3hM9w3J9K7Ki685ZRdtzA2Ea9SF6D7+yyesYBm dQjqxe4gMYqU6MxeR3FZmQZwu6lWoA4OQLqdpfKY= Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [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 696BA6EC61 for ; Fri, 9 Apr 2021 20:36:57 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 696BA6EC61 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1lUv3w-00078B-Hv; Fri, 09 Apr 2021 20:36:56 +0300 To: v.shpilevoy@tarantool.org, tsafin@tarantool.org Cc: tarantool-patches@dev.tarantool.org Date: Fri, 9 Apr 2021 20:36:56 +0300 Message-Id: <14bb43fcd1f34bef5c92279ae6c274d1e437cc86.1617984948.git.imeevma@gmail.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-7564579A: EEAE043A70213CC8 X-77F55803: 4F1203BC0FB41BD92FFCB8E6708E7480D608FE24BC85426BB1B55F651FED8C70182A05F538085040A48F5C696C2D8B4251E7ED185938C5346EC4B74B55F3F48321CCEC96FCB6C332 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7492D3E4238663367EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063795AFAF91F541EBCE8638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B2C6B47B7444BF57DCD7682A044DB2B0BD8CCF7685675CC1A7D2E47CDBA5A96583C09775C1D3CA48CFE478A468B35FE767117882F4460429724CE54428C33FAD30A8DF7F3B2552694AC26CFBAC0749D213D2E47CDBA5A9658378DA827A17800CE7A6779F98BF527B7A9FA2833FD35BB23DF004C906525384302BEBFE083D3B9BA73A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E735C6EABA9B74D0DA47B5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: C20DE7B7AB408E4181F030C43753B8186998911F362727C414F749A5E30D975CD0035DD76F8A8A4F8A0530FD1249C430F7D190B9D25A87DB9C2B6934AE262D3EE7EAB7254005DCED7532B743992DF240BDC6A1CF3F042BAD6DF99611D93F60EF0417BEADF48D1460699F904B3F4130E343918A1A30D5E7FCCB5012B2E24CD356 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D346B71C4B0698719D7F9134D434E53757879EB055CECF9334C34D0925136A4429427C21EFBB1E8CADC1D7E09C32AA3244C2E6B31BEF3AD246839AD29B1A1CBF72D60759606DA2E136AFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojyO2lHpuZu4RUg+CiULveKg== X-Mailru-Sender: 689FA8AB762F73936BC43F508A063822C0AF5FAF39EE42D9536DAA82D582657A83D72C36FC87018B9F80AB2734326CD2FB559BB5D741EB96352A0ABBE4FDA4210A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: [Tarantool-patches] [PATCH v5 13/52] sql: introduce mem_copy() 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: imeevma@tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 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;