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 891F56EC5E; Fri, 9 Apr 2021 19:51:51 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 891F56EC5E DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1617987111; bh=20BYMT5FBv/g1dxNTZfrEeycxZxVs9Ma5UqjI9SecZM=; 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=MB/obzkriQuwJVJaYd+V+Zkfbp7plqDJtLz5sI7l9J2B+OXQUV5xw0Rf4jzlqIDRJ irVFZ/XNlFyf+53f5TOn+StAVrqZsifiBBA6KLPgr+ECfcfRzlIpJcjuhmc0kpI9CD RGx/6X2SbXH0JAw8wWo1lKeHA3hOhitnlg9jjZoU= Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 4512B6EC5E for ; Fri, 9 Apr 2021 19:51:23 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4512B6EC5E Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1lUuLq-0005yX-EU; Fri, 09 Apr 2021 19:51:22 +0300 To: v.shpilevoy@tarantool.org, tsafin@tarantool.org Cc: tarantool-patches@dev.tarantool.org Date: Fri, 9 Apr 2021 19:51:22 +0300 Message-Id: <3fda2029b186d0cea67ba01269dc3d6e209b3c89.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: 4F1203BC0FB41BD92FFCB8E6708E7480BE79914FF86F9151AC38CC435EA4A654182A05F5380850401BBA007E63F6FEB1CC6A169BB2CB12C726A11364EAAEE419DB7A12A6805A65E5 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE748069E1E80091F23EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006379F6495389D012EA98638F802B75D45FF914D58D5BE9E6BC1A93B80C6DEB9DEE97C6FB206A91F05B27291EC9A2853E637935B73D0850407DFEBA8952C4932A151D2E47CDBA5A96583C09775C1D3CA48CFCA5A41EBD8A3A0199FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C317B107DEF921CE79117882F4460429728AD0CFFFB425014E868A13BD56FB6657E2021AF6380DFAD18AA50765F790063735872C767BF85DA227C277FBC8AE2E8BDC0F6C5B2EEF3D0C75ECD9A6C639B01B4E70A05D1297E1BBCB5012B2E24CD356 X-C1DE0DAB: C20DE7B7AB408E4181F030C43753B8186998911F362727C414F749A5E30D975CD0035DD76F8A8A4FF3F47B6E66F81545B7668C64C79A860F9C2B6934AE262D3EE7EAB7254005DCED7532B743992DF240BDC6A1CF3F042BAD6DF99611D93F60EF0417BEADF48D1460699F904B3F4130E343918A1A30D5E7FCCB5012B2E24CD356 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34AA13E2DDB90678622A813845719812FD3309B2BAA6C48838E0075608574A68F176C561C407410BE11D7E09C32AA3244C793D39DDE02A8FB4E916ABEF3F77AD86408A6A02710B7304FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojbL9S8ysBdXj5Lj0u+HExrNGiEgrxx21X X-Mailru-Sender: 689FA8AB762F73936BC43F508A0638222B769ACB51A87AC9E8A7FA296BAADE1183D72C36FC87018B9F80AB2734326CD2FB559BB5D741EB96352A0ABBE4FDA4210A04DAD6CC59E33667EA787935ED9F1B X-Mras: Ok Subject: [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: imeevma@tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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. New patch: commit 3fda2029b186d0cea67ba01269dc3d6e209b3c89 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. 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++; } }