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 8A6B525375 for ; Fri, 11 May 2018 16:59:49 -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 PoQ-SEFrbwBH for ; Fri, 11 May 2018 16:59:49 -0400 (EDT) Received: from smtp50.i.mail.ru (smtp50.i.mail.ru [94.100.177.110]) (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 AEF282531C for ; Fri, 11 May 2018 16:59:48 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v5 2/3] sql: remove SQL fields from Table and Column References: From: Vladislav Shpilevoy Message-ID: <70dd9a56-4374-56b7-6564-7cfb0309f0b7@tarantool.org> Date: Fri, 11 May 2018 23:59:45 +0300 MIME-Version: 1.0 In-Reply-To: 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 the patch! See below 43 comments. On 11/05/2018 11:49, Kirill Shcherbatov wrote: > 1. Removed zName, is_nullable, collation, type > from SQL Column. > 2. Removed zColumns, zName from SQL Table. > 3. Refactored Parser to use def_expression directly. > 4. Introduced is_view flag. > 4. Introduced sql_table_def_rebuild intended for collect > fragmented with sql_field_retrieve space_def into memory > located in one allocation. > > Part of #3272, #3218. > --- > src/box/field_def.h | 6 + > src/box/space_def.c | 29 +++-- > src/box/sql.c | 128 +++++++++++++++---- > src/box/sql.h | 33 +++++ > src/box/sql/alter.c | 55 +++++--- > src/box/sql/analyze.c | 16 ++- > src/box/sql/build.c | 334 ++++++++++++++++++++++++++---------------------- > src/box/sql/delete.c | 21 +-- > src/box/sql/expr.c | 14 +- > src/box/sql/fkey.c | 38 +++--- > src/box/sql/hash.c | 5 +- > src/box/sql/hash.h | 2 +- > src/box/sql/insert.c | 67 +++++----- > src/box/sql/pragma.c | 34 +++-- > src/box/sql/prepare.c | 45 +++---- > src/box/sql/resolve.c | 22 ++-- > src/box/sql/select.c | 159 +++++++++++++---------- > src/box/sql/sqliteInt.h | 37 ++++-- > src/box/sql/tokenize.c | 5 +- > src/box/sql/treeview.c | 2 +- > src/box/sql/trigger.c | 7 +- > src/box/sql/update.c | 33 ++--- > src/box/sql/util.c | 9 -- > src/box/sql/vdbe.c | 2 +- > src/box/sql/vdbeaux.c | 9 +- > src/box/sql/where.c | 12 +- > src/box/sql/wherecode.c | 4 +- > src/box/sql/whereexpr.c | 6 +- > 28 files changed, 675 insertions(+), 459 deletions(-) > > diff --git a/src/box/field_def.h b/src/box/field_def.h > index dfc1950..a42beab 100644 > --- a/src/box/field_def.h > +++ b/src/box/field_def.h > @@ -116,6 +116,12 @@ struct field_def { > struct Expr *default_value_expr; > }; > > +static inline bool > +nullable_action_to_is_nullable(enum on_conflict_action nullable_action) > +{ > + return nullable_action == ON_CONFLICT_ACTION_NONE; > +} 1. Lets remove 'to': nullable_action_is_nullable. It is too long too, but I can not think up another. > 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); 2. Same as in the previous review: garbage diff. > @@ -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); 3. Same. > diff --git a/src/box/sql.c b/src/box/sql.c > index 166bb71..7d48cdc 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -1681,3 +1697,65 @@ space_column_default_expr(uint32_t space_id, uint32_t fieldno) > > return space->def->fields[fieldno].default_value_expr; > } > + > +struct space_def * > +sql_ephemeral_space_def_new(Parse *parser, const char *name) > +{ > + struct space_def *def = NULL; > + struct region *region = &fiber()->gc; > + size_t name_len = name != NULL ? strlen(name) : 0; > + size_t size = sizeof(struct space_def) + name_len + 1; 4. Use space_def_sizeof for this please. > + def = (struct space_def *)region_alloc(region, size); > + if (def == NULL) { > + diag_set(OutOfMemory, sizeof(struct tuple_dictionary), > + "region_alloc", "sql_ephemeral_space_def_new"); > + parser->rc = SQL_TARANTOOL_ERROR; > + parser->nErr++; > + return NULL; > + } else { 5. If you return on def == NULL, then either move all the other code under 'else' body, or remove the 'else'. > + memset(def, 0, size); > + } > + memcpy(def->name, name, name_len); > + def->name[name_len] = '\0'; > + def->opts.temporary = true; > + return def; > +} > + > diff --git a/src/box/sql.h b/src/box/sql.h > index db92d80..ac8d07a 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,38 @@ 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. 6. Same as in the previous review - ephemeric -> ephemeral. > + * @param parser SQL Parser object. > + * @param name Table to create name. > + * @retval NULL on memory allocation error, Parser state changed. > + * @retval not NULL on success. > + */ > +struct Table * > +sql_ephemeral_table_new(struct Parse *parser, const char *name); > + > +/** > + * Create and initialize a new ephemeric space_def object. 7. Same. > + * @param parser SQL Parser object. > + * @param name Table to create name. > + * @retval NULL on memory allocation error, Parser state changed. > + * @retval not NULL on success. > + */ > +struct space_def * > +sql_ephemeral_space_def_new(struct Parse *parser, const char *name); > + > +/** > + * Rebuild struct def in Table with memory allocated on a single > + * malloc. Fields and strings are expected to be allocated with > + * sqlite3DbMalloc. 8. In the function body: "/* All allocations are on region. */". Please, fix the comment. > diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c > index 24f0965..85404ac 100644 > --- a/src/box/sql/alter.c > +++ b/src/box/sql/alter.c > @@ -195,7 +199,12 @@ sqlite3AlterFinishAddColumn(Parse * pParse, Token * pColDef) > "Cannot add a REFERENCES column with non-NULL default value"); > return; > } > - if (pCol->notNull && !pDflt) { > + assert(pNew->def->fields[pNew->def->field_count - 1].is_nullable == > + nullable_action_to_is_nullable( > + pNew->def->fields[pNew->def->field_count - 1].nullable_action)); > + > + if (pNew->def->fields[pNew->def->field_count - 1].nullable_action 9. Please, do not use non-boolean variables as booleans. Use (action != ON_CONFLICT_ACTION_NONE) instead of just (action). > + && !pDflt) { 10. pDflt == NULL. > @@ -281,25 +293,30 @@ sqlite3AlterBeginAddColumn(Parse * pParse, SrcList * pSrc) > pNew = (Table *) sqlite3DbMallocZero(db, sizeof(Table)); > if (!pNew) > goto exit_begin_add_column; > + pNew->def = space_def_dup(pTab->def); > + if (pNew->def == NULL) { 11. Forgot to set sqlite3OomFault(db). I know, that this code is unreachable, but in the future it will be not. > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index a2b712a..a02fe89 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -291,15 +291,8 @@ sqlite3CommitInternalChanges() > void > sqlite3DeleteColumnNames(sqlite3 * db, Table * pTable) > { > - int i; > - Column *pCol; > assert(pTable != 0); > - if ((pCol = pTable->aCol) != 0) { > - for (i = 0; i < pTable->nCol; i++, pCol++) { > - sqlite3DbFree(db, pCol->zName); > - } > - sqlite3DbFree(db, pTable->aCol); > - } > + sqlite3DbFree(db, pTable->aCol); 12. Nice. Now this function consists of a single DbFree - lets just inline it and remove sqlite3DeleteColumnNames. > @@ -494,31 +484,19 @@ sqlite3PrimaryKeyIndex(Table * pTab) > > /** > * Create and initialize a new SQL Table object. > + * All memory is allocated on region. 13. Not all - the table itself is on the heap. > @@ -626,30 +605,35 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr) > static struct field_def * > sql_field_retrieve(Parse *parser, Table *table, uint32_t id) > { > - sqlite3 *db = parser->db; > struct field_def *field; > assert(table->def != NULL); > - assert(table->def->exact_field_count >= (uint32_t)table->nCol); > assert(id < SQLITE_MAX_COLUMN); > > if (id >= table->def->exact_field_count) { > - uint32_t columns = table->def->exact_field_count; > - columns = (columns > 0) ? 2 * columns : 1; > - field = sqlite3DbRealloc(db, table->def->fields, > - columns * sizeof(table->def->fields[0])); > + uint32_t columns_new = table->def->exact_field_count; > + columns_new = (columns_new > 0) ? 2 * columns_new : 1; > + struct region *region = &fiber()->gc; > + field = region_alloc(region, columns_new * > + sizeof(table->def->fields[0])); > if (field == NULL) { > - parser->rc = SQLITE_NOMEM_BKPT; > + diag_set(OutOfMemory, columns_new * sizeof(table->def->fields[0]), > + "region_alloc", "sql_field_retrieve"); 14. Out of 80 symbols. > + parser->rc = SQL_TARANTOOL_ERROR; > parser->nErr++; > return NULL; > } > > - for (uint32_t i = columns / 2; i < columns; i++) { > + for (uint32_t i = 0; i < table->def->exact_field_count; i++) { > + memcpy(&field[i], &table->def->fields[i], > + sizeof(struct field_def)); 15. Why you can not do one memcpy(field, table->def->fields, sizeof(*field) * table->def->exact_field_count); ? > + } > + for (uint32_t i = columns_new / 2; i < columns_new; i++) { > memcpy(&field[i], &field_def_default, > sizeof(struct field_def)); > @@ -675,42 +659,44 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType) > sqlite3 *db = pParse->db; > if ((p = pParse->pNewTable) == 0) > return; > + assert(p->def->opts.temporary); > #if SQLITE_MAX_COLUMN > - if (p->nCol + 1 > db->aLimit[SQLITE_LIMIT_COLUMN]) { > - sqlite3ErrorMsg(pParse, "too many columns on %s", p->zName); > + if ((int)p->def->field_count + 1 > db->aLimit[SQLITE_LIMIT_COLUMN]) { > + sqlite3ErrorMsg(pParse, "too many columns on %s", > + p->def->name); > return; > } > #endif > - if (sql_field_retrieve(pParse, p, (uint32_t) p->nCol) == NULL) > + if (sql_field_retrieve(pParse, p, (uint32_t) p->def->field_count) == NULL) 16. Out of 80 symbols. > return; > - z = sqlite3DbMallocRaw(db, pName->n + 1); > + struct region *region = &fiber()->gc; > + z = region_alloc(region, pName->n + 1); > if (z == 0) 17. Missed diag_set + pParse->nErr + pParse->rc. > @@ -756,9 +741,11 @@ sqlite3AddNotNull(Parse * pParse, int onError) > { > Table *p; > p = pParse->pNewTable; > - if (p == 0 || NEVER(p->nCol < 1)) > + if (p == 0 || NEVER(p->def->field_count < 1)) > return; > - p->aCol[p->nCol - 1].notNull = (u8) onError; > + p->def->fields[p->def->field_count - 1].nullable_action = (u8)onError; > + p->def->fields[p->def->field_count - 1].is_nullable = > + (onError == ON_CONFLICT_ACTION_NONE); 18. Now you have nullable_action_to_is_nullable. > @@ -871,38 +858,31 @@ void > sqlite3AddDefaultValue(Parse * pParse, ExprSpan * pSpan) > { > Table *p; > - Column *pCol; > sqlite3 *db = pParse->db; > p = pParse->pNewTable; > + assert(p->def->opts.temporary); > if (p != 0) { > - pCol = &(p->aCol[p->nCol - 1]); > if (!sqlite3ExprIsConstantOrFunction > (pSpan->pExpr, db->init.busy)) { > sqlite3ErrorMsg(pParse, > "default value of column [%s] is not constant", > - pCol->zName); > + p->def->fields[p->def->field_count - 1].name); > } else { > - /* A copy of pExpr is used instead of the original, as pExpr contains > - * tokens that point to volatile memory. The 'span' of the expression > - * is required by pragma table_info. > - */ > - Expr x; > assert(p->def != NULL); > struct field_def *field = > - &p->def->fields[p->nCol - 1]; > - sql_expr_free(db, field->default_value_expr, false); > - > - memset(&x, 0, sizeof(x)); > - x.op = TK_SPAN; > - x.u.zToken = sqlite3DbStrNDup(db, (char *)pSpan->zStart, > - (int)(pSpan->zEnd - > - pSpan->zStart)); > - x.pLeft = pSpan->pExpr; > - x.flags = EP_Skip; > - > - field->default_value_expr = > - sqlite3ExprDup(db, &x, EXPRDUP_REDUCE); > - sqlite3DbFree(db, x.u.zToken); > + &p->def->fields[p->def->field_count - 1]; > + struct region *region = &fiber()->gc; > + uint32_t default_length = (int)(pSpan->zEnd - pSpan->zStart); > + field->default_value = region_alloc(region, > + default_length + 1); > + if (field->default_value == NULL) { 19. Lets use diag + SQL_TARANTOOL_ERROR. > + pParse->rc = SQLITE_NOMEM_BKPT; > + pParse->nErr++; > + return; > + } > + strncpy(field->default_value, (char *)pSpan->zStart, 20. Why do you case to char*? zStart is already const char* and strncpy takes it ok. > @@ -1087,8 +1067,10 @@ sql_column_collation(Table *table, uint32_t column) > * SQL specific structures. > */ > if (space == NULL || space_index(space, 0) == NULL) { > - assert(column < (uint32_t)table->nCol); > - return table->aCol[column].coll; > + assert(column < (uint32_t)table->def->field_count); > + struct coll *coll = > + coll_by_id(table->def->fields[column].coll_id); > + return coll; 21. It is simpler and shorter to return coll_by_id() with no saving into the variable. > @@ -2149,11 +2142,16 @@ sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable) > * normally holds CHECK constraints on an ordinary table, but for > * a VIEW it holds the list of column names. > */ > - sqlite3ColumnsFromExprList(pParse, pTable->pCheck, > - &pTable->nCol, > - &pTable->aCol); > + sqlite3ColumnsFromExprList(pParse, pTable->pCheck, pTable); > + struct space_def *old_def = pTable->def; > + /* Delete it manually. */ > + old_def->opts.temporary = true; > + if (sql_table_def_rebuild(db, pTable) != 0) > + nErr++; > + space_def_delete(old_def); 22. On error pTable->def is not reset, so here you would delete actually old pTable->def, that is on a region. Do this space_def_delete in 'else' branch. > @@ -2163,12 +2161,31 @@ sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable) > * the column names from the SELECT statement that defines the view. > */ > assert(pTable->aCol == 0); > - pTable->nCol = pSelTab->nCol; > + assert((int)pTable->def->field_count == -1); > + assert(pSelTab->def->opts.temporary); > + > + struct space_def *old_def = pTable->def; > + struct space_def *new_def = NULL; 23. Useless initialization - you reset this variable on the next line. > + new_def = sql_ephemeral_space_def_new(pParse, old_def->name); 24. Out of 80 symbols. > + if (new_def == NULL) { > + nErr++; > + } else { > + memcpy(new_def, old_def, sizeof(struct space_def)); 25. Same. > + new_def->dict = NULL; > + new_def->opts.temporary = true; > + new_def->fields = pSelTab->def->fields; > + new_def->field_count = pSelTab->def->field_count; 26. Same. > @@ -2193,10 +2210,22 @@ sqliteViewResetAll(sqlite3 * db) > for (i = sqliteHashFirst(&db->pSchema->tblHash); i; > i = sqliteHashNext(i)) { > Table *pTab = sqliteHashData(i); > - if (pTab->pSelect) { > + assert(pTab->def->opts.is_view == (pTab->pSelect != NULL)); > + if (pTab->def->opts.is_view) { > sqlite3DeleteColumnNames(db, pTab); > + struct space_def *old_def = pTab->def; > + assert(old_def->opts.temporary == false); > + /* Ignore fields allocated on region */ 27. As I can see, here pTab is the normal view, that has normal non-temporary space_def on malloc. Not on region. > + pTab->def = space_def_new(old_def->id, old_def->uid, > + 0, old_def->name, > + strlen(old_def->name), > + old_def->engine_name, > + strlen(old_def->engine_name), > + &old_def->opts, > + NULL, 0); > + assert(pTab->def); 28. It is not guaranteed. You must check for def == NULL and set sqlite3OomFault(db). And do not reset pTab def, if you can not create a new one. > @@ -2966,7 +2996,9 @@ 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); 29. Please, do not wrap '->'. And it is out of 80 symbols anyway. > diff --git a/src/box/sql/hash.c b/src/box/sql/hash.c > index cedcb7d..b109cca 100644 > --- a/src/box/sql/hash.c > +++ b/src/box/sql/hash.c > @@ -69,6 +69,7 @@ sqlite3HashClear(Hash * pH) > while (elem) { > HashElem *next_elem = elem->next; > sqlite3_free(elem); > + free(elem->pKey); > elem = next_elem; > } > pH->count = 0; > @@ -292,7 +293,7 @@ sqlite3HashInsert(Hash * pH, const char *pKey, void *data) > removeElementGivenHash(pH, elem, h); > } else { > elem->data = data; > - elem->pKey = pKey; > + elem->pKey = strdup(pKey); 30. strdup can fail. See the sqlite3HashInsert what to do in such a case. > @@ -301,7 +302,7 @@ sqlite3HashInsert(Hash * pH, const char *pKey, void *data) > new_elem = (HashElem *) sqlite3Malloc(sizeof(HashElem)); > if (new_elem == 0) > return data; > - new_elem->pKey = pKey; > + new_elem->pKey = strdup(pKey); 31. Same. > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index 939b5e3..c272ae1 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -146,13 +146,14 @@ sqlite3TableAffinity(Vdbe * v, Table * pTab, int iReg) > char *zColAff = pTab->zColAff; > if (zColAff == 0) { > sqlite3 *db = sqlite3VdbeDb(v); > - zColAff = (char *)sqlite3DbMallocRaw(0, pTab->nCol + 1); > + zColAff = (char *)sqlite3DbMallocRaw(0, > + pTab->def->field_count + 1); 32. Out of 80. > @@ -611,10 +612,10 @@ sqlite3Insert(Parse * pParse, /* Parser context */ > ipkColumn = pTab->iPKey; > } > > - if (pColumn == 0 && nColumn && nColumn != (pTab->nCol)) { > + if (pColumn == 0 && nColumn && nColumn != ((int)pTab->def->field_count)) { 33. Same. > diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c > index 2ab8751..926c3dd 100644 > --- a/src/box/sql/prepare.c > +++ b/src/box/sql/prepare.c > @@ -456,3 +435,21 @@ sqlite3_prepare_v2(sqlite3 * db, /* Database handle. */ > assert(rc == SQLITE_OK || ppStmt == 0 || *ppStmt == 0); /* VERIFY: F13021 */ > return rc; > } > + > +void > +sql_parser_free(Parse *parser) > +{ > + if (parser == NULL) > + return; 34. It is never == NULL as I can see. > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index 5a50413..32a8e08 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -1661,14 +1661,15 @@ columnTypeImpl(NameContext * pNC, Expr * pExpr, > } else if (pTab->pSchema) { > /* A real table */ > assert(!pS); > - assert(iCol >= 0 && iCol < pTab->nCol); > + assert(iCol >= 0 && iCol < > + (int)pTab->def->field_count); 35. Out of 80. > @@ -1819,11 +1818,25 @@ sqlite3ColumnsFromExprList(Parse * pParse, /* Parsing context */ > testcase(aCol == 0); > } else { > nCol = 0; > - aCol = 0; > + aCol = NULL; > } > assert(nCol == (i16) nCol); > - *pnCol = nCol; > - *paCol = aCol; > + > + /* > + * This should be a table without resolved columns. > + * sqlite3ViewGetColumnNames could use it to resolve > + * names for existent table. 36. existent -> existing. > + */ > + assert(pTable->def->fields == NULL); 37. Lets better check def->opts.is_temporary. field == NULL means nothing. > + struct region *region = &fiber()->gc; > + pTable->def->fields = > + region_alloc(region, nCol * sizeof(pTable->def->fields[0])); 38. region_alloc can fail - set diag + pParse->rc = SQL_TARANTOOL_ERROR + pParse->nErr++; or sqlite3OomFault(). Unfortunately we still have no standard way to handle OOM in SQL. > @@ -1874,22 +1887,28 @@ sqlite3ColumnsFromExprList(Parse * pParse, /* Parsing context */ > if (cnt > 3) > sqlite3_randomness(sizeof(cnt), &cnt); > } > - pCol->zName = zName; > - if (zName && sqlite3HashInsert(&ht, zName, pCol) == pCol) { > + uint32_t name_len = (uint32_t)strlen(zName); > + if (zName != NULL && sqlite3HashInsert(&ht, zName, pCol) == pCol) > sqlite3OomFault(db); > + pTable->def->fields[i].name = > + region_alloc(region, name_len + 1); > + if (pTable->def->fields[i].name == NULL) { > + sqlite3OomFault(db); > + } else { > + memcpy(pTable->def->fields[i].name, zName, name_len); > + pTable->def->fields[i].name[name_len] = '\0'; > } > } > sqlite3HashClear(&ht); > - if (db->mallocFailed) { > - for (j = 0; j < i; j++) { > - sqlite3DbFree(db, aCol[j].zName); > - } > + int rc = db->mallocFailed ? SQLITE_NOMEM_BKPT : SQLITE_OK; > + if (rc != SQLITE_OK) { > sqlite3DbFree(db, aCol); > - *paCol = 0; > - *pnCol = 0; > - return SQLITE_NOMEM_BKPT; > + pTable->def->fields = NULL; > + pTable->def->field_count = 0; 39. pTable->def is on region and is not deleted together with table. You can skip this cleaning. > @@ -4683,18 +4701,24 @@ selectExpander(Walker * pWalker, Select * p) > assert(pFrom->pTab == 0); > if (sqlite3WalkSelect(pWalker, pSel)) > return WRC_Abort; > + /* Will overwritten with pointer as unique identifier. */ 40. Out of 66. 41. Will *be* overwritten. > + const char *name = "sqlite_sq_DEADBEAFDEADBEAF"; > pFrom->pTab = pTab = > - sqlite3DbMallocZero(db, sizeof(Table)); > - if (pTab == 0) > + sql_ephemeral_table_new(pParse, name); > + if (pTab == NULL) > return WRC_Abort; > + /* rewrite old name with correct pointer */ 42. Same in the previous review - please, start a sentence from a capital letter, and finish with dot. > diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h > index 8bb45c9..4fba008 100644 > --- a/src/box/sql/sqliteInt.h > +++ b/src/box/sql/sqliteInt.h > @@ -1867,15 +1867,6 @@ struct Savepoint { > * of this structure. > */ > struct Column { > - char *zName; /* Name of this column */ > - enum field_type type; /* Column type. */ > - /** Collating sequence. */ > - struct coll *coll; > - /** > - * An ON_CONFLICT_ACTION code for handling a NOT NULL > - * constraint. > - */ > - enum on_conflict_action notNull; Niceeee. > @@ -4153,4 +4142,24 @@ table_column_nullable_action(struct Table *tab, uint32_t column); > bool > table_column_is_nullable(struct Table *tab, uint32_t column); > > +/** > + * Initialize a new parser object. > + * @param parser object to initialize. > + */ > +static inline void > +sql_parser_create(struct Parse *parser) > +{ > + memset(parser, 0, PARSE_HDR_SZ); > + memset(PARSE_TAIL(parser), 0, PARSE_TAIL_SZ); > + struct region *region = &fiber()->gc; > + parser->region_initial_size = region_used(region); > +} > + > +/** > + * Release the parser object resources. > + * @param parser object to release. > + */ > +void > +sql_parser_free(struct Parse *parser); 43. Out convention is 'create/destroy'. Please, respect it.