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 06AB520393 for ; Thu, 19 Apr 2018 11:25:08 -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 mdP9DwQernKO for ; Thu, 19 Apr 2018 11:25:07 -0400 (EDT) Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94]) (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 46FCD2049B for ; Thu, 19 Apr 2018 11:25:06 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 1/1] sql: remove zName and nColumn from SQL References: <0a797b62416743d6e6494949710b70a330f36c04.1523977261.git.kshcherbatov@tarantool.org> <9b8b35f390c9309ce4ecd9d186162d13b2332c56.1524141971.git.kshcherbatov@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Thu, 19 Apr 2018 18:25:04 +0300 MIME-Version: 1.0 In-Reply-To: <9b8b35f390c9309ce4ecd9d186162d13b2332c56.1524141971.git.kshcherbatov@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: tarantool-patches@freelists.org, Kirill Shcherbatov Hello. Thanks for contributing! See below my 14 comments. On 19/04/2018 15:47, Kirill Shcherbatov wrote: > 1. Removed zName from SQL Column. > 2. Removed zColumns from SQL Table. > 3. Refactored Parser to use def_expression directly. > 4. Introduced sql_table_def_rebuild intended for collect > fragmented with sql_field_retrieve space_def into memory > located in one allocation. > > Needed for #3272. > --- > src/box/space_def.c | 29 ++++---- > src/box/sql.c | 39 +++++++--- > src/box/sql.h | 22 ++++++ > src/box/sql/alter.c | 31 +++++--- > src/box/sql/analyze.c | 5 +- > src/box/sql/build.c | 194 +++++++++++++++++++++++------------------------- > src/box/sql/delete.c | 6 +- > src/box/sql/expr.c | 11 +-- > src/box/sql/fkey.c | 20 ++--- > src/box/sql/insert.c | 53 +++++++------ > src/box/sql/pragma.c | 24 +++--- > src/box/sql/resolve.c | 16 ++-- > src/box/sql/select.c | 96 +++++++++++++----------- > src/box/sql/sqliteInt.h | 15 +++- > src/box/sql/update.c | 29 ++++---- > src/box/sql/where.c | 6 +- > src/box/sql/wherecode.c | 2 +- > src/box/sql/whereexpr.c | 4 +- > 18 files changed, 341 insertions(+), 261 deletions(-) > > diff --git a/src/box/space_def.c b/src/box/space_def.c > index 22bd3ca..77c0e02 100644 > --- a/src/box/space_def.c > +++ b/src/box/space_def.c > > @@ -116,12 +117,13 @@ space_def_dup(const struct space_def *src) > if (src->fields[i].default_value != NULL) { > ret->fields[i].default_value = strs_pos; > strs_pos += strlen(strs_pos) + 1; > - > - struct Expr *e = > - src->fields[i].default_value_expr; > - assert(e != NULL); > + } > + struct Expr *e = > + src->fields[i].default_value_expr; > + if (e != NULL) { > char *expr_pos_old = expr_pos; > - e = sql_expr_dup(sql_get(), e, 0, &expr_pos); > + e = sql_expr_dup(sql_get(), e, 0, > + &expr_pos); 1. Unnecessary diff. > assert(e != NULL); > /* Note: due to SQL legacy > * duplicactor pointer is not > @@ -201,12 +203,13 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count, > fields[i].default_value, len); > def->fields[i].default_value[len] = 0; > strs_pos += len + 1; > - > - struct Expr *e = > - fields[i].default_value_expr; > - assert(e != NULL); > + } > + struct Expr *e = > + fields[i].default_value_expr; > + if (e != NULL) { > char *expr_pos_old = expr_pos; > - e = sql_expr_dup(sql_get(), e, 0, &expr_pos); > + e = sql_expr_dup(sql_get(), e, 0, > + &expr_pos); 2. Same as 1. > diff --git a/src/box/sql.c b/src/box/sql.c > index 6bc82c2..e3ae82c 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -1712,3 +1710,26 @@ space_column_default_expr(uint32_t space_id, uint32_t fieldno) > > return space->def->fields[fieldno].default_value_expr; > } > + > +int > +sql_table_def_rebuild(struct sqlite3 *db, struct Table *pTable) > +{ > + struct space_def *old_def = pTable->def; > + struct space_def *new_def = NULL; > + new_def = space_def_new(old_def->id, old_def->uid, > + old_def->field_count, old_def->name, > + strlen(old_def->name), old_def->engine_name, > + strlen(old_def->engine_name), &old_def->opts, > + old_def->fields, old_def->field_count); > + if (new_def == NULL) { > + sqlite3OomFault(db); > + return 1; 3. Lets return -1, it is a common practice. And as I can understand, here old_def is fragmented still, so it will not be fully deleted by space_def_delete later in deleteTable. Fix this, please. As far as I can remember, I proposed to do space_def rebuild on each sql_field_retrieve until we start to use region in parser. > diff --git a/src/box/sql.h b/src/box/sql.h > index db92d80..2a9b59f 100644 > --- a/src/box/sql.h > +++ b/src/box/sql.h > @@ -65,6 +65,7 @@ sql_get(); > struct Expr; > struct Parse; > struct Select; > +struct Table; > > /** > * Perform parsing of provided expression. This is done by > @@ -143,6 +144,27 @@ sql_expr_dup(struct sqlite3 *db, struct Expr *p, int flags, char **buffer); > void > sql_expr_free(struct sqlite3 *db, struct Expr *expr, bool extern_alloc); > > +/** > + * Create and initialize a new ephemeric SQL Table object. 4. 'Ephemeral', not 'ephemeric' - they mean completely different things. > + * @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. > + */ > +struct Table * > +sql_ephemeric_table_new(struct Parse *pParse); 5. Lets use Tarantool code style for parameters. > + > +/** > + * Rebuilds struct def in Table with memory allocated on region. 6. Function comment must not describe it as an object. It must describe what the function does. So not 'rebuilds', but 'rebuild'. And this function does not use region. What do you mean? > + * Fields and strings are expected to be allocated with sqlite3DbMalloc. > + * @param db The database connection. > + * @param pTable The Table with fragmented memory in def to rebuild.. > + * @retval 1 on memory allocation error > + * @retval 0 on success > + */ > +int > +sql_table_def_rebuild(struct sqlite3 *db, struct Table *pTable); > + > #if defined(__cplusplus) > } /* extern "C" { */ > #endif > diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c > index cf8be5d..47ee5e8 100644 > --- a/src/box/sql/alter.c > +++ b/src/box/sql/alter.c > diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c > index 665bfbc..350bc8c 100644 > --- a/src/box/sql/analyze.c > +++ b/src/box/sql/analyze.c > @@ -1028,9 +1028,10 @@ analyzeOneTable(Parse * pParse, /* Parser context */ > regKeyStat = sqlite3GetTempRange(pParse, nPkColumn); > for (j = 0; j < nPkColumn; j++) { > k = pPk->aiColumn[j]; > - assert(k >= 0 && k < pTab->nCol); > + assert(k >= 0 && k < (int)pTab->def->field_count); > sqlite3VdbeAddOp3(v, OP_Column, iIdxCur, k, regKeyStat + j); > - VdbeComment((v, "%s", pTab->aCol[pPk->aiColumn[j]].zName)); > + VdbeComment((v, "%s", > + pTab->def->fields[pPk->aiColumn[j]].name)); 7. Please, align the new line. > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index ab6df46..4ac421c 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -297,8 +297,7 @@ sqlite3DeleteColumnNames(sqlite3 * db, Table * pTable) > Column *pCol; > assert(pTable != 0); > if ((pCol = pTable->aCol) != 0) { > - for (i = 0; i < pTable->nCol; i++, pCol++) { > - sqlite3DbFree(db, pCol->zName); > + for (i = 0; i < (int)pTable->def->field_count; i++, pCol++) { > sqlite3DbFree(db, pCol->zColl); 8. Please, rebase on latest 2.0 - this place is slightly changed already. And do not put 'for' body into {}, when it consists of a single line. > /** > * 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 is > - * changed. 9. Looks like you force pushed your patch over my review fixes. Here 'is' is needed. > @@ -624,13 +627,12 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr) > * @retval not NULL on success. > * @retval NULL on out of memory. > */ > -static struct field_def * > +struct field_def * 10. Same - you removed my fixes. It is used in build.c only and can be static. > 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(pTable->def != NULL); > assert(id < (uint32_t)db->aLimit[SQLITE_LIMIT_COLUMN]); > > if (id >= pTable->def->exact_field_count) { > @@ -638,11 +640,8 @@ sql_field_retrieve(Parse *pParse, Table *pTable, uint32_t id) > nCol = (nCol > 0) ? 2 * nCol : 1; > field = sqlite3DbRealloc(db, pTable->def->fields, > nCol * sizeof(pTable->def->fields[0])); > - if (field == NULL) { > - pParse->rc = SQLITE_NOMEM_BKPT; > - pParse->nErr++; > - return NULL; > - } > + if (field == NULL) > + goto error; 11. Why you moved it to the bottom and added label? I can see only one goto error, so it makes no sense to move this code. > @@ -1375,7 +1366,7 @@ estimateTableWidth(Table * pTab) > unsigned wTable = 0; > const Column *pTabCol; > int i; > - for (i = pTab->nCol, pTabCol = pTab->aCol; i > 0; i--, pTabCol++) { > + for (i = pTab->def->field_count, pTabCol = pTab->aCol; i > 0; i--, pTabCol++) { 12. Out of 80 symbols. > @@ -2950,7 +2944,7 @@ sqlite3CreateIndex(Parse * pParse, /* All information about this parse */ > */ > if (pList == 0) { > Token prevCol; > - sqlite3TokenInit(&prevCol, pTab->aCol[pTab->nCol - 1].zName); > + sqlite3TokenInit(&prevCol, pTab->def->fields[pTab->def->field_count - 1].name); 13. Same as 12. > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index 7ce5bac..df26bcc 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -4156,4 +4154,15 @@ table_column_nullable_action(struct Table *tab, uint32_t column); > bool > table_column_is_nullable(struct Table *tab, uint32_t column); > > +/** > + * 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. > + */ > +struct field_def * > +sql_field_retrieve(Parse *pParse, Table *pTable, uint32_t id); 14. Why? This function is used in build.c only and can be static.