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 13F8C6EC5B; Wed, 14 Apr 2021 01:06:42 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 13F8C6EC5B DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1618351602; bh=77/SLZhiDtd9D9pth2X6uRvHLGQRPKo+YaKP4FE73To=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=l4/hphLQYQ3A0DGHT8aJx1hXBMK30zUJoacY96RsdBfOliX+xmVn2nnCOAOzQIfpB 9GLqMq2pw+quFY7a7inFfRUxkT3cFFvwbbLzxyUtKY3zj4xTC0khX1u8oqpyY5WuC/ Fdd4O4spLzJwySL5IzUi67WYE4L/YBcjdMxEbqd4= Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (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 565AD6EC5B for ; Wed, 14 Apr 2021 01:06:41 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 565AD6EC5B Received: by smtp38.i.mail.ru with esmtpa (envelope-from ) id 1lWRBA-00073z-DB; Wed, 14 Apr 2021 01:06:40 +0300 Date: Wed, 14 Apr 2021 01:06:39 +0300 To: Vladislav Shpilevoy Message-ID: <20210413220639.GA39558@tarantool.org> References: <70953d26-6661-8f6f-9172-c1f4430999d5@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <70953d26-6661-8f6f-9172-c1f4430999d5@tarantool.org> X-7564579A: 78E4E2B564C1792B X-77F55803: 4F1203BC0FB41BD92FFCB8E6708E7480EBD5CA77A668ECB87DA2124B0A8E6609182A05F5380850402D919D763C4EE593EFB25ACD67B37F5159F2F46EBA7D01F1B87D3688C47543CF X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE78C6616F30072131EEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006378D071468F2F69688EA1F7E6F0F101C67CDEEF6D7F21E0D1D9295C2E9FA3191EE1B59CA4C82EFA65824241253590D39C05EFC4FB59F6A7662F6B57BC7E64490618DEB871D839B73339E8FC8737B5C2249D082881546D93491CC7F00164DA146DAFE8445B8C89999729449624AB7ADAF37F6B57BC7E64490611E7FA7ABCAF51C92176DF2183F8FC7C0C26CFBAC0749D213D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE0A3850AC1BE2E7351C9461EB66F04EBFD8FC6C240DEA7642DBF02ECDB25306B2B78CF848AE20165D0A6AB1C7CE11FEE31F9513A7CA91E555302FCEF25BFAB345C4224003CC836476B096227BAE17F874E2021AF6380DFAD18AA50765F790063735872C767BF85DA227C277FBC8AE2E8B953A8A48A05D51F175ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A2368A440D3B0F6089093C9A16E5BC824A2A04A2ABAA09D2538DECB10C4E1BE3BCB8CCF60F6B7FD3948E1CD14B953EB46DD77DF86E79CFCEC1355D89D7DBCDD132 X-C1DE0DAB: 0D63561A33F958A5F97FA0D83F06FCC7391EDC216A71DE04C7EB95ED5B25C511D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34D041FB2E16F174C4151C6C54A300DB68DA96C46EB4C46812243BB3874ADE09C6FEC0AF2ED1CA0E601D7E09C32AA3244C6E9FD202030DA5838AD0E5180B2A8FCC795D98D676DD64D0FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojnA7/qPBUIXH1VpV+E3OQxQ== X-Mailru-Sender: 5C3750E245F362008BC1685FEC6306ED6D59CF1238F98B6AEFB25ACD67B37F5184876E3DC5E244DF5105BD0848736F9966FEC6BF5C9C28D97E07721503EA2E00ED97202A5A4E92BF7402F9BA4338D657ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v5 30/52] sql: introduce mem_copy_bin() 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: Mergen Imeev Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Thank you for the review! My answers and new patch below. Patch was a bit changed due to merge conflicts with new version of "sql: introduce mem_is_*() functions". On Tue, Apr 13, 2021 at 01:36:15AM +0200, Vladislav Shpilevoy wrote: > I appreciate the work you did here! > > See 2 comments below. > > > diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c > > index 2622cdd82..e30795de5 100644 > > --- a/src/box/sql/mem.c > > +++ b/src/box/sql/mem.c > > @@ -40,6 +40,19 @@ > > #include "lua/utils.h" > > #include "lua/msgpack.h" > > > > +/* > > + * Make sure pMem->z points to a writable allocation of at least > > + * min(n,32) bytes. > > + * > > + * If the bPreserve argument is true, then copy of the content of > > + * pMem->z into the new allocation. pMem must be either a string or > > + * blob if bPreserve is true. If bPreserve is false, any prior content > > + * in pMem->z is discarded. > > + */ > > +static int > > +sqlVdbeMemGrow(struct Mem *pMem, int n, int preserve); > > + > > + > > 1. Double empty line. > Fixed during resolving merge conflicts with new version of "sql: introduce mem_is_*() functions". > > bool > > mem_is_null(const struct Mem *mem) > > { > > @@ -477,6 +490,23 @@ mem_set_bin_allocated(struct Mem *mem, char *value, uint32_t size) > > set_bin_dynamic(mem, value, size, 0); > > } > > > > +int > > +mem_copy_bin(struct Mem *mem, const char *value, uint32_t size) > > +{ > > + if ((mem->flags & (MEM_Agg | MEM_Frame)) != 0) > > + mem_clear(mem); > > 2. The same comment as for copy_str. > Same as there, in case copied value is value of the MEM it copied to and its allocation type being MEM_Dyn, it will be lost during clear(). > > + bool is_own_value = (mem->flags & (MEM_Str | MEM_Blob)) != 0 && > > + mem->z == value; > > + if (sqlVdbeMemGrow(mem, size, is_own_value) != 0) > > + return -1; > > + if (!is_own_value) > > + memcpy(mem->z, value, size); > > + mem->n = size; > > + mem->flags = MEM_Blob; > > + mem->field_type = FIELD_TYPE_VARBINARY; > > + return 0; > > +} New patch: commit 8788956575ecf095d86d63de5eeea2e932fb0bac Author: Mergen Imeev Date: Tue Mar 16 11:55:05 2021 +0300 sql: introduce mem_copy_bin() This patch introduces mem_copy_bin() function. This function copies given binary value to a newly allocated by MEM memory. Part of #5818 diff --git a/src/box/sql/mem.c b/src/box/sql/mem.c index 1e7adfcb0..190311d80 100644 --- a/src/box/sql/mem.c +++ b/src/box/sql/mem.c @@ -40,6 +40,18 @@ #include "lua/utils.h" #include "lua/msgpack.h" +/* + * Make sure pMem->z points to a writable allocation of at least + * min(n,32) bytes. + * + * If the bPreserve argument is true, then copy of the content of + * pMem->z into the new allocation. pMem must be either a string or + * blob if bPreserve is true. If bPreserve is false, any prior content + * in pMem->z is discarded. + */ +static int +sqlVdbeMemGrow(struct Mem *pMem, int n, int preserve); + enum { BUF_SIZE = 32, }; @@ -335,6 +347,23 @@ mem_set_bin_allocated(struct Mem *mem, char *value, uint32_t size) set_bin_dynamic(mem, value, size, 0); } +int +mem_copy_bin(struct Mem *mem, const char *value, uint32_t size) +{ + if ((mem->flags & (MEM_Agg | MEM_Frame)) != 0) + mem_clear(mem); + bool is_own_value = (mem->flags & (MEM_Str | MEM_Blob)) != 0 && + mem->z == value; + if (sqlVdbeMemGrow(mem, size, is_own_value) != 0) + return -1; + if (!is_own_value) + memcpy(mem->z, value, size); + mem->n = size; + mem->flags = MEM_Blob; + mem->field_type = FIELD_TYPE_VARBINARY; + return 0; +} + int mem_copy(struct Mem *to, const struct Mem *from) { @@ -1869,17 +1898,8 @@ mem_convert_to_numeric(struct Mem *mem, enum field_type type) return mem_convert_to_integer(mem); } -/* - * Make sure pMem->z points to a writable allocation of at least - * min(n,32) bytes. - * - * If the bPreserve argument is true, then copy of the content of - * pMem->z into the new allocation. pMem must be either a string or - * blob if bPreserve is true. If bPreserve is false, any prior content - * in pMem->z is discarded. - */ -SQL_NOINLINE int -sqlVdbeMemGrow(Mem * pMem, int n, int bPreserve) +static int +sqlVdbeMemGrow(struct Mem *pMem, int n, int bPreserve) { assert(sqlVdbeCheckMemInvariants(pMem)); testcase(pMem->db == 0); diff --git a/src/box/sql/mem.h b/src/box/sql/mem.h index 617966cf7..197c16461 100644 --- a/src/box/sql/mem.h +++ b/src/box/sql/mem.h @@ -456,6 +456,13 @@ mem_set_bin_dynamic(struct Mem *mem, char *value, uint32_t size); void mem_set_bin_allocated(struct Mem *mem, char *value, uint32_t size); +/** + * Copy binary value to a newly allocated memory. The MEM type becomes + * VARBINARY. + */ +int +mem_copy_bin(struct Mem *mem, const char *value, uint32_t size); + /** * 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 @@ -699,7 +706,6 @@ mem_convert_to_numeric(struct Mem *mem, enum field_type type); /** Setters = Change MEM value. */ -int sqlVdbeMemGrow(struct Mem * pMem, int n, int preserve); int sqlVdbeMemClearAndResize(struct Mem * pMem, int n); /** diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c index cd7811846..d573e1e8f 100644 --- a/src/box/sql/vdbe.c +++ b/src/box/sql/vdbe.c @@ -2240,11 +2240,8 @@ case OP_MakeRecord: { * routine. */ if (bIsEphemeral) { - if (sqlVdbeMemClearAndResize(pOut, tuple_size) != 0) + if (mem_copy_bin(pOut, tuple, tuple_size) != 0) goto abort_due_to_error; - pOut->flags = MEM_Blob; - pOut->n = tuple_size; - memcpy(pOut->z, tuple, tuple_size); region_truncate(region, used); } else { /* Allocate memory on the region for the tuple @@ -2584,7 +2581,7 @@ case OP_SequenceTest: { * Open a new cursor that points to a fake table that contains a single * row of data. The content of that one row is the content of memory * register P2. In other words, cursor P1 becomes an alias for the - * MEM_Blob content contained in register P2. + * MEM with binary content contained in register P2. * * A pseudo-table created by this opcode is used to hold a single * row output from the sorter so that the row can be decomposed into diff --git a/src/box/sql/vdbeapi.c b/src/box/sql/vdbeapi.c index 7114f4351..7656d8d67 100644 --- a/src/box/sql/vdbeapi.c +++ b/src/box/sql/vdbeapi.c @@ -168,7 +168,7 @@ sql_result_blob(sql_context * pCtx, mem_set_bin_allocated(pCtx->pOut, (char *)z, n); else if (xDel != SQL_TRANSIENT) mem_set_bin_dynamic(pCtx->pOut, (char *)z, n); - else if (sqlVdbeMemSetStr(pCtx->pOut, z, n, 0, xDel) != 0) + else if (mem_copy_bin(pCtx->pOut, z, n) != 0) pCtx->is_aborted = true; } @@ -807,7 +807,7 @@ sql_bind_blob(sql_stmt * pStmt, mem_set_bin_allocated(var, (char *)zData, nData); else if (xDel != SQL_TRANSIENT) mem_set_bin_dynamic(var, (char *)zData, nData); - else if (sqlVdbeMemSetStr(var, zData, nData, 0, xDel) != 0) + else if (mem_copy_bin(var, zData, nData) != 0) return -1; return sql_bind_type(p, i, "varbinary"); } diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c index dad9cab03..ffc85b91d 100644 --- a/src/box/sql/vdbeaux.c +++ b/src/box/sql/vdbeaux.c @@ -1305,19 +1305,26 @@ sqlVdbeList(Vdbe * p) * has not already been seen. */ if (pOp->p4type == P4_SUBPROGRAM) { - int nByte = (nSub + 1) * sizeof(SubProgram *); int j; for (j = 0; j < nSub; j++) { if (apSub[j] == pOp->p4.pProgram) break; } - if (j == nSub && - sqlVdbeMemGrow(pSub, nByte, - nSub != 0) == 0) { - apSub = (SubProgram **) pSub->z; - apSub[nSub++] = pOp->p4.pProgram; - pSub->flags |= MEM_Blob; - pSub->n = nSub * sizeof(SubProgram *); + if (nSub == 0) { + uint32_t size = sizeof(SubProgram *); + char *bin = (char *)&pOp->p4.pProgram; + if (mem_copy_bin(pSub, bin, size) != 0) + return -1; + } else if (j == nSub) { + struct Mem tmp; + mem_create(&tmp); + uint32_t size = sizeof(SubProgram *); + char *bin = (char *)&pOp->p4.pProgram; + mem_set_bin_ephemeral(&tmp, bin, size); + int rc = mem_concat(pSub, &tmp, pSub); + mem_destroy(&tmp); + if (rc != 0) + return -1; } } } diff --git a/src/box/sql/vdbesort.c b/src/box/sql/vdbesort.c index a9a5f45af..3da425be4 100644 --- a/src/box/sql/vdbesort.c +++ b/src/box/sql/vdbesort.c @@ -2164,12 +2164,8 @@ sqlVdbeSorterRowkey(const VdbeCursor * pCsr, Mem * pOut) assert(pCsr->eCurType == CURTYPE_SORTER); pSorter = pCsr->uc.pSorter; pKey = vdbeSorterRowkey(pSorter, &nKey); - if (sqlVdbeMemClearAndResize(pOut, nKey)) { + if (mem_copy_bin(pOut, pKey, nKey) != 0) return -1; - } - pOut->n = nKey; - MemSetTypeFlag(pOut, MEM_Blob); - memcpy(pOut->z, pKey, nKey); return 0; }