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 4509A2735E for ; Wed, 18 Apr 2018 03:31:10 -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 y5wOUw4938X7 for ; Wed, 18 Apr 2018 03:31:10 -0400 (EDT) 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 CC7A826A31 for ; Wed, 18 Apr 2018 03:31:07 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 1/1] Removed Expr pointer from SQL Column structure. 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> From: Kirill Shcherbatov Message-ID: Date: Wed, 18 Apr 2018 10:31:02 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" 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: tarantool-patches@freelists.org Cc: "v.shpilevoy@tarantool.org" >From 0ed6b21e9e45974dcccfbcfe53675513ed731d0f Mon Sep 17 00:00:00 2001 From: Kirill Shcherbatov Date: Wed, 18 Apr 2018 10:26:51 +0300 Subject: [PATCH] Fixes --- src/box/sql/alter.c | 2 +- src/box/sql/build.c | 20 +++++++------------- src/box/sql/fkey.c | 1 - src/box/sql/sqliteInt.h | 3 ++- src/box/sql/update.c | 4 ---- 5 files changed, 10 insertions(+), 20 deletions(-) diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c index 39ae070..747d20e 100644 --- a/src/box/sql/alter.c +++ b/src/box/sql/alter.c @@ -161,7 +161,7 @@ sqlite3AlterFinishAddColumn(Parse * pParse, Token * pColDef) zTab = &pNew->zName[16]; /* Skip the "sqlite_altertab_" prefix on the name */ pCol = &pNew->aCol[pNew->nCol - 1]; - assert(pNew->def); + assert(pNew->def != NULL); pDflt = pNew->def->fields[pNew->nCol - 1].default_value_expr; pTab = sqlite3HashFind(&db->pSchema->tblHash, zTab);; assert(pTab); diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 20e9611..f3e9a7f 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -397,7 +397,7 @@ deleteTable(sqlite3 * db, Table * pTable) sqlite3SelectDelete(db, pTable->pSelect); sqlite3ExprListDelete(db, pTable->pCheck); if (pTable->def) { - /* fields has been allocated on separate region */ + /* Fields has been allocated independently. */ struct field_def *fields = pTable->def->fields; space_def_delete(pTable->def); sqlite3DbFree(db, fields); @@ -511,7 +511,6 @@ sql_table_new(Parse *pParse, char *zName) struct space_def *def = space_def_new(0, 0, 0, NULL, 0, NULL, 0, &space_opts_default, NULL, 0); if (pTable == NULL || def == NULL) { - assert(db->mallocFailed); if (def != NULL) space_def_delete(def); sqlite3DbFree(db, pTable); @@ -521,11 +520,6 @@ sql_table_new(Parse *pParse, char *zName) } pTable->def = def; - /* store allocated fields count */ - def->exact_field_count = 0; - def->field_count = 0; - pTable->def->fields = NULL; - pTable->zName = zName; pTable->iPKey = -1; pTable->iAutoIncPKey = -1; @@ -640,19 +634,19 @@ sql_field_retrieve(Parse *pParse, Table *pTable, uint32_t id) if (id >= pTable->def->exact_field_count) { uint32_t nCol = pTable->def->exact_field_count; - nCol = (nCol > 0) ? 2*nCol : 1; + nCol = (nCol > 0) ? 2 * nCol : 1; field = sqlite3DbRealloc(db, pTable->def->fields, nCol * sizeof(pTable->def->fields[0])); if (field == NULL) { - assert(db->mallocFailed); pParse->rc = SQLITE_NOMEM_BKPT; pParse->nErr++; return NULL; } - for (uint32_t i = nCol/2; i < nCol; i++) - memcpy(&field[i], &field_def_default, - sizeof(struct field_def)); + for (uint32_t i = nCol / 2; i < nCol; i++) { + memcpy(&field[i], &field_def_default, + sizeof(struct field_def)); + } pTable->def->fields = field; pTable->def->exact_field_count = nCol; @@ -893,7 +887,7 @@ sqlite3AddDefaultValue(Parse * pParse, ExprSpan * pSpan) * is required by pragma table_info. */ Expr x; - assert(p->def); + assert(p->def != NULL); struct field_def *field = &p->def->fields[p->nCol - 1]; sql_expr_free(db, field->default_value_expr, false); diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c index d196fd4..9b88b5f 100644 --- a/src/box/sql/fkey.c +++ b/src/box/sql/fkey.c @@ -36,7 +36,6 @@ #include #include "sqliteInt.h" #include "box/session.h" -#include "box/schema.h" #include "tarantoolInt.h" #ifndef SQLITE_OMIT_FOREIGN_KEY diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 90b7e08..19efbf3 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -1969,7 +1969,8 @@ 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 */ + /* Space definition with Tarantool metadata. */ + struct space_def *def; }; /* diff --git a/src/box/sql/update.c b/src/box/sql/update.c index 8d1cfdd..2743053 100644 --- a/src/box/sql/update.c +++ b/src/box/sql/update.c @@ -81,12 +81,8 @@ sqlite3ColumnDefault(Vdbe * v, Table * pTab, int i, int iReg) 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; - sqlite3ValueFromExpr(sqlite3VdbeDb(v), expr, pCol->affinity, &pValue); -- 2.7.4 On 17.04.2018 21:30, Vladislav Shpilevoy wrote: > 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. >