From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 658012659E for ; Wed, 30 Jan 2019 08:04:35 -0500 (EST) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 5YdAJQoxEd2b for ; Wed, 30 Jan 2019 08:04:35 -0500 (EST) 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 turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id E9F9C25E94 for ; Wed, 30 Jan 2019 08:04:34 -0500 (EST) Subject: [tarantool-patches] Re: [PATCH 5/8] sql: replace field type with affinity for VDBE runtime References: <6e7623ee-fdcc-616f-6a99-20aff0e97f43@tarantool.org> <93178595-D7AF-49DE-8273-0FBD18545D56@tarantool.org> From: Vladislav Shpilevoy Message-ID: <115cb635-13f4-1946-22b3-36b828d9d679@tarantool.org> Date: Wed, 30 Jan 2019 16:04:33 +0300 MIME-Version: 1.0 In-Reply-To: <93178595-D7AF-49DE-8273-0FBD18545D56@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: "n.pettik" , tarantool-patches@freelists.org Thanks for the fixes! On 28/01/2019 19:39, n.pettik wrote: > >>>>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c >>>>> index 917e6e30b..c823c5a06 100644 >>>>> --- a/src/box/sql/expr.c >>>>> +++ b/src/box/sql/expr.c >>>>> @@ -2858,8 +2858,11 @@ sqlite3CodeSubselect(Parse * pParse, /* Parsing context */ >>>>> jmpIfDynamic = -1; >>>>> } >>>>> r3 = sqlite3ExprCodeTarget(pParse, pE2, r1); >>>>> + char type = >>>>> + sql_affinity_to_field_type(affinity); >>>>> sqlite3VdbeAddOp4(v, OP_MakeRecord, r3, >>>>> - 1, r2, &affinity, 1); >>>>> + 1, r2, &type, >>>>> + 1); >>>> >>>> 4. I do not understand. Is p4type of sqlite3VdbeAddOp4 of OP_MakeRecord >>>> a length of an affinity string, or a type of its allocation? Or both? >>> Both. p4type > 0 means that p4type bytes are copied to freshly allocated memory >>> and P4 set to DYNAMIC. sqlite3DbStrNDup() in vdbeChangeP4Full() reserves >>> one more byte for NULL termination. >> >> I see now, that OP_MakeRecord uses only p4.z and does not touch p4type, so it makes >> no sense to pass here length of the field_type array. Please, use only P4_STATIC and >> P4_DYNAMIC. At this moment this opcode is mess: somewhere 0 is passed, somewhere >> P4_DYNAMIC, in other places length of a field_type array. > > Well, actually we can’t pass here neither of these args and that’s why: > STATIC and DYNAMIC indicate that memory which is passed through > P4 argument will be copied to new chunk of memory: > > 985 void > 986 sqlite3VdbeChangeP4(Vdbe * p, int addr, const char *zP4, int n) > 987 { > > … > > 1008 if (n == P4_INT32) { > 1009 /* Note: this cast is safe, because the origin data point was an int > 1010 * that was cast to a (const char *). > 1011 */ > 1012 pOp->p4.i = SQLITE_PTR_TO_INT(zP4); > 1013 pOp->p4type = P4_INT32; > 1014 } if (n == P4_BOOL) { > 1015 pOp->p4.b = *(bool*)zP4; > 1016 pOp->p4type = P4_BOOL; > 1017 } else { > 1018 assert(n < 0); > 1019 pOp->p4.p = (void *)zP4; > 1020 pOp->p4type = (signed char)n; > 1021 } > 1022 } > > Both P4_STATIC and P4_DYNAMIC are less than 0, so pOp->p4.p points > to object on allocated on stack (in this particular case). > > Passing n > 0 we are implying that n bytes of memory starting from zP4 > must be copied to new chunk, which comes with P4_DYNAMIC > > If you still have doubts, I assure you that I’ve already tried to replace it and > always get assertion faults like: > Assertion failed: (f_type < field_type_MAX), function mem_apply_type, file tarantool/src/box/sql/vdbe.c, line 334. > > In other places (where we use P4_DYNAMIC), field_type array is allocated > on heap, so it can be freed after VDBE execution. Ok, now I got it. Thanks for the explanation. > > As an option we can allocate on heap this string before calling > sqlite3VdbeChangeP4, like this: > > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index ca39faf51..27bbc1b1c 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -2860,11 +2860,15 @@ sqlite3CodeSubselect(Parse * pParse, /* Parsing context */ > r3 = sqlite3ExprCodeTarget(pParse, pE2, r1); > enum field_type type = > sql_affinity_to_field_type(affinity); > - enum field_type types[2] = > - { type, field_type_MAX }; > + size_t sz = 2 * sizeof(enum field_type); > + enum field_type *types = > + sqlite3DbMallocRaw(pParse->db, > + sz); > + types[0] = type; > + types[1] = field_type_MAX; > sqlite3VdbeAddOp4(v, OP_MakeRecord, r3, > 1, r2, (char *)types, > - sizeof(types)); > + P4_DYNAMIC); No, never mind. The current way is ok. >>> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c >>> index 0a80ca622..ca39faf51 100644 >>> --- a/src/box/sql/expr.c >>> +++ b/src/box/sql/expr.c >>> @@ -3172,7 +3177,10 @@ sqlite3ExprCodeIN(Parse * pParse, /* Parsing and code generating context */ >>> * of the RHS using the LHS as a probe. If found, the result is >>> * true. >>> */ >>> - sqlite3VdbeAddOp4(v, OP_Affinity, rLhs, nVector, 0, zAff, nVector); >>> + enum field_type *types = sql_affinity_str_to_field_type_str(zAff); >>> + types[nVector] = field_type_MAX; >>> + sqlite3VdbeAddOp4(v, OP_ApplyType, rLhs, nVector, 0, (char *)types, >>> + P4_DYNAMIC); >> >> 2. Why do you need types[nVector] = field_type_MAX? As I see, before your patch >> there was no additional zero termination. > > To cut off array of types. Before my patch, first nVector bytes > were copied inside vdbeChangeP4Full() and automatically > terminated with NULL (sqlite3DbStrNDup). Still do not understand. As I see, zAff was taken from exprINAffinity(). This function allocates a string of affinities of length sqlite3ExprVectorSize(pExpr->pLeft). But nVector was initialized with exactly the same value. So strlen(zAff) should be equal to nVector, and your function sql_affinity_str_to_field_type_str() should return already correct array. I tried to test it and faced into a very strange situation. Firstly, it appeared that already at the beginning of the function, after this code: zAff = exprINAffinity(pParse, pExpr); nVector = sqlite3ExprVectorSize(pExpr->pLeft); They are not already equal. This assertion fails: strlen(zAff) == nVector. I tried to print their values in debug, and found, that zAff is stored in a buffer of length 2, but both bytes are zeros. Why? It is a first strange situation. Secondly, your function sql_affinity_str_to_field_type_str() in such a case returns a buffer of one byte - [field_type_MAX], because it uses strlen(affinity_str) + 1, which == 1 here. Further, you do types[nVector] = field_type_MAX, so you are out of array bounds, because nVector == 1, not 0. The only reason why it does not fail is that malloc returns more memory than necessary. Please, check out if I am right, and fix the bug if so. The assertion fails in sql-tap/randexpr1.test.lua. Also, see 2 comments below. > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index 58e35e0d9..d263c1bb5 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -2854,8 +2854,17 @@ sqlite3CodeSubselect(Parse * pParse, /* Parsing context */ > jmpIfDynamic = -1; > } > r3 = sqlite3ExprCodeTarget(pParse, pE2, r1); > + enum field_type type = > + sql_affinity_to_field_type(affinity); > + size_t sz = 2 * sizeof(enum field_type); > + enum field_type *types = > + sqlite3DbMallocRaw(pParse->db, > + sz); > + types[0] = type; > + types[1] = field_type_MAX; 1. Please, either check for malloc result, or just use the same method as in other places - declare it on the stack and pass sizeof(this_array) instead of P4_DYNAMIC. > sqlite3VdbeAddOp4(v, OP_MakeRecord, r3, > - 1, r2, &affinity, 1); > + 1, r2, (char *)types, > + P4_DYNAMIC); > sqlite3ExprCacheAffinityChange(pParse, > r3, 1); > sqlite3VdbeAddOp2(v, OP_IdxInsert, r2, > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index 24d992284..80d2fd0aa 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -2881,7 +2873,7 @@ case OP_MakeRecord: { > * of the record to data0. > */ > nField = pOp->p1; > - zAffinity = pOp->p4.z; > + enum field_type *types = (enum field_type *)pOp->p4.z; 2. Maybe, it is worth adding enum field_type *types into VdbeOp.p4union and do not cast. Like we did with many other pointers. > bIsEphemeral = pOp->p5; > assert(nField>0 && pOp->p2>0 && pOp->p2+nField<=(p->nMem+1 - p->nCursor)+1); > pData0 = &aMem[nField];