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 EFB466ECC0; Thu, 9 Dec 2021 03:32:15 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org EFB466ECC0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1639009936; bh=rCZmjgg9vxeMnt3I0KaTWzqx8XaEAule0p/zQr26JrA=; h=Date:To:Cc:References:In-Reply-To:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From:Reply-To:From; b=CdhAW8hoN352m10OI35PiIfaIO8f7/NTl+zHw24yhe9HyaZWJwwtzarxUEYRctpaB e09dWW2oRkeJyVaT0NJbc1RBSG3m/BYLoKdT/XLmfXwnEnbvXGqdEocnay4y5zU+Zo OuZ+C6JHx9TiNpegcg+5gw5tBc8M9/fZJhUpweFo= Received: from smtpng1.i.mail.ru (smtpng1.i.mail.ru [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 3DA856ECC0 for ; Thu, 9 Dec 2021 03:31:41 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3DA856ECC0 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1mv7LY-0002kO-H2; Thu, 09 Dec 2021 03:31:40 +0300 Message-ID: <9b75890d-32e4-aaa9-c377-01854278bc76@tarantool.org> Date: Thu, 9 Dec 2021 01:31:39 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.3.2 Content-Language: en-US To: Mergen Imeev Cc: tarantool-patches@dev.tarantool.org References: <4ecfb3439688bef76c96270624410dee8822176f.1637244389.git.imeevma@gmail.com> <662f6b87-f085-ab10-53b8-d087d9598b19@tarantool.org> <20211125085529.GB56448@tarantool.org> <20211202083851.GB8207@tarantool.org> In-Reply-To: <20211202083851.GB8207@tarantool.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD99F281DB1F96F126DEB009BA0867D17CA9D5BE74F09717CA300894C459B0CD1B98EB648B51579E38781043D646D964098DC5E47865CB58546F600E77B63AAB791 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7E5D7EAC6EBA58433EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637334E2757C55E8D4EEA1F7E6F0F101C6723150C8DA25C47586E58E00D9D99D84E1BDDB23E98D2D38BBCA57AF85F7723F2D8A1C662A9C986ACE6A38637FCDA9338CC7F00164DA146DAFE8445B8C89999728AA50765F79006375FFD5C25497261569FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C3A7AC412AE061D850117882F4460429728AD0CFFFB425014E868A13BD56FB6657E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F79006377AA2284B41911753EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A5982E46E4B344C78A888783319243CFDBF1BF1E729BEB3B10D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7506FE1F977233B9BB410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D3468964757141B82E05E1FA832A4F52AA61C650B122EF5EC1DC9671A28BE0163AE124BB1BF431714921D7E09C32AA3244CB41A5E38CC1B02720CA75B2C1407B56A97FE24653F78E668729B2BEF169E0186 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioja3eBVivI1t29lEwpi6Zdrw== X-Mailru-Sender: 689FA8AB762F7393C37E3C1AEC41BA5DB1FD4BE756E4A5563AE60DB27DCF4D083841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E25FEEDEB644C299C0ED14614B50AE0675 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v1 2/2] sql: introduce syntax for MAP values 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: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Thanks for the fixes! >>>>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c >>>>> index 74a98c550..789d8906c 100644 >>>>> --- a/src/box/sql/expr.c >>>>> +++ b/src/box/sql/expr.c >>>>> @@ -3432,6 +3432,35 @@ expr_code_array(struct Parse *parser, struct Expr *expr, int reg) >>>>> sqlVdbeAddOp3(vdbe, OP_Array, count, reg, values_reg); >>>>> } >>>>> >>>>> +static void >>>>> +expr_code_map(struct Parse *parser, struct Expr *expr, int reg) >>>> >>>> 1. I thought the policy was that we name functions, generating VDBE code, >>>> using 'emit' suffix. For instance, `vdbe_emit_map()` or `sql_emit_map()`. >>>> Don't know about prefix though. I see both vdbe_ and sql_ are used. >>>> >>> This is usually true, but this function is actually part of sqlExprCodeTarget(). >>> I believe these functions were created to make sqlExprCodeTarget() more >>> readable. All such functions are named sqlExprCode*(), code*() or >>> expr_code _*(), for example: sqlExprCodeGetColumn(), codeReal(), >>> expr_code_int(). >>> >>> Since all these functions are static, I think we should drop "expr_" prefix for >>> them. Not in this patch, though. >> >> If functions take Expr as an argument like these do, they could be >> considered methods of Expr. In that case dropping the expr_ prefix would >> violate our naming convention. It is not about static or global here. >> >> As an alternative they could be considered as methods of Parse, but >> then they would need to have parse_ prefix. >> >> For 'code' vs 'emit' - 'code' is fine by me as long as it is static. But >> if it goes public, then either 'code' or 'emit' must be chosen as one >> correct suffix. Not a mix. >> > After some thought, I think you are right. However, I would suggest removing the > parser and vdbe from these functions and converting them to proper struct expr > methods. This way we can make these functions return a value (most likely as an > "out" argument). For example expr_code_dec() should give us DECIMAL. In this > case we can make some improvements, for example we can remove "is_neg" from > expr_code_int() and turn it into expr_code_uint(), since we know that this '-' > sign will be specified as another expr. Also, since these will be valid expr > methods, we can drop "static" from their definition. We then should name them > accordingly, for example "expr_code_dec" may be named "expr_to_dec". AFAIU, their value is not only in converting the value, but also in doing the sqlVdbeAddOp action. You will need to duplicate this call in all places where expr_to_dec() would be used. The aspiration for refactoring this code is righteous anyway though. > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 55e494332..86de3f98a 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -1438,6 +1438,26 @@ case OP_Array: { > break; > } > > +/** > + * Opcode: Map P1 P2 P3 * * > + * Synopsis: r[P2] = map(P3@P1) > + * > + * Construct an MAP value from P1 registers starting at reg(P3). > + */ > +case OP_Map: { > + pOut = &aMem[pOp->p2]; > + > + uint32_t size; > + struct region *region = &fiber()->gc; > + size_t svp = region_used(region); > + char *val = mem_encode_map(&aMem[pOp->p3], pOp->p1, &size, region); > + if (val == NULL || mem_copy_map(pOut, val, size) != 0) { > + region_truncate(region, svp); > + goto abort_due_to_error; > + } You should probably truncate the region regardless of the result. Otherwise in case of success you will leak the region inside of the query bit by bit while SELECT works: box.execute('CREATE TABLE test (id INTEGER PRIMARY KEY)') box.execute('INSERT INTO test VALUES (1), (2), (3)') box.execute('SELECT {id: id} FROM test') Here you will do OP_Map 3 times, all will leave some region memory leaked every time. It is freed in the end of execution probably, but it might do some big usage while the request is in progress when the row count is much bigger than 3. Btw, worth adding a multirow test. All current map tests select a single row.