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 DE7EF2BD67 for ; Tue, 17 Apr 2018 14:30:46 -0400 (EDT) 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 Iyrtm0DRqp2f for ; Tue, 17 Apr 2018 14:30:46 -0400 (EDT) Received: from smtp41.i.mail.ru (smtp41.i.mail.ru [94.100.177.101]) (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 995332BBA4 for ; Tue, 17 Apr 2018 14:30:46 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 1/1] Removed Expr pointer from SQL Column structure. From: Vladislav Shpilevoy References: <60fd0daec8e35563602212ca8a3307ab49b14d15.1523896524.git.kshcherbatov@tarantool.org> <71db35b8-be0a-34be-07ab-09d63c4c39cd@tarantool.org> <07b96d39-dcd4-9900-defe-3f11b9b20537@tarantool.org> <951402e9-502b-d1c2-e9ef-9805bd56ac90@tarantool.org> Message-ID: Date: Tue, 17 Apr 2018 21:30:44 +0300 MIME-Version: 1.0 In-Reply-To: <951402e9-502b-d1c2-e9ef-9805bd56ac90@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: Kirill Shcherbatov , tarantool-patches@freelists.org 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 >>  #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.