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 BBD9E6BD2D; Tue, 13 Apr 2021 15:12:41 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BBD9E6BD2D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1618315961; bh=IY3C7eSs+v6pJWE9+dYMX15RWw/Gt52XtYfMn3Ih7cI=; 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=eDHjDn3XwN7IW75IuEuPE4f4WXygFaxv+HrUoruSUJ/grnUuIfaE5lk06HNuiUWM4 oFJL3U0FGMVbFEqdcJ3nrSU9TBHyybW/lcaPC/CASjEhaDFX7Cth+lQ0BtvnIa0jYA tEXAfhfx6viuioFIYNbndTyGfr4ob8eQZmkntTX0= 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 A9C556BD29 for ; Tue, 13 Apr 2021 15:12:39 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A9C556BD29 Received: by smtpng1.m.smailru.net with esmtpa (envelope-from ) id 1lWHuI-0000Wf-PK; Tue, 13 Apr 2021 15:12:39 +0300 Date: Tue, 13 Apr 2021 15:12:37 +0300 To: Vladislav Shpilevoy Message-ID: <20210413121237.GA95794@tarantool.org> References: <3fda2029b186d0cea67ba01269dc3d6e209b3c89.1617984948.git.imeevma@gmail.com> <57dd6994-64f3-7645-f0ec-376751bd0b3e@tarantool.org> <20210413120116.GA95443@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20210413120116.GA95443@tarantool.org> X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD92FFCB8E6708E7480257C85EA0BB7A95D0F00AE41BB9A5343182A05F538085040F60104C13A156707D369553E9E01897D3687AEEE6DD8FFA7CB3E1B1F3D785FF1 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7F10646C0E1989908EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006374E88016F1B7D8D248638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B26D40970404F1104663A232EED75ADA45088CACE0DB6A842FD2E47CDBA5A96583C09775C1D3CA48CFCA5A41EBD8A3A0199FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C3CA5A41EBD8A3A0199FA2833FD35BB23DF004C906525384302BEBFE083D3B9BA73A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E7355E1C53F199C2BB95B5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A5537A3AB0D10C11F5C31FD01083CAFFB1AAD22FB962732FACD59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D347324AA9FA07FF01E1F5FFEFC5BC61E8DA893AA16C40F87B644CC25D8C52736B72F90AB587091D9F21D7E09C32AA3244CC31C67682099E78F4D4551847939F1D255E75C8D0ED9F6EEFACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojnA7/qPBUIXHr2fHrcCNKAA== X-Mailru-Sender: 689FA8AB762F73936BC43F508A0638226D2982268FAAD72E6CB1AB5F5776774B83D72C36FC87018B9F80AB2734326CD2FB559BB5D741EB96352A0ABBE4FDA4210A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH v5 01/52] sql: enhance vdbe_decode_msgpack_into_mem() 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" Sorry, I forgot to add name of the new branch and link to issues that will be closed by this patch. Here they are: https://github.com/tarantool/tarantool/tree/imeevma/gh-5818-encapsulate-mem-type-checking-and-changing-reviewed https://github.com/tarantool/tarantool/issues/5011 https://github.com/tarantool/tarantool/issues/5704 https://github.com/tarantool/tarantool/issues/5913 On Tue, Apr 13, 2021 at 03:01:18PM +0300, Mergen Imeev wrote: > Thank you for the review! I'a sorry to ask you so late, but could you look at > new version of the patch? It was decided that this patch should be pushed to > 2.6 and 2.7 along with master. Only this patch of the patch-set. > > Diff and new version below. Please note, that I added additional SELECT to > the first test since currently sql-tap test system does not support binary > result. Also, in the last two tests I created new table for each since this way > we could see proper error. If I used space T there then in the last test there > would be the same error as in the test 5 due to UUID being before DECIMAL in > space format. To remove dependence of their position, I added two new spaces. > > On Sun, Apr 11, 2021 at 07:42:22PM +0200, Vladislav Shpilevoy wrote: > > Hi! Good job on the patch! > > > > On 09.04.2021 18:51, Mergen Imeev via Tarantool-patches wrote: > > > Hi! Thank you for the review! My answer and new patch below. I didn't include > > > diffs in answers since due to merge conflicts they are partly useless. > > > > > > On 30.03.2021 01:57, Vladislav Shpilevoy wrote: > > >> Hi! I appreciate the work you did here! > > >> > > >> Truly huge patch. But it does the important thing which I think > > >> should give a huge help in the task of SQL code rework. > > >> > > >> On 23.03.2021 10:34, Mergen Imeev via Tarantool-patches wrote: > > >>> Currently, vdbe_decode_msgpack_into_mem() creates a MEM that is not > > >>> properly initialized in case msgpack contains MP_EXT, MP_MAP, or > > >>> MP_ARRAY fields. Also, it doesn't set field_type. > > >> > > >> AFAIR, field type wasn't set deliberately. Because we use it only for > > >> strictly typed values obtained from formatted space fields. It wasn't > > >> applied to plain non-formatted values on purpose. > > >> > > >> Why do you change that? > > > > > > I didn't know about that. I thought that all MEMs that contains data should have > > > field_type set. However, I tried to understand where did this field come from > > > and found that it was added for two purposes: to show difference between NUMBER > > > and INTEGER in MEM before DOUBLE was added and to show column name instead of > > > type determined from MP-type in typeof(). I believe that both these purposes are > > > not needed now and that this field should be removed from struct MEM. I created > > > an issue for this: #5957. However, I was prohibited to remove this field for now > > > by @tsafin, who believes that this field is actually important. > > > > I tend to agree, that it must be done separately. Although probably could be done > > before this patchset to make it kind of simpler. Anyway, thanks for the > > explanation. > > > Diff: > > > diff --git a/test/sql-tap/gh-5913-segfault-on-select-uuid.test.lua b/test/sql-tap/gh-5913-segfault-on-select-uuid.test.lua > new file mode 100755 > index 000000000..60978c2b5 > --- /dev/null > +++ b/test/sql-tap/gh-5913-segfault-on-select-uuid.test.lua > @@ -0,0 +1,99 @@ > +#!/usr/bin/env tarantool > +local test = require("sqltester") > +test:plan(6) > + > +local uuid = require("uuid").fromstr("11111111-1111-1111-1111-111111111111") > +local decimal = require("decimal").new(111.111) > + > +box.schema.create_space('T') > +box.space.T:format({{name = "I", type = "integer"}, {name = "U", type = "uuid"}, > + {name = "D", type = "decimal"}}) > +box.space.T:create_index("primary") > +box.space.T:insert({1, uuid, decimal}) > + > +-- > +-- Make sure that there is no segmentation fault on select from field that > +-- contains UUID or DECIMAL. Currently SQL does not support UUID and DECIMAL, > +-- so they treated as VARBINARY. > +-- > +test:do_execsql_test( > + "gh-5913-1", > + [[ > + SELECT i, u, d FROM t; > + SELECT i from t; > + ]], { > + 1 > + }) > + > +box.schema.create_space('T1') > +box.space.T1:format({{name = "I", type = "integer"}, > + {name = "U", type = "uuid", is_nullable = true}, > + {name = "D", type = "decimal", is_nullable = true}}) > +box.space.T1:create_index("primary") > + > +-- > +-- Since SQL does not support UUID and DECIMAL and they treated as VARBINARY, > +-- they cannot be inserted from SQL. > +-- > +test:do_catchsql_test( > + "gh-5913-2", > + [[ > + INSERT INTO t1 SELECT i, u, NULL FROM t; > + ]], { > + 1, "Type mismatch: can not convert varbinary to uuid" > + }) > + > +test:do_catchsql_test( > + "gh-5913-3", > + [[ > + INSERT INTO t1 SELECT i, NULL, d FROM t; > + ]], { > + 1, "Type mismatch: can not convert varbinary to decimal" > + }) > + > +-- > +-- Still, if UUID or DECIMAL fields does not selected directly, insert is > +-- working properly. > +-- > +test:do_execsql_test( > + "gh-5913-4", > + [[ > + INSERT INTO t1 SELECT * FROM t; > + SELECT count() FROM t1; > + ]], { > + 1 > + }) > + > +box.schema.create_space('TU') > +box.space.TU:format({{name = "I", type = "integer"}, > + {name = "U", type = "uuid"}}) > +box.space.TU:create_index("primary") > +box.space.TU:insert({1, uuid}) > + > +box.schema.create_space('TD') > +box.space.TD:format({{name = "I", type = "integer"}, > + {name = "D", type = "decimal"}}) > +box.space.TD:create_index("primary") > +box.space.TD:insert({1, decimal}) > + > +-- > +-- Update of UUID or VARBINARY also does not lead to segfault, however throws an > +-- error since after changing value cannot be inserted into the field from SQL. > +-- > +test:do_catchsql_test( > + "gh-5913-5", > + [[ > + UPDATE tu SET u = u; > + ]], { > + 1, "Type mismatch: can not convert varbinary to uuid" > + }) > + > +test:do_catchsql_test( > + "gh-5913-6", > + [[ > + UPDATE td SET d = d; > + ]], { > + 1, "Type mismatch: can not convert varbinary to decimal" > + }) > + > +test:finish_test() > > > > New patch: > > > commit d41d8f756f8ab6bfae70ee5787bc4c509ddb844d > Author: Mergen Imeev > Date: Thu Mar 4 17:17:18 2021 +0300 > > sql: enhance vdbe_decode_msgpack_into_mem() > > Currently, vdbe_decode_msgpack_into_mem() creates a MEM that is not > properly initialized in case msgpack contains MP_EXT, MP_MAP, or > MP_ARRAY fields. This patch makes it so that after execution of this > function, all created MEMs are properly initialized. > > Closes #5011 > Closes #5704 > Closes #5913 > Needed for #5818 > > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 3b3b1f01d..9a4f38bb9 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -846,16 +846,6 @@ vdbe_field_ref_fetch_data(struct vdbe_field_ref *field_ref, uint32_t fieldno) > return field_begin; > } > > -static inline enum field_type > -vdbe_field_ref_fetch_type(struct vdbe_field_ref *field_ref, uint32_t fieldno) > -{ > - const struct tuple_field *tf = > - vdbe_field_ref_fetch_field(field_ref, fieldno); > - if (tf == NULL || tf->type == FIELD_TYPE_ANY) > - return field_type_MAX; > - return tf->type; > -} > - > /** > * Fetch field by fieldno using vdbe_field_ref and store result > * in dest_mem. > @@ -879,17 +869,6 @@ vdbe_field_ref_fetch(struct vdbe_field_ref *field_ref, uint32_t fieldno, > if (vdbe_decode_msgpack_into_mem(data, dest_mem, &dummy) != 0) > return -1; > > - /* > - * MsgPack map, array or extension (unsupported in sql). > - * Wrap it in a blob verbatim. > - */ > - if (dest_mem->flags == 0) { > - dest_mem->z = (char *) data; > - dest_mem->n = vdbe_field_ref_fetch_data(field_ref, > - fieldno + 1) - data; > - dest_mem->flags = MEM_Blob | MEM_Ephem | MEM_Subtype; > - dest_mem->subtype = SQL_SUBTYPE_MSGPACK; > - } > /* > * Add 0 termination (at most for strings) > * Not sure why do we check MEM_Ephem > @@ -909,7 +888,6 @@ vdbe_field_ref_fetch(struct vdbe_field_ref *field_ref, uint32_t fieldno, > dest_mem->flags |= MEM_Term; > } > UPDATE_MAX_BLOBSIZE(dest_mem); > - dest_mem->field_type = vdbe_field_ref_fetch_type(field_ref, fieldno); > return 0; > } > > diff --git a/src/box/sql/vdbeaux.c b/src/box/sql/vdbeaux.c > index 91b64316e..772476377 100644 > --- a/src/box/sql/vdbeaux.c > +++ b/src/box/sql/vdbeaux.c > @@ -2793,38 +2793,62 @@ vdbe_decode_msgpack_into_mem(const char *buf, struct Mem *mem, uint32_t *len) > { > const char *start_buf = buf; > switch (mp_typeof(*buf)) { > - case MP_ARRAY: > - case MP_MAP: > - case MP_EXT: > - default: { > - mem->flags = 0; > + case MP_ARRAY: { > + mem->z = (char *)buf; > + mp_next(&buf); > + mem->n = buf - mem->z; > + mem->flags = MEM_Blob | MEM_Ephem | MEM_Subtype; > + mem->subtype = SQL_SUBTYPE_MSGPACK; > + mem->field_type = FIELD_TYPE_ARRAY; > + break; > + } > + case MP_MAP: { > + mem->z = (char *)buf; > + mp_next(&buf); > + mem->n = buf - mem->z; > + mem->flags = MEM_Blob | MEM_Ephem | MEM_Subtype; > + mem->subtype = SQL_SUBTYPE_MSGPACK; > + mem->field_type = FIELD_TYPE_MAP; > + break; > + } > + case MP_EXT: { > + mem->z = (char *)buf; > + mp_next(&buf); > + mem->n = buf - mem->z; > + mem->flags = MEM_Blob | MEM_Ephem; > + mem->field_type = FIELD_TYPE_VARBINARY; > break; > } > case MP_NIL: { > mp_decode_nil(&buf); > mem->flags = MEM_Null; > + mem->field_type = field_type_MAX; > break; > } > case MP_BOOL: { > mem->u.b = mp_decode_bool(&buf); > mem->flags = MEM_Bool; > + mem->field_type = FIELD_TYPE_BOOLEAN; > break; > } > case MP_UINT: { > uint64_t v = mp_decode_uint(&buf); > mem->u.u = v; > mem->flags = MEM_UInt; > + mem->field_type = FIELD_TYPE_INTEGER; > break; > } > case MP_INT: { > mem->u.i = mp_decode_int(&buf); > mem->flags = MEM_Int; > + mem->field_type = FIELD_TYPE_INTEGER; > break; > } > case MP_STR: { > /* XXX u32->int */ > mem->n = (int) mp_decode_strl(&buf); > mem->flags = MEM_Str | MEM_Ephem; > + mem->field_type = FIELD_TYPE_STRING; > install_blob: > mem->z = (char *)buf; > buf += mem->n; > @@ -2834,18 +2858,33 @@ install_blob: > /* XXX u32->int */ > mem->n = (int) mp_decode_binl(&buf); > mem->flags = MEM_Blob | MEM_Ephem; > + mem->field_type = FIELD_TYPE_VARBINARY; > goto install_blob; > } > case MP_FLOAT: { > mem->u.r = mp_decode_float(&buf); > - mem->flags = sqlIsNaN(mem->u.r) ? MEM_Null : MEM_Real; > + if (sqlIsNaN(mem->u.r)) { > + mem->flags = MEM_Null; > + mem->field_type = FIELD_TYPE_DOUBLE; > + } else { > + mem->flags = MEM_Real; > + mem->field_type = FIELD_TYPE_DOUBLE; > + } > break; > } > case MP_DOUBLE: { > mem->u.r = mp_decode_double(&buf); > - mem->flags = sqlIsNaN(mem->u.r) ? MEM_Null : MEM_Real; > + if (sqlIsNaN(mem->u.r)) { > + mem->flags = MEM_Null; > + mem->field_type = FIELD_TYPE_DOUBLE; > + } else { > + mem->flags = MEM_Real; > + mem->field_type = FIELD_TYPE_DOUBLE; > + } > break; > } > + default: > + unreachable(); > } > *len = (uint32_t)(buf - start_buf); > return 0; > @@ -2868,15 +2907,8 @@ sqlVdbeRecordUnpackMsgpack(struct key_def *key_def, /* Information about the rec > pMem->z = 0; > uint32_t sz = 0; > vdbe_decode_msgpack_into_mem(zParse, pMem, &sz); > - if (sz == 0) { > - /* MsgPack array, map or ext. Treat as blob. */ > - pMem->z = (char *)zParse; > - mp_next(&zParse); > - pMem->n = zParse - pMem->z; > - pMem->flags = MEM_Blob | MEM_Ephem; > - } else { > - zParse += sz; > - } > + assert(sz != 0); > + zParse += sz; > pMem++; > } > } > diff --git a/test/sql-tap/gh-5913-segfault-on-select-uuid.test.lua b/test/sql-tap/gh-5913-segfault-on-select-uuid.test.lua > new file mode 100755 > index 000000000..60978c2b5 > --- /dev/null > +++ b/test/sql-tap/gh-5913-segfault-on-select-uuid.test.lua > @@ -0,0 +1,99 @@ > +#!/usr/bin/env tarantool > +local test = require("sqltester") > +test:plan(6) > + > +local uuid = require("uuid").fromstr("11111111-1111-1111-1111-111111111111") > +local decimal = require("decimal").new(111.111) > + > +box.schema.create_space('T') > +box.space.T:format({{name = "I", type = "integer"}, {name = "U", type = "uuid"}, > + {name = "D", type = "decimal"}}) > +box.space.T:create_index("primary") > +box.space.T:insert({1, uuid, decimal}) > + > +-- > +-- Make sure that there is no segmentation fault on select from field that > +-- contains UUID or DECIMAL. Currently SQL does not support UUID and DECIMAL, > +-- so they treated as VARBINARY. > +-- > +test:do_execsql_test( > + "gh-5913-1", > + [[ > + SELECT i, u, d FROM t; > + SELECT i from t; > + ]], { > + 1 > + }) > + > +box.schema.create_space('T1') > +box.space.T1:format({{name = "I", type = "integer"}, > + {name = "U", type = "uuid", is_nullable = true}, > + {name = "D", type = "decimal", is_nullable = true}}) > +box.space.T1:create_index("primary") > + > +-- > +-- Since SQL does not support UUID and DECIMAL and they treated as VARBINARY, > +-- they cannot be inserted from SQL. > +-- > +test:do_catchsql_test( > + "gh-5913-2", > + [[ > + INSERT INTO t1 SELECT i, u, NULL FROM t; > + ]], { > + 1, "Type mismatch: can not convert varbinary to uuid" > + }) > + > +test:do_catchsql_test( > + "gh-5913-3", > + [[ > + INSERT INTO t1 SELECT i, NULL, d FROM t; > + ]], { > + 1, "Type mismatch: can not convert varbinary to decimal" > + }) > + > +-- > +-- Still, if UUID or DECIMAL fields does not selected directly, insert is > +-- working properly. > +-- > +test:do_execsql_test( > + "gh-5913-4", > + [[ > + INSERT INTO t1 SELECT * FROM t; > + SELECT count() FROM t1; > + ]], { > + 1 > + }) > + > +box.schema.create_space('TU') > +box.space.TU:format({{name = "I", type = "integer"}, > + {name = "U", type = "uuid"}}) > +box.space.TU:create_index("primary") > +box.space.TU:insert({1, uuid}) > + > +box.schema.create_space('TD') > +box.space.TD:format({{name = "I", type = "integer"}, > + {name = "D", type = "decimal"}}) > +box.space.TD:create_index("primary") > +box.space.TD:insert({1, decimal}) > + > +-- > +-- Update of UUID or VARBINARY also does not lead to segfault, however throws an > +-- error since after changing value cannot be inserted into the field from SQL. > +-- > +test:do_catchsql_test( > + "gh-5913-5", > + [[ > + UPDATE tu SET u = u; > + ]], { > + 1, "Type mismatch: can not convert varbinary to uuid" > + }) > + > +test:do_catchsql_test( > + "gh-5913-6", > + [[ > + UPDATE td SET d = d; > + ]], { > + 1, "Type mismatch: can not convert varbinary to decimal" > + }) > + > +test:finish_test()