[tarantool-patches] Re: [PATCH v2 1/1] Removed Expr pointer from SQL Column structure.
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Tue Apr 17 21:29:40 MSK 2018
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 <box/coll.h>
> #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.
More information about the Tarantool-patches
mailing list