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 C256A228DF for ; Fri, 1 Feb 2019 11:39:03 -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 tCjzibZKp6EV for ; Fri, 1 Feb 2019 11:39:03 -0500 (EST) 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 turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id E0EB927012 for ; Fri, 1 Feb 2019 11:39:02 -0500 (EST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH 5/8] sql: replace field type with affinity for VDBE runtime From: "n.pettik" In-Reply-To: <115cb635-13f4-1946-22b3-36b828d9d679@tarantool.org> Date: Fri, 1 Feb 2019 19:39:00 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <2ED4082B-7899-4880-8FF2-25E1A3E6D90B@tarantool.org> References: <6e7623ee-fdcc-616f-6a99-20aff0e97f43@tarantool.org> <93178595-D7AF-49DE-8273-0FBD18545D56@tarantool.org> <115cb635-13f4-1946-22b3-36b828d9d679@tarantool.org> 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: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy >>>> 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 =3D = sql_affinity_str_to_field_type_str(zAff); >>>> + types[nVector] =3D field_type_MAX; >>>> + sqlite3VdbeAddOp4(v, OP_ApplyType, rLhs, nVector, 0, (char = *)types, >>>> + P4_DYNAMIC); >>>=20 >>> 2. Why do you need types[nVector] =3D 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). >=20 > 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. >=20 > Firstly, it appeared that already at the beginning of the > function, after this code: >=20 > zAff =3D exprINAffinity(pParse, pExpr); > nVector =3D sqlite3ExprVectorSize(pExpr->pLeft); >=20 > They are not already equal. This assertion fails: >=20 > strlen(zAff) =3D=3D nVector. >=20 > 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. In a nutshell: owing to the fact that AFFINITY_UNDEFINED equals to 0. Long story: Consider simple example: SELECT NULL IN (2, 3, 4, NULL) When we reach sqlite3ExprCodeIN(), pExpr represents AST with the root in = IN operator: (lldb) print (int)pExpr->op (int) $4 =3D 12 #define TK_IN 12 LHS of the root is NULL (terminal symbol): (lldb) print *pExpr->pLeft (Expr) $6 =3D { op =3D 'K' =3D (affinity =3D AFFINITY_UNDEFINED, on_conflict_action =3D = ON_CONFLICT_ACTION_NONE) flags =3D 8388612 u =3D (zToken =3D "NULL", iValue =3D 59018336) So nVector is 1. Moreover, NULL's affinity is UNDEFINED. That is due to assigment to all expression by default UNDEFINED affinity (as you may remember, I fixed it in the next patch by setting it to SCALAR): 811 /* Construct a new Expr object from a single identifier. Use the 812 ** new Expr to populate pOut. Set the span of pOut to be the = identifier 813 ** that created the expression. 814 */ 815 static void spanExpr(ExprSpan *pOut, Parse *pParse, int op, Token = t){ 816 Expr *p =3D sqlite3DbMallocRawNN(pParse->db, sizeof(Expr)+t.n+1); 817 if( p ){ 818 memset(p, 0, sizeof(Expr)); 819 switch (op) { 820 case TK_STRING: 821 p->affinity =3D AFFINITY_TEXT; 822 break; 823 case TK_BLOB: 824 p->affinity =3D AFFINITY_BLOB; Since NULL token is out of TK_STRING, TK_BLOB etc enumeration, its affinity is 0, i.e. UNDERFINED. Therefore, we see that nVector is 1, so zAff is of lenght 2 (because of additional null-termination), and finally, sql_affinity_str_to_field_type_str() returns shorter array, then is should be. Fortunately, it doesn=E2=80=99t affect further patches, since there we get rid of affinity, and SCALAR in enum is not 0. > 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 =3D=3D 1 here. >=20 > Further, you do types[nVector] =3D field_type_MAX, so you are > out of array bounds, because nVector =3D=3D 1, not 0. The only > reason why it does not fail is that malloc returns more > memory than necessary. >=20 > Please, check out if I am right, and fix the bug if so. >=20 > The assertion fails in sql-tap/randexpr1.test.lua. As a workaround (this problem is automatically resolved in next patch) I suggest to pass to sql_affinity_str_to.. len of affinity array: diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 514d0ca9d..3adc6e262 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -521,17 +521,16 @@ sql_field_type_to_affinity(enum field_type = field_type) } =20 enum field_type * -sql_affinity_str_to_field_type_str(const char *affinity_str) +sql_affinity_str_to_field_type_str(const char *affinity_str, uint32_t = len) { if (affinity_str =3D=3D NULL) return NULL; - size_t len =3D strlen(affinity_str) + 1; - size_t sz =3D len * sizeof(enum field_type); + size_t sz =3D (len + 1) * sizeof(enum field_type); enum field_type *types =3D (enum field_type *) sqlite3DbMallocRaw(sql_get(), sz); - for (uint32_t i =3D 0; i < len - 1; ++i) + for (uint32_t i =3D 0; i < len; ++i) types[i] =3D = sql_affinity_to_field_type(affinity_str[i]); - types[len - 1] =3D field_type_MAX; + types[len] =3D field_type_MAX; return types; } =20 diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index d263c1bb5..b02089bc2 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -3177,8 +3177,8 @@ sqlite3ExprCodeIN(Parse * pParse, /* Parsing and = code generating context */ * of the RHS using the LHS as a probe. If found, the result is * true. */ - enum field_type *types =3D = sql_affinity_str_to_field_type_str(zAff); - types[nVector] =3D field_type_MAX; + enum field_type *types =3D = sql_affinity_str_to_field_type_str(zAff, + = nVector); sqlite3VdbeAddOp4(v, OP_ApplyType, rLhs, nVector, 0, (char = *)types, P4_DYNAMIC); if (destIfFalse =3D=3D destIfNull) { diff --git a/src/box/sql/select.c b/src/box/sql/select.c index 4b6983842..ef4514374 100644 --- a/src/box/sql/select.c +++ b/src/box/sql/select.c @@ -1204,7 +1204,8 @@ selectInnerLoop(Parse * pParse, /* The = parser context */ (unsigned int)nResultCol); enum field_type *types =3D = sql_affinity_str_to_field_type_str( - pDest->zAffSdst); + pDest->zAffSdst, + = strlen(pDest->zAffSdst)); sqlite3VdbeAddOp4(v, OP_MakeRecord, = regResult, nResultCol, r1, (char = *)types, P4_DYNAMIC); @@ -1631,7 +1632,8 @@ generateSortTail(Parse * pParse, /* Parsing = context */ sqlite3Strlen30(pDest->zAffSdst)); =20 enum field_type *types =3D - = sql_affinity_str_to_field_type_str(pDest->zAffSdst); + = sql_affinity_str_to_field_type_str(pDest->zAffSdst, + = strlen(pDest->zAffSdst)); sqlite3VdbeAddOp4(v, OP_MakeRecord, regRow, = nColumn, regTupleid, (char *)types, P4_DYNAMIC); @@ -3070,7 +3072,8 @@ generateOutputSubroutine(struct Parse *parse, = struct Select *p, testcase(in->nSdst > 1); r1 =3D sqlite3GetTempReg(parse); enum field_type *types =3D - = sql_affinity_str_to_field_type_str(dest->zAffSdst); + = sql_affinity_str_to_field_type_str(dest->zAffSdst, + = strlen(dest->zAffSdst)); sqlite3VdbeAddOp4(v, OP_MakeRecord, in->iSdst, in->nSdst, r1, (char *)types, P4_DYNAMIC); diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 729e5257c..79999509c 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -3454,7 +3454,7 @@ enum affinity_type sql_field_type_to_affinity(enum field_type field_type); =20 enum field_type * -sql_affinity_str_to_field_type_str(const char *affinity_str); +sql_affinity_str_to_field_type_str(const char *affinity_str, uint32_t = len); =20 /** * Compile view, i.e. create struct Select from diff --git a/src/box/sql/wherecode.c b/src/box/sql/wherecode.c index efbc91cf8..e3e7c5222 100644 --- a/src/box/sql/wherecode.c +++ b/src/box/sql/wherecode.c @@ -398,8 +398,7 @@ codeApplyAffinity(Parse * pParse, int base, int n, = char *zAff) =20 if (n > 0) { enum field_type *types =3D - sql_affinity_str_to_field_type_str(zAff); - types[n] =3D field_type_MAX; + sql_affinity_str_to_field_type_str(zAff, n); sqlite3VdbeAddOp4(v, OP_ApplyType, base, n, 0, (char = *)types, P4_DYNAMIC); > Also, see 2 comments below. >=20 >> 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 =3D -1; >> } >> r3 =3D = sqlite3ExprCodeTarget(pParse, pE2, r1); >> + enum field_type type =3D >> + = sql_affinity_to_field_type(affinity); >> + size_t sz =3D 2 * sizeof(enum = field_type); >> + enum field_type *types =3D >> + = sqlite3DbMallocRaw(pParse->db, >> + sz); >> + types[0] =3D type; >> + types[1] =3D field_type_MAX; >=20 > 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. Sorry, I accidentally added it to diff - it was proposal to refactoring (unsuccessful). Returned to its previous state. diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c index d263c1bb5..5676d0720 100644 --- a/src/box/sql/expr.c +++ b/src/box/sql/expr.c @@ -2856,15 +2856,11 @@ sqlite3CodeSubselect(Parse * pParse, /* = Parsing context */ r3 =3D = sqlite3ExprCodeTarget(pParse, pE2, r1); enum field_type type =3D = sql_affinity_to_field_type(affinity); - size_t sz =3D 2 * sizeof(enum = field_type); - enum field_type *types =3D - = sqlite3DbMallocRaw(pParse->db, - sz); - types[0] =3D type; - types[1] =3D field_type_MAX; + enum field_type types[2] =3D + { type, field_type_MAX = }; sqlite3VdbeAddOp4(v, = OP_MakeRecord, r3, 1, r2, (char = *)types, - P4_DYNAMIC); + = sizeof(types)); >> 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 =3D pOp->p1; >> - zAffinity =3D pOp->p4.z; >> + enum field_type *types =3D (enum field_type *)pOp->p4.z; >=20 > 2. Maybe, it is worth adding enum field_type *types into = VdbeOp.p4union > and do not cast. Like we did with many other pointers. Thanks for suggestion, but I guess it is not necessary now. Lets consider this refactoring a bit later.