[tarantool-patches] Re: [PATCH 5/8] sql: replace field type with affinity for VDBE runtime
n.pettik
korablev at tarantool.org
Fri Feb 1 19:39:00 MSK 2019
>>>> 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.
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 = 12
#define TK_IN 12
LHS of the root is NULL (terminal symbol):
(lldb) print *pExpr->pLeft
(Expr) $6 = {
op = 'K'
= (affinity = AFFINITY_UNDEFINED, on_conflict_action = ON_CONFLICT_ACTION_NONE)
flags = 8388612
u = (zToken = "NULL", iValue = 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 = 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 = AFFINITY_TEXT;
822 break;
823 case TK_BLOB:
824 p->affinity = 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’t 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 == 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.
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)
}
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 == NULL)
return NULL;
- size_t len = strlen(affinity_str) + 1;
- size_t sz = len * sizeof(enum field_type);
+ size_t sz = (len + 1) * sizeof(enum field_type);
enum field_type *types =
(enum field_type *) sqlite3DbMallocRaw(sql_get(), sz);
- for (uint32_t i = 0; i < len - 1; ++i)
+ for (uint32_t i = 0; i < len; ++i)
types[i] = sql_affinity_to_field_type(affinity_str[i]);
- types[len - 1] = field_type_MAX;
+ types[len] = field_type_MAX;
return types;
}
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 = sql_affinity_str_to_field_type_str(zAff);
- types[nVector] = field_type_MAX;
+ enum field_type *types = sql_affinity_str_to_field_type_str(zAff,
+ nVector);
sqlite3VdbeAddOp4(v, OP_ApplyType, rLhs, nVector, 0, (char *)types,
P4_DYNAMIC);
if (destIfFalse == 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 =
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));
enum field_type *types =
- 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 = sqlite3GetTempReg(parse);
enum field_type *types =
- 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);
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);
/**
* 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)
if (n > 0) {
enum field_type *types =
- sql_affinity_str_to_field_type_str(zAff);
- types[n] = 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.
>
>> 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.
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 = 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;
+ enum field_type types[2] =
+ { 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 = 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.
Thanks for suggestion, but I guess it is not necessary now.
Lets consider this refactoring a bit later.
More information about the Tarantool-patches
mailing list