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 19FC32BA78 for ; Tue, 17 Apr 2018 14:29:43 -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 zF2L_CJn7hjn for ; Tue, 17 Apr 2018 14:29:43 -0400 (EDT) Received: from smtp35.i.mail.ru (smtp35.i.mail.ru [94.100.177.95]) (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 C90992B9E0 for ; Tue, 17 Apr 2018 14:29:42 -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> From: Vladislav Shpilevoy Message-ID: <951402e9-502b-d1c2-e9ef-9805bd56ac90@tarantool.org> Date: Tue, 17 Apr 2018 21:29:40 +0300 MIME-Version: 1.0 In-Reply-To: <07b96d39-dcd4-9900-defe-3f11b9b20537@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 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.