From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
To: Kirill Shcherbatov <kshcherbatov@tarantool.org>,
tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH v5 2/3] sql: remove SQL fields from Table and Column
Date: Fri, 11 May 2018 23:59:45 +0300 [thread overview]
Message-ID: <70dd9a56-4374-56b7-6564-7cfb0309f0b7@tarantool.org> (raw)
In-Reply-To: <b0e872bbbb844bf9527aaa481f7c76894f195332.1526028449.git.kshcherbatov@tarantool.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.
next prev parent reply other threads:[~2018-05-11 20:59 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-11 8:49 [tarantool-patches] [PATCH v5 0/3] sql: refactor SQL Parser structures Kirill Shcherbatov
2018-05-11 8:49 ` [tarantool-patches] [PATCH v5 1/3] sql: fix code style in sqlite3Pragma Kirill Shcherbatov
2018-05-11 20:59 ` [tarantool-patches] " Vladislav Shpilevoy
2018-05-11 8:49 ` [tarantool-patches] [PATCH v5 2/3] sql: remove SQL fields from Table and Column Kirill Shcherbatov
2018-05-11 20:59 ` Vladislav Shpilevoy [this message]
2018-05-14 11:20 ` [tarantool-patches] " Kirill Shcherbatov
2018-05-14 13:39 ` Vladislav Shpilevoy
2018-05-15 15:56 ` Kirill Shcherbatov
2018-05-11 8:49 ` [tarantool-patches] [PATCH v5 3/3] sql: space_def* instead of Table* in Expr Kirill Shcherbatov
2018-05-11 20:59 ` [tarantool-patches] " Vladislav Shpilevoy
2018-05-14 11:20 ` Kirill Shcherbatov
2018-05-11 8:58 ` [tarantool-patches] Re: [PATCH v5 0/3] sql: refactor SQL Parser structures Vladislav Shpilevoy
2018-05-11 19:40 ` [tarantool-patches] Re[2]: [tarantool-patches] " Kirill Shcherbatov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=70dd9a56-4374-56b7-6564-7cfb0309f0b7@tarantool.org \
--to=v.shpilevoy@tarantool.org \
--cc=kshcherbatov@tarantool.org \
--cc=tarantool-patches@freelists.org \
--subject='[tarantool-patches] Re: [PATCH v5 2/3] sql: remove SQL fields from Table and Column' \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox