[tarantool-patches] Re: [PATCH v2 1/1] Removed Expr pointer from SQL Column structure.

[Vladislav Shpilevoy]

And please, rename the branch as I asked you - the
issue 3051 (lost tuple format) is not linked with the
problem, that we solve here.

On 17/04/2018 21:29, Vladislav Shpilevoy wrote:
> Hello. Thanks for fixes! See my 11 new comments below.
> 
>> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
>> index 129ef82..39ae070 100644
>> --- a/src/box/sql/alter.c
>> +++ b/src/box/sql/alter.c
>> @@ -161,7 +161,8 @@ sqlite3AlterFinishAddColumn(Parse * pParse, Token 
>> * pColDef)
>>
>>      zTab = &pNew->zName[16];    /* Skip the "sqlite_altertab_" prefix 
>> on the name */
>>      pCol = &pNew->aCol[pNew->nCol - 1];
>> -    pDflt = pCol->pDflt;
>> +    assert(pNew->def);
> 
> 1. Please, use explicit != NULL, when compare pointers. In other places
> too.
> 
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index 92f3cb6..394ee3a 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -397,6 +396,12 @@ deleteTable(sqlite3 * db, Table * pTable)
>>      sqlite3DbFree(db, pTable->zColAff);
>>      sqlite3SelectDelete(db, pTable->pSelect);
>>      sqlite3ExprListDelete(db, pTable->pCheck);
>> +    if (pTable->def) {
>> +        /* fields has been allocated on separate region */
>> +        struct field_def *fields = pTable->def->fields;
>> +        space_def_delete(pTable->def);
>> +        sqlite3DbFree(db, fields);
>> +    }
> 
> 2. Please, do not use 'region' term here. In Tarantool region is
> the allocator, and this comment slightly confuses, since the
> fields array is allocated on heap instead of region.
> 
> More style comments: start a comment with capital letter and put
> a dot at the end of sentences.
> 
>> @@ -490,6 +495,49 @@ sqlite3PrimaryKeyIndex(Table * pTab)
>>      return p;
>>  }
>>
>> +/**
>> + * Create and initialize a new SQL Table object.
>> + * @param pParse SQL Parser object.
>> + * @param zName Table to create name.
>> + * @retval NULL on memory allocation error, Parser state changed.
>> + * @retval not NULL on success.
>> + */
>> +static Table *
>> +sql_table_new(Parse *pParse, char *zName)
>> +{
>> +    sqlite3 *db = pParse->db;
>> +
>> +    Table *pTable = sqlite3DbMallocZero(db, sizeof(Table));
>> +    struct space_def *def = space_def_new(0, 0, 0, NULL, 0, NULL, 0,
>> +                          &space_opts_default,
>> +                          &field_def_default, 0);
>> +    if (pTable == NULL || def == NULL) {
>> +        assert(db->mallocFailed);
> 
> 3. This assertion fails, if space_def_new is failed, because
> it does not set db flags.
> 
>> +    pTable->def = def;
>> +    /* store allocated fields count */
>> +    def->exact_field_count = 0;
> 
> 4. Same as 2 - comments style.
> 
>> +    def->exact_field_count = 0;
>> +    def->field_count = 0;
>> +    pTable->def->fields = NULL;
> 
> 5. These fields are initialized already in space_def_new.
> 
>> +/**
>> + * Get field by id. Allocate memory if needed.
>> + * @param pParse SQL Parser object.
>> + * @param pTable SQL Table object.
>> + * @param id column identifier.
>> + * @retval not NULL on success.
>> + * @retval NULL on out of memory.
>> + */
>> +static struct field_def *
>> +sql_field_retrieve(Parse *pParse, Table *pTable, uint32_t id)
>> +{
>> +    sqlite3 *db = pParse->db;
>> +    struct field_def *field;
>> +    assert(pTable->def);
>> +    assert(pTable->def->exact_field_count >= (uint32_t)pTable->nCol);
>> +    assert(id < (uint32_t)db->aLimit[SQLITE_LIMIT_COLUMN]);
>> +
>> +    if (id >= pTable->def->exact_field_count) {
>> +        uint32_t nCol = pTable->def->exact_field_count;
>> +        nCol = (nCol > 0) ? 2*nCol : 1;
> 
> 6. Please, put spaces around arithmetic operators.
> 
>> +        for (uint32_t i = nCol/2; i < nCol; i++)
>> +             memcpy(&field[i], &field_def_default,
>> +                sizeof(struct field_def));
> 
> 7. Same as 6. And put a 'for' body into {}, when it consists of
> multiple lines.
> 
>> @@ -813,7 +894,11 @@ sqlite3AddDefaultValue(Parse * pParse, ExprSpan * 
>> pSpan)
>>               * is required by pragma table_info.
>>               */
>>              Expr x;
>> -            sql_expr_free(db, pCol->pDflt, false);
>> +            assert(p->def);
> 
> 8. Same as 1.
> 
>> diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
>> index f56b6d9..d196fd4 100644
>> --- a/src/box/sql/fkey.c
>> +++ b/src/box/sql/fkey.c
>> @@ -36,6 +36,8 @@
>>  #include <box/coll.h>
>>  #include "sqliteInt.h"
>>  #include "box/session.h"
>> +#include "box/schema.h"
> 
> 9. Unused header.
> 
>> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
>> index 59662cf..90b7e08 100644
>> --- a/src/box/sql/sqliteInt.h
>> +++ b/src/box/sql/sqliteInt.h
>> @@ -1970,6 +1969,7 @@ struct Table {
>>      Trigger *pTrigger;    /* List of triggers stored in pSchema */
>>      Schema *pSchema;    /* Schema that contains this table */
>>      Table *pNextZombie;    /* Next on the Parse.pZombieTab list */
>> +    struct space_def *def;    /* Space definition with Tarantool 
>> metadata */
> 
> 10. For new members please use Tarantool code style - the
> comment above the member.
> 
>> diff --git a/src/box/sql/update.c b/src/box/sql/update.c
>> index 83c05ab..8d1cfdd 100644
>> --- a/src/box/sql/update.c
>> +++ b/src/box/sql/update.c
>> @@ -35,6 +35,8 @@
>>   */
>>  #include "sqliteInt.h"
>>  #include "box/session.h"
>> +#include "tarantoolInt.h"
>> +#include "box/schema.h"
>>
>>  /*
>>   * The most recently coded instruction was an OP_Column to retrieve the
>> @@ -75,7 +77,18 @@ sqlite3ColumnDefault(Vdbe * v, Table * pTab, int i, 
>> int iReg)
>>          Column *pCol = &pTab->aCol[i];
>>          VdbeComment((v, "%s.%s", pTab->zName, pCol->zName));
>>          assert(i < pTab->nCol);
>> -        sqlite3ValueFromExpr(sqlite3VdbeDb(v), pCol->pDflt,
>> +
>> +        Expr *expr = NULL;
>> +        struct space *space =
>> +            space_cache_find(SQLITE_PAGENO_TO_SPACEID(pTab->tnum));
>> +        /* FIXME: ephemeral spaces are not present in the cache now */
>> +        if (space != NULL && space->def->fields != NULL)
>> +            expr = space->def->fields[i].default_value_expr;
>> +        if (expr == NULL && pTab->def != NULL)
>> +            expr = pTab->def->fields[i].default_value_expr;
>> +
> 
> 11. You must not use default from pTab->def after DDL is finished. As I
> remember we discussed it verbally, so lets just remove it.