* [tarantool-patches] [PATCH v2 1/1] Removed Expr pointer from SQL Column structure.
@ 2018-04-16 16:35 Kirill Shcherbatov
2018-04-16 19:23 ` [tarantool-patches] " Vladislav Shpilevoy
0 siblings, 1 reply; 11+ messages in thread
From: Kirill Shcherbatov @ 2018-04-16 16:35 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy, Kirill Shcherbatov
Introduced space_def field in SQL Table structure which
already contains Expr field.
Needed for #3051.
---
src/box/space_def.c | 2 +
src/box/sql.c | 12 ++++-
src/box/sql.h | 13 +++++
src/box/sql/alter.c | 4 +-
src/box/sql/build.c | 127 ++++++++++++++++++++++++++++++++++++++++++------
src/box/sql/fkey.c | 6 ++-
src/box/sql/insert.c | 20 +++++---
src/box/sql/pragma.c | 10 ++--
src/box/sql/sqliteInt.h | 1 +
src/box/sql/update.c | 5 +-
10 files changed, 166 insertions(+), 34 deletions(-)
diff --git a/src/box/space_def.c b/src/box/space_def.c
index 22bd3ca..5a4fd6d 100644
--- a/src/box/space_def.c
+++ b/src/box/space_def.c
@@ -239,6 +239,8 @@ space_def_destroy_fields(struct field_def *fields, uint32_t field_count)
void
space_def_delete(struct space_def *def)
{
+ if (def == NULL)
+ return;
space_opts_destroy(&def->opts);
tuple_dictionary_unref(def->dict);
space_def_destroy_fields(def->fields, def->field_count);
diff --git a/src/box/sql.c b/src/box/sql.c
index a6713f1..6418cbd 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1466,7 +1466,8 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
for (i = 0; i < n; i++) {
const char *t;
struct coll *coll = NULL;
- struct Expr *def = aCol[i].pDflt;
+ struct field_def *field = sql_field_get(pTable, i);
+ struct Expr *def = field->default_value_expr;
if (aCol[i].zColl != NULL &&
strcasecmp(aCol[i].zColl, "binary") != 0) {
coll = sqlite3FindCollSeq(aCol[i].zColl);
@@ -1711,3 +1712,12 @@ space_column_default_expr(uint32_t space_id, uint32_t fieldno)
return space->def->fields[fieldno].default_value_expr;
}
+
+struct field_def *
+sql_field_get(struct Table *pTable, int id)
+{
+ assert(pTable->def);
+ assert((uint32_t)id < pTable->def->exact_field_count);
+ assert((uint32_t)id < pTable->def->field_count);
+ return &pTable->def->fields[id];
+}
diff --git a/src/box/sql.h b/src/box/sql.h
index db92d80..d177341 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -65,6 +65,8 @@ sql_get();
struct Expr;
struct Parse;
struct Select;
+struct Table;
+struct Hash;
/**
* Perform parsing of provided expression. This is done by
@@ -143,6 +145,17 @@ 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);
+/**
+ * Get field by id.
+ * @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_get(struct Table *pTable, int id);
+
#if defined(__cplusplus)
} /* extern "C" { */
#endif
diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
index 129ef82..d2e0968 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;
+ struct field_def *field = sql_field_get(pNew, pNew->nCol - 1);
+ pDflt = field->default_value_expr;
pTab = sqlite3HashFind(&db->pSchema->tblHash, zTab);;
assert(pTab);
@@ -297,7 +298,6 @@ sqlite3AlterBeginAddColumn(Parse * pParse, SrcList * pSrc)
Column *pCol = &pNew->aCol[i];
pCol->zName = sqlite3DbStrDup(db, pCol->zName);
pCol->zColl = 0;
- pCol->pDflt = 0;
}
pNew->pSchema = db->pSchema;
pNew->addColOffset = pTab->addColOffset;
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 92f3cb6..d6033c9 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -299,7 +299,6 @@ sqlite3DeleteColumnNames(sqlite3 * db, Table * pTable)
if ((pCol = pTable->aCol) != 0) {
for (i = 0; i < pTable->nCol; i++, pCol++) {
sqlite3DbFree(db, pCol->zName);
- sql_expr_free(db, pCol->pDflt, false);
sqlite3DbFree(db, pCol->zColl);
}
sqlite3DbFree(db, pTable->aCol);
@@ -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);
+ }
sqlite3DbFree(db, pTable);
/* Verify that no lookaside memory was used by schema tables */
@@ -490,6 +495,53 @@ sqlite3PrimaryKeyIndex(Table * pTab)
return p;
}
+static Table *
+sql_table_new(Parse *pParse, char *zName, uint32_t nFields)
+{
+
+ sqlite3 *db = pParse->db;
+
+
+ Table *pTable = sqlite3DbMallocZero(db, sizeof(Table));
+ struct space_def *def = space_def_new(0 /* space id */, 0 /* user id */,
+ 0, "ephemeral",
+ strlen("ephemeral"), "memtx",
+ strlen("memtx"),
+ &space_opts_default,
+ &field_def_default,
+ 0/* length of field_def */);
+ struct field_def *fields =
+ sqlite3DbMallocZero(db,
+ nFields*sizeof(struct field_def));
+ if (pTable == NULL || def == NULL || fields == NULL) {
+ assert(db->mallocFailed);
+ space_def_delete(def);
+ sqlite3DbFree(db, fields);
+ sqlite3DbFree(db, pTable);
+ pParse->rc = SQLITE_NOMEM_BKPT;
+ pParse->nErr++;
+ return NULL;
+ }
+
+ pTable->def = def;
+ pTable->def->fields = fields;
+ for (uint32_t i = 0; i < nFields; i++)
+ memcpy(&def->fields[i], &field_def_default,
+ sizeof(struct field_def));
+ /* store allocated fields count */
+ def->exact_field_count = nFields;
+
+ pTable->zName = zName;
+ pTable->iPKey = -1;
+ pTable->iAutoIncPKey = -1;
+ pTable->pSchema = db->pSchema;
+ sqlite3HashInit(&pTable->idxHash);
+ pTable->nTabRef = 1;
+ pTable->nRowLogEst = 200;
+ assert(200 == sqlite3LogEst(1048576));
+ return pTable;
+}
+
/*
* Begin constructing a new table representation in memory. This is
* the first of several action routines that get called in response
@@ -547,21 +599,10 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
goto begin_table_error;
}
- pTable = sqlite3DbMallocZero(db, sizeof(Table));
- if (pTable == 0) {
- assert(db->mallocFailed);
- pParse->rc = SQLITE_NOMEM_BKPT;
- pParse->nErr++;
+ pTable = sql_table_new(pParse, zName, 1);
+ if (pTable == NULL)
goto begin_table_error;
- }
- pTable->zName = zName;
- pTable->iPKey = -1;
- pTable->iAutoIncPKey = -1;
- pTable->pSchema = db->pSchema;
- sqlite3HashInit(&pTable->idxHash);
- pTable->nTabRef = 1;
- pTable->nRowLogEst = 200;
- assert(200 == sqlite3LogEst(1048576));
+
assert(pParse->pNewTable == 0);
pParse->pNewTable = pTable;
@@ -585,6 +626,49 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
return;
}
+/**
+ * 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 && pTable->def->fields);
+ assert(pTable->def->exact_field_count >= (uint32_t)pTable->nCol);
+ assert(id < pTable->def->exact_field_count * 2);
+ assert(id < (uint32_t)db->aLimit[SQLITE_LIMIT_COLUMN]);
+
+ if (id >= pTable->def->exact_field_count) {
+ field =
+ sqlite3DbRealloc(db, pTable->def->fields,
+ pTable->def->exact_field_count * 2
+ * sizeof(pTable->def->fields[0]));
+ if (field == NULL) {
+ assert(db->mallocFailed);
+ pParse->rc = SQLITE_NOMEM_BKPT;
+ pParse->nErr++;
+ return NULL;
+ }
+
+ for (uint32_t i = pTable->def->exact_field_count;
+ i < 2*pTable->def->exact_field_count; i++)
+ memcpy(&field[i], &field_def_default,
+ sizeof(struct field_def));
+
+ pTable->def->fields = field;
+ pTable->def->exact_field_count *= 2;
+ }
+
+ field = &pTable->def->fields[id];
+ return field;
+}
+
/*
* Add a new column to the table currently being constructed.
*
@@ -610,6 +694,8 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType)
return;
}
#endif
+ if (sql_field_retrieve(pParse, p, (uint32_t) p->nCol) == NULL)
+ return;
z = sqlite3DbMallocRaw(db, pName->n + 1);
if (z == 0)
return;
@@ -668,6 +754,7 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType)
}
}
p->nCol++;
+ p->def->field_count++;
pParse->constraintName.n = 0;
}
@@ -813,7 +900,11 @@ sqlite3AddDefaultValue(Parse * pParse, ExprSpan * pSpan)
* is required by pragma table_info.
*/
Expr x;
- sql_expr_free(db, pCol->pDflt, false);
+
+ struct field_def *field =
+ sql_field_get(p, (uint32_t) 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,
@@ -821,6 +912,10 @@ sqlite3AddDefaultValue(Parse * pParse, ExprSpan * pSpan)
pSpan->zStart));
x.pLeft = pSpan->pExpr;
x.flags = EP_Skip;
+
+ field->default_value_expr =
+ sqlite3ExprDup(db, &x, EXPRDUP_REDUCE);
+
pCol->pDflt = sqlite3ExprDup(db, &x, EXPRDUP_REDUCE);
sqlite3DbFree(db, x.u.zToken);
}
diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
index f56b6d9..625cf3c 100644
--- a/src/box/sql/fkey.c
+++ b/src/box/sql/fkey.c
@@ -1346,8 +1346,10 @@ fkActionTrigger(Parse * pParse, /* Parse context */
&tToCol,
0));
} else if (action == OE_SetDflt) {
- Expr *pDflt =
- pFKey->pFrom->aCol[iFromCol].pDflt;
+ struct field_def *field =
+ sql_field_get(pFKey->pFrom,
+ iFromCol);
+ Expr *pDflt = field->default_value_expr;
if (pDflt) {
pNew =
sqlite3ExprDup(db, pDflt,
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index b24d55b..f6db89a 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -1801,14 +1801,18 @@ xferOptimization(Parse * pParse, /* Parser context */
}
/* Default values for second and subsequent columns need to match. */
if (i > 0) {
- assert(pDestCol->pDflt == 0
- || pDestCol->pDflt->op == TK_SPAN);
- assert(pSrcCol->pDflt == 0
- || pSrcCol->pDflt->op == TK_SPAN);
- if ((pDestCol->pDflt == 0) != (pSrcCol->pDflt == 0)
- || (pDestCol->pDflt
- && strcmp(pDestCol->pDflt->u.zToken,
- pSrcCol->pDflt->u.zToken) != 0)
+ struct field_def *pSrcField = sql_field_get(pSrc, i);
+ struct field_def *pDestField = sql_field_get(pSrc, i);
+ Expr *pSrcDflt = pSrcField->default_value_expr;
+ Expr *pDestDflt = pDestField->default_value_expr;
+ assert(pDestDflt == 0
+ || pDestDflt->op == TK_SPAN);
+ assert(pSrcDflt == 0
+ || pSrcDflt->op == TK_SPAN);
+ if ((pDestDflt == 0) != (pSrcDflt == 0)
+ || (pDestDflt
+ && strcmp(pDestDflt->u.zToken,
+ pSrcDflt->u.zToken) != 0)
) {
return 0; /* Default values must be the same for all columns */
}
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index b724c98..c0bf7fd 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -359,6 +359,9 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */
sqlite3ViewGetColumnNames(pParse, pTab);
for (i = 0, pCol = pTab->aCol; i < pTab->nCol;
i++, pCol++) {
+ struct field_def *field =
+ sql_field_get(pTab, i);
+ Expr *pDflt = field->default_value_expr;
if (!table_column_is_in_pk(pTab, i)) {
k = 0;
} else if (pPk == 0) {
@@ -370,8 +373,8 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */
i; k++) {
}
}
- assert(pCol->pDflt == 0
- || pCol->pDflt->op == TK_SPAN);
+ assert(pDflt == 0
+ || pDflt->op == TK_SPAN);
bool nullable = table_column_is_nullable(pTab, i);
sqlite3VdbeMultiLoad(v, 1, "issisi",
i, pCol->zName,
@@ -379,8 +382,7 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */
sqlite3ColumnType
(pCol)],
nullable == 0,
- pCol->
- pDflt ? pCol->
+ pDflt ?
pDflt->u.zToken
: 0, k);
sqlite3VdbeAddOp2(v, OP_ResultRow, 1,
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 59662cf..9a7d99c 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -1970,6 +1970,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;
};
/*
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index 83c05ab..f6aa24b 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -75,7 +75,10 @@ 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 = pTab->def ?
+ sql_field_get(pTab, i)->default_value_expr : NULL;
+ sqlite3ValueFromExpr(sqlite3VdbeDb(v),
+ expr,
pCol->affinity, &pValue);
if (pValue) {
sqlite3VdbeAppendP4(v, pValue, P4_MEM);
--
2.7.4
^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH v2 1/1] Removed Expr pointer from SQL Column structure.
2018-04-16 16:35 [tarantool-patches] [PATCH v2 1/1] Removed Expr pointer from SQL Column structure Kirill Shcherbatov
@ 2018-04-16 19:23 ` Vladislav Shpilevoy
2018-04-17 11:02 ` Kirill Shcherbatov
0 siblings, 1 reply; 11+ messages in thread
From: Vladislav Shpilevoy @ 2018-04-16 19:23 UTC (permalink / raw)
To: tarantool-patches, Kirill Shcherbatov
Hello. Thanks for the patch, it looks almost ok, your patches become
better and better! See my 11 comments below.
On 16/04/2018 19:35, Kirill Shcherbatov wrote:
> Introduced space_def field in SQL Table structure which
> already contains Expr field.
>
> Needed for #3051.
1. Please, use this guide to write commit messages:
https://tarantool.io/en/doc/1.9/dev_guide/developer_guidelines.html#how-to-write-a-commit-message
In particular this:
- limit the subject line to 50 characters,
- prefix the subject with a subsystem name. Here it is 'sql: ',
- do not end the subject line with a period,
- use the imperative mood in the subject line.
> diff --git a/src/box/space_def.c b/src/box/space_def.c
> index 22bd3ca..5a4fd6d 100644
> --- a/src/box/space_def.c
> +++ b/src/box/space_def.c
> @@ -239,6 +239,8 @@ space_def_destroy_fields(struct field_def *fields, uint32_t field_count)
> void
> space_def_delete(struct space_def *def)
> {
> + if (def == NULL)
> + return;
2. The space_def_delete must not be called with NULL. Please, fix the
caller function instead of space_def_delete.
> space_opts_destroy(&def->opts);
> tuple_dictionary_unref(def->dict);
> space_def_destroy_fields(def->fields, def->field_count);
> diff --git a/src/box/sql.c b/src/box/sql.c
> index a6713f1..6418cbd 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -1466,7 +1466,8 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
> for (i = 0; i < n; i++) {
> const char *t;
> struct coll *coll = NULL;
> - struct Expr *def = aCol[i].pDflt;
> + struct field_def *field = sql_field_get(pTable, i);
3. The wrapper for assertions only is useless. Please, inline it with
no these obvious assertions.
> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
> index 129ef82..d2e0968 100644
> --- a/src/box/sql/alter.c
> +++ b/src/box/sql/alter.c
> @@ -297,7 +298,6 @@ sqlite3AlterBeginAddColumn(Parse * pParse, SrcList * pSrc)
> Column *pCol = &pNew->aCol[i];
> pCol->zName = sqlite3DbStrDup(db, pCol->zName);
> pCol->zColl = 0;
> - pCol->pDflt = 0;
4. I jumped around the patch and it looks, that you forgot the main part
of it - removal of struct Column.pDflt. Thanks to you, now it is very
easy.
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 92f3cb6..d6033c9 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -490,6 +495,53 @@ sqlite3PrimaryKeyIndex(Table * pTab)
> return p;
> }
>
> +static Table *
> +sql_table_new(Parse *pParse, char *zName, uint32_t nFields)
5. Why do you need nFields argument? It is always 1. And I
can not understand why 1? When a parser sees 'CREATE TABLE name (', it
does not know even about a first column. Please, write a comments for
the function.
> +{
> +
> + sqlite3 *db = pParse->db;
> +
> +
> + Table *pTable = sqlite3DbMallocZero(db, sizeof(Table));
> + struct space_def *def = space_def_new(0 /* space id */, 0 /* user id */,
> + 0, "ephemeral",
> + strlen("ephemeral"), "memtx",
> + strlen("memtx"),
> + &space_opts_default,
> + &field_def_default,
> + 0/* length of field_def */);
6. Looks like you copy-pasted this code. The space_def is created for
defaults only, and it must be mentioned here in comments. Other
space_def fields are dummy and invalid. It means that its name must be
NULL, engine must be NULL, fields array must be NULL. And do not put
comments on each argument - it must be done in function declaration, but
not on each function call. Ok? Please, fix it. I do not know, why
ephemeral tables violate these rules(
> @@ -585,6 +626,49 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
> return;
> }
>
> +/**
> + * 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)
Off topic: good name.
> diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
> index f56b6d9..625cf3c 100644
> --- a/src/box/sql/fkey.c
> +++ b/src/box/sql/fkey.c
> @@ -1346,8 +1346,10 @@ fkActionTrigger(Parse * pParse, /* Parse context */
> &tToCol,
> 0));
> } else if (action == OE_SetDflt) {
> - Expr *pDflt =
> - pFKey->pFrom->aCol[iFromCol].pDflt;
> + struct field_def *field =
> + sql_field_get(pFKey->pFrom,
> + iFromCol);
7. It is not DDL, so here please use space_def from the space, not from
the table. Use here space_column_default_expr.
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index b24d55b..f6db89a 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -1801,14 +1801,18 @@ xferOptimization(Parse * pParse, /* Parser context */
> }
> /* Default values for second and subsequent columns need to match. */
> if (i > 0) {
> - assert(pDestCol->pDflt == 0
> - || pDestCol->pDflt->op == TK_SPAN);
> - assert(pSrcCol->pDflt == 0
> - || pSrcCol->pDflt->op == TK_SPAN);
> - if ((pDestCol->pDflt == 0) != (pSrcCol->pDflt == 0)
> - || (pDestCol->pDflt
> - && strcmp(pDestCol->pDflt->u.zToken,
> - pSrcCol->pDflt->u.zToken) != 0)
> + struct field_def *pSrcField = sql_field_get(pSrc, i);
> + struct field_def *pDestField = sql_field_get(pSrc, i);
8. Same as 7.
> diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
> index b724c98..c0bf7fd 100644
> --- a/src/box/sql/pragma.c
> +++ b/src/box/sql/pragma.c
> @@ -359,6 +359,9 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */
> sqlite3ViewGetColumnNames(pParse, pTab);
> for (i = 0, pCol = pTab->aCol; i < pTab->nCol;
> i++, pCol++) {
> + struct field_def *field =
> + sql_field_get(pTab, i);
> + Expr *pDflt = field->default_value_expr;
9. Same as 7.
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 59662cf..9a7d99c 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -1970,6 +1970,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;
10. Please, write a comment, for what this member is used, and what
stores.
> };
>
> /*
> diff --git a/src/box/sql/update.c b/src/box/sql/update.c
> index 83c05ab..f6aa24b 100644
> --- a/src/box/sql/update.c
> +++ b/src/box/sql/update.c
> @@ -75,7 +75,10 @@ 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 = pTab->def ?
> + sql_field_get(pTab, i)->default_value_expr : NULL;
> + sqlite3ValueFromExpr(sqlite3VdbeDb(v),
> + expr,
> pCol->affinity, &pValue);
11. Same as 7.
> if (pValue) {
> sqlite3VdbeAppendP4(v, pValue, P4_MEM);
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH v2 1/1] Removed Expr pointer from SQL Column structure.
2018-04-16 19:23 ` [tarantool-patches] " Vladislav Shpilevoy
@ 2018-04-17 11:02 ` Kirill Shcherbatov
2018-04-17 18:29 ` Vladislav Shpilevoy
0 siblings, 1 reply; 11+ messages in thread
From: Kirill Shcherbatov @ 2018-04-17 11:02 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
New commit message:
sql: remove Expr pointer from SQL Column struct
Introduced space_def field in SQL Table structure which
already contains Expr field as it is required by
global Parser refactoring.
Needed for #3051.
From 876a96547dfe69af760bfc39bdd0759b8dcadd1f Mon Sep 17 00:00:00 2001
From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Date: Tue, 17 Apr 2018 13:57:10 +0300
Subject: [PATCH] Accounted Vald's commits.
---
src/box/space_def.c | 2 --
src/box/sql.c | 11 +--------
src/box/sql.h | 11 ---------
src/box/sql/alter.c | 4 ++--
src/box/sql/build.c | 60 +++++++++++++++++++++----------------------------
src/box/sql/fkey.c | 12 ++++++----
src/box/sql/insert.c | 27 ++++++++++++----------
src/box/sql/pragma.c | 18 ++++++++-------
src/box/sql/sqliteInt.h | 3 +--
src/box/sql/update.c | 16 +++++++++++--
10 files changed, 77 insertions(+), 87 deletions(-)
diff --git a/src/box/space_def.c b/src/box/space_def.c
index 5a4fd6d..22bd3ca 100644
--- a/src/box/space_def.c
+++ b/src/box/space_def.c
@@ -239,8 +239,6 @@ space_def_destroy_fields(struct field_def *fields, uint32_t field_count)
void
space_def_delete(struct space_def *def)
{
- if (def == NULL)
- return;
space_opts_destroy(&def->opts);
tuple_dictionary_unref(def->dict);
space_def_destroy_fields(def->fields, def->field_count);
diff --git a/src/box/sql.c b/src/box/sql.c
index 6418cbd..6bc82c2 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1466,7 +1466,7 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
for (i = 0; i < n; i++) {
const char *t;
struct coll *coll = NULL;
- struct field_def *field = sql_field_get(pTable, i);
+ struct field_def *field = &pTable->def->fields[i];
struct Expr *def = field->default_value_expr;
if (aCol[i].zColl != NULL &&
strcasecmp(aCol[i].zColl, "binary") != 0) {
@@ -1712,12 +1712,3 @@ space_column_default_expr(uint32_t space_id, uint32_t fieldno)
return space->def->fields[fieldno].default_value_expr;
}
-
-struct field_def *
-sql_field_get(struct Table *pTable, int id)
-{
- assert(pTable->def);
- assert((uint32_t)id < pTable->def->exact_field_count);
- assert((uint32_t)id < pTable->def->field_count);
- return &pTable->def->fields[id];
-}
diff --git a/src/box/sql.h b/src/box/sql.h
index d177341..54981a3 100644
--- a/src/box/sql.h
+++ b/src/box/sql.h
@@ -145,17 +145,6 @@ 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);
-/**
- * Get field by id.
- * @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_get(struct Table *pTable, int id);
-
#if defined(__cplusplus)
} /* extern "C" { */
#endif
diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
index d2e0968..39ae070 100644
--- a/src/box/sql/alter.c
+++ b/src/box/sql/alter.c
@@ -161,8 +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];
- struct field_def *field = sql_field_get(pNew, pNew->nCol - 1);
- pDflt = field->default_value_expr;
+ assert(pNew->def);
+ pDflt = pNew->def->fields[pNew->nCol - 1].default_value_expr;
pTab = sqlite3HashFind(&db->pSchema->tblHash, zTab);;
assert(pTab);
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index d6033c9..394ee3a 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -495,28 +495,26 @@ 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, uint32_t nFields)
+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 /* space id */, 0 /* user id */,
- 0, "ephemeral",
- strlen("ephemeral"), "memtx",
- strlen("memtx"),
+ struct space_def *def = space_def_new(0, 0, 0, NULL, 0, NULL, 0,
&space_opts_default,
- &field_def_default,
- 0/* length of field_def */);
- struct field_def *fields =
- sqlite3DbMallocZero(db,
- nFields*sizeof(struct field_def));
- if (pTable == NULL || def == NULL || fields == NULL) {
+ &field_def_default, 0);
+ if (pTable == NULL || def == NULL) {
assert(db->mallocFailed);
- space_def_delete(def);
- sqlite3DbFree(db, fields);
+ if (def != NULL)
+ space_def_delete(def);
sqlite3DbFree(db, pTable);
pParse->rc = SQLITE_NOMEM_BKPT;
pParse->nErr++;
@@ -524,12 +522,10 @@ sql_table_new(Parse *pParse, char *zName, uint32_t nFields)
}
pTable->def = def;
- pTable->def->fields = fields;
- for (uint32_t i = 0; i < nFields; i++)
- memcpy(&def->fields[i], &field_def_default,
- sizeof(struct field_def));
/* store allocated fields count */
- def->exact_field_count = nFields;
+ def->exact_field_count = 0;
+ def->field_count = 0;
+ pTable->def->fields = NULL;
pTable->zName = zName;
pTable->iPKey = -1;
@@ -599,7 +595,7 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
goto begin_table_error;
}
- pTable = sql_table_new(pParse, zName, 1);
+ pTable = sql_table_new(pParse, zName);
if (pTable == NULL)
goto begin_table_error;
@@ -639,16 +635,15 @@ sql_field_retrieve(Parse *pParse, Table *pTable, uint32_t id)
{
sqlite3 *db = pParse->db;
struct field_def *field;
- assert(pTable->def && pTable->def->fields);
+ assert(pTable->def);
assert(pTable->def->exact_field_count >= (uint32_t)pTable->nCol);
- assert(id < pTable->def->exact_field_count * 2);
assert(id < (uint32_t)db->aLimit[SQLITE_LIMIT_COLUMN]);
if (id >= pTable->def->exact_field_count) {
- field =
- sqlite3DbRealloc(db, pTable->def->fields,
- pTable->def->exact_field_count * 2
- * sizeof(pTable->def->fields[0]));
+ uint32_t nCol = pTable->def->exact_field_count;
+ nCol = (nCol > 0) ? 2*nCol : 1;
+ field = sqlite3DbRealloc(db, pTable->def->fields,
+ nCol * sizeof(pTable->def->fields[0]));
if (field == NULL) {
assert(db->mallocFailed);
pParse->rc = SQLITE_NOMEM_BKPT;
@@ -656,13 +651,12 @@ sql_field_retrieve(Parse *pParse, Table *pTable, uint32_t id)
return NULL;
}
- for (uint32_t i = pTable->def->exact_field_count;
- i < 2*pTable->def->exact_field_count; i++)
+ for (uint32_t i = nCol/2; i < nCol; i++)
memcpy(&field[i], &field_def_default,
sizeof(struct field_def));
pTable->def->fields = field;
- pTable->def->exact_field_count *= 2;
+ pTable->def->exact_field_count = nCol;
}
field = &pTable->def->fields[id];
@@ -900,9 +894,9 @@ sqlite3AddDefaultValue(Parse * pParse, ExprSpan * pSpan)
* is required by pragma table_info.
*/
Expr x;
-
+ assert(p->def);
struct field_def *field =
- sql_field_get(p, (uint32_t) p->nCol - 1);
+ &p->def->fields[p->nCol - 1];
sql_expr_free(db, field->default_value_expr, false);
memset(&x, 0, sizeof(x));
@@ -915,8 +909,6 @@ sqlite3AddDefaultValue(Parse * pParse, ExprSpan * pSpan)
field->default_value_expr =
sqlite3ExprDup(db, &x, EXPRDUP_REDUCE);
-
- pCol->pDflt = sqlite3ExprDup(db, &x, EXPRDUP_REDUCE);
sqlite3DbFree(db, x.u.zToken);
}
}
diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
index 625cf3c..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"
+#include "tarantoolInt.h"
#ifndef SQLITE_OMIT_FOREIGN_KEY
#ifndef SQLITE_OMIT_TRIGGER
@@ -1346,10 +1348,12 @@ fkActionTrigger(Parse * pParse, /* Parse context */
&tToCol,
0));
} else if (action == OE_SetDflt) {
- struct field_def *field =
- sql_field_get(pFKey->pFrom,
- iFromCol);
- Expr *pDflt = field->default_value_expr;
+ uint32_t space_id =
+ SQLITE_PAGENO_TO_SPACEID(
+ pFKey->pFrom->tnum);
+ Expr *pDflt =
+ space_column_default_expr(
+ space_id, (uint32_t)iFromCol);
if (pDflt) {
pNew =
sqlite3ExprDup(db, pDflt,
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index f6db89a..067f50a 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -1801,18 +1801,21 @@ xferOptimization(Parse * pParse, /* Parser context */
}
/* Default values for second and subsequent columns need to match. */
if (i > 0) {
- struct field_def *pSrcField = sql_field_get(pSrc, i);
- struct field_def *pDestField = sql_field_get(pSrc, i);
- Expr *pSrcDflt = pSrcField->default_value_expr;
- Expr *pDestDflt = pDestField->default_value_expr;
- assert(pDestDflt == 0
- || pDestDflt->op == TK_SPAN);
- assert(pSrcDflt == 0
- || pSrcDflt->op == TK_SPAN);
- if ((pDestDflt == 0) != (pSrcDflt == 0)
- || (pDestDflt
- && strcmp(pDestDflt->u.zToken,
- pSrcDflt->u.zToken) != 0)
+ uint32_t src_space_id =
+ SQLITE_PAGENO_TO_SPACEID(pSrc->tnum);
+ struct space *src_space =
+ space_cache_find(src_space_id);
+ uint32_t dest_space_id =
+ SQLITE_PAGENO_TO_SPACEID(pDest->tnum);
+ struct space *dest_space =
+ space_cache_find(dest_space_id);
+ char *src_expr_str =
+ src_space->def->fields[i].default_value;
+ char *dest_expr_str =
+ dest_space->def->fields[i].default_value;
+ if ((dest_expr_str == NULL) != (src_expr_str == NULL)
+ || (dest_expr_str
+ && strcmp(src_expr_str, dest_expr_str) != 0)
) {
return 0; /* Default values must be the same for all columns */
}
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index c0bf7fd..3ca94a1 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -36,6 +36,7 @@
#include <box/index.h>
#include <box/box.h>
#include <box/tuple.h>
+#include "box/schema.h"
#include "sqliteInt.h"
#include "vdbeInt.h"
#include "box/session.h"
@@ -64,6 +65,7 @@
* ../tool/mkpragmatab.tcl.
*/
#include "pragma.h"
+#include "tarantoolInt.h"
/*
* Interpret the given string as a safety level. Return 0 for OFF,
@@ -359,9 +361,6 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */
sqlite3ViewGetColumnNames(pParse, pTab);
for (i = 0, pCol = pTab->aCol; i < pTab->nCol;
i++, pCol++) {
- struct field_def *field =
- sql_field_get(pTab, i);
- Expr *pDflt = field->default_value_expr;
if (!table_column_is_in_pk(pTab, i)) {
k = 0;
} else if (pPk == 0) {
@@ -373,18 +372,21 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */
i; k++) {
}
}
- assert(pDflt == 0
- || pDflt->op == TK_SPAN);
bool nullable = table_column_is_nullable(pTab, i);
+ uint32_t space_id =
+ SQLITE_PAGENO_TO_SPACEID(
+ pTab->tnum);
+ struct space *space =
+ space_cache_find(space_id);
+ char *expr_str = space->
+ def->fields[i].default_value;
sqlite3VdbeMultiLoad(v, 1, "issisi",
i, pCol->zName,
field_type_strs[
sqlite3ColumnType
(pCol)],
nullable == 0,
- pDflt ?
- pDflt->u.zToken
- : 0, k);
+ expr_str, k);
sqlite3VdbeAddOp2(v, OP_ResultRow, 1,
6);
}
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 9a7d99c..90b7e08 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -1877,7 +1877,6 @@ struct Savepoint {
struct Column {
char *zName; /* Name of this column */
enum field_type type; /* Column type. */
- Expr *pDflt; /* Default value of this column */
char *zColl; /* Collating sequence. If NULL, use the default */
enum on_conflict_action notNull; /* An ON_CONFLICT_ACTION code for
* handling a NOT NULL constraint
@@ -1970,7 +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;
+ struct space_def *def; /* Space definition with Tarantool metadata */
};
/*
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index f6aa24b..2505a5d 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,8 +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);
- Expr *expr = pTab->def ?
- sql_field_get(pTab, i)->default_value_expr : NULL;
+
+ 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) {
+ expr = space->def->fields[i].default_value_expr;
+ } else {
+ expr = pTab->def ?
+ pTab->def->fields[i].default_value_expr : NULL;
+ }
+
sqlite3ValueFromExpr(sqlite3VdbeDb(v),
expr,
pCol->affinity, &pValue);
--
2.7.4
On 16.04.2018 22:23, Vladislav Shpilevoy wrote:
> Hello. Thanks for the patch, it looks almost ok, your patches become
> better and better! See my 11 comments below.
>
>
> On 16/04/2018 19:35, Kirill Shcherbatov wrote:
>> Introduced space_def field in SQL Table structure which
>> already contains Expr field.
>>
>> Needed for #3051.
>
> 1. Please, use this guide to write commit messages:
> https://tarantool.io/en/doc/1.9/dev_guide/developer_guidelines.html#how-to-write-a-commit-message
>
> In particular this:
> - limit the subject line to 50 characters,
> - prefix the subject with a subsystem name. Here it is 'sql: ',
> - do not end the subject line with a period,
> - use the imperative mood in the subject line.
>
>> diff --git a/src/box/space_def.c b/src/box/space_def.c
>> index 22bd3ca..5a4fd6d 100644
>> --- a/src/box/space_def.c
>> +++ b/src/box/space_def.c
>> @@ -239,6 +239,8 @@ space_def_destroy_fields(struct field_def *fields, uint32_t field_count)
>> void
>> space_def_delete(struct space_def *def)
>> {
>> + if (def == NULL)
>> + return;
>
> 2. The space_def_delete must not be called with NULL. Please, fix the
> caller function instead of space_def_delete.
>
>> space_opts_destroy(&def->opts);
>> tuple_dictionary_unref(def->dict);
>> space_def_destroy_fields(def->fields, def->field_count);
>> diff --git a/src/box/sql.c b/src/box/sql.c
>> index a6713f1..6418cbd 100644
>> --- a/src/box/sql.c
>> +++ b/src/box/sql.c
>> @@ -1466,7 +1466,8 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
>> for (i = 0; i < n; i++) {
>> const char *t;
>> struct coll *coll = NULL;
>> - struct Expr *def = aCol[i].pDflt;
>> + struct field_def *field = sql_field_get(pTable, i);
>
> 3. The wrapper for assertions only is useless. Please, inline it with
> no these obvious assertions.
>
>> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
>> index 129ef82..d2e0968 100644
>> --- a/src/box/sql/alter.c
>> +++ b/src/box/sql/alter.c
>> @@ -297,7 +298,6 @@ sqlite3AlterBeginAddColumn(Parse * pParse, SrcList * pSrc)
>> Column *pCol = &pNew->aCol[i];
>> pCol->zName = sqlite3DbStrDup(db, pCol->zName);
>> pCol->zColl = 0;
>> - pCol->pDflt = 0;
>
> 4. I jumped around the patch and it looks, that you forgot the main part
> of it - removal of struct Column.pDflt. Thanks to you, now it is very
> easy.
>> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
>> index 92f3cb6..d6033c9 100644
>> --- a/src/box/sql/build.c
>> +++ b/src/box/sql/build.c
>> @@ -490,6 +495,53 @@ sqlite3PrimaryKeyIndex(Table * pTab)
>> return p;
>> }
>>
>> +static Table *
>> +sql_table_new(Parse *pParse, char *zName, uint32_t nFields)
>
> 5. Why do you need nFields argument? It is always 1. And I
> can not understand why 1? When a parser sees 'CREATE TABLE name (', it
> does not know even about a first column. Please, write a comments for
> the function.
>
>> +{
>> +
>> + sqlite3 *db = pParse->db;
>> +
>> +
>> + Table *pTable = sqlite3DbMallocZero(db, sizeof(Table));
>> + struct space_def *def = space_def_new(0 /* space id */, 0 /* user id */,
>> + 0, "ephemeral",
>> + strlen("ephemeral"), "memtx",
>> + strlen("memtx"),
>> + &space_opts_default,
>> + &field_def_default,
>> + 0/* length of field_def */);
>
> 6. Looks like you copy-pasted this code. The space_def is created for
> defaults only, and it must be mentioned here in comments. Other
> space_def fields are dummy and invalid. It means that its name must be
> NULL, engine must be NULL, fields array must be NULL. And do not put
> comments on each argument - it must be done in function declaration, but
> not on each function call. Ok? Please, fix it. I do not know, why
> ephemeral tables violate these rules(
>> @@ -585,6 +626,49 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
>> return;
>> }
>>
>> +/**
>> + * 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)
>
> Off topic: good name.
>
>> diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
>> index f56b6d9..625cf3c 100644
>> --- a/src/box/sql/fkey.c
>> +++ b/src/box/sql/fkey.c
>> @@ -1346,8 +1346,10 @@ fkActionTrigger(Parse * pParse, /* Parse context */
>> &tToCol,
>> 0));
>> } else if (action == OE_SetDflt) {
>> - Expr *pDflt =
>> - pFKey->pFrom->aCol[iFromCol].pDflt;
>> + struct field_def *field =
>> + sql_field_get(pFKey->pFrom,
>> + iFromCol);
>
> 7. It is not DDL, so here please use space_def from the space, not from
> the table. Use here space_column_default_expr.
>> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
>> index b24d55b..f6db89a 100644
>> --- a/src/box/sql/insert.c
>> +++ b/src/box/sql/insert.c
>> @@ -1801,14 +1801,18 @@ xferOptimization(Parse * pParse, /* Parser context */
>> }
>> /* Default values for second and subsequent columns need to match. */
>> if (i > 0) {
>> - assert(pDestCol->pDflt == 0
>> - || pDestCol->pDflt->op == TK_SPAN);
>> - assert(pSrcCol->pDflt == 0
>> - || pSrcCol->pDflt->op == TK_SPAN);
>> - if ((pDestCol->pDflt == 0) != (pSrcCol->pDflt == 0)
>> - || (pDestCol->pDflt
>> - && strcmp(pDestCol->pDflt->u.zToken,
>> - pSrcCol->pDflt->u.zToken) != 0)
>> + struct field_def *pSrcField = sql_field_get(pSrc, i);
>> + struct field_def *pDestField = sql_field_get(pSrc, i);
>
> 8. Same as 7.
>> diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
>> index b724c98..c0bf7fd 100644
>> --- a/src/box/sql/pragma.c
>> +++ b/src/box/sql/pragma.c
>> @@ -359,6 +359,9 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */
>> sqlite3ViewGetColumnNames(pParse, pTab);
>> for (i = 0, pCol = pTab->aCol; i < pTab->nCol;
>> i++, pCol++) {
>> + struct field_def *field =
>> + sql_field_get(pTab, i);
>> + Expr *pDflt = field->default_value_expr;
>
> 9. Same as 7.
>> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
>> index 59662cf..9a7d99c 100644
>> --- a/src/box/sql/sqliteInt.h
>> +++ b/src/box/sql/sqliteInt.h
>> @@ -1970,6 +1970,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;
>
> 10. Please, write a comment, for what this member is used, and what
> stores.
>
>
>> };
>>
>> /*
>> diff --git a/src/box/sql/update.c b/src/box/sql/update.c
>> index 83c05ab..f6aa24b 100644
>> --- a/src/box/sql/update.c
>> +++ b/src/box/sql/update.c
>> @@ -75,7 +75,10 @@ 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 = pTab->def ?
>> + sql_field_get(pTab, i)->default_value_expr : NULL;
>> + sqlite3ValueFromExpr(sqlite3VdbeDb(v),
>> + expr,
>> pCol->affinity, &pValue);
>
> 11. Same as 7.
>
>> if (pValue) {
>> sqlite3VdbeAppendP4(v, pValue, P4_MEM);
>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH v2 1/1] Removed Expr pointer from SQL Column structure.
2018-04-17 11:02 ` Kirill Shcherbatov
@ 2018-04-17 18:29 ` Vladislav Shpilevoy
2018-04-17 18:30 ` Vladislav Shpilevoy
0 siblings, 1 reply; 11+ messages in thread
From: Vladislav Shpilevoy @ 2018-04-17 18:29 UTC (permalink / raw)
To: Kirill Shcherbatov, tarantool-patches
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH v2 1/1] Removed Expr pointer from SQL Column structure.
2018-04-17 18:29 ` Vladislav Shpilevoy
@ 2018-04-17 18:30 ` Vladislav Shpilevoy
2018-04-18 7:31 ` Kirill Shcherbatov
0 siblings, 1 reply; 11+ messages in thread
From: Vladislav Shpilevoy @ 2018-04-17 18:30 UTC (permalink / raw)
To: Kirill Shcherbatov, tarantool-patches
And please, rename the branch as I asked you - the
issue 3051 (lost tuple format) is not linked with the
problem, that we solve here.
On 17/04/2018 21:29, Vladislav Shpilevoy wrote:
> 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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH v2 1/1] Removed Expr pointer from SQL Column structure.
2018-04-17 18:30 ` Vladislav Shpilevoy
@ 2018-04-18 7:31 ` Kirill Shcherbatov
2018-04-18 9:47 ` Vladislav Shpilevoy
0 siblings, 1 reply; 11+ messages in thread
From: Kirill Shcherbatov @ 2018-04-18 7:31 UTC (permalink / raw)
To: tarantool-patches; +Cc: v.shpilevoy
From 0ed6b21e9e45974dcccfbcfe53675513ed731d0f Mon Sep 17 00:00:00 2001
From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Date: Wed, 18 Apr 2018 10:26:51 +0300
Subject: [PATCH] Fixes
---
src/box/sql/alter.c | 2 +-
src/box/sql/build.c | 20 +++++++-------------
src/box/sql/fkey.c | 1 -
src/box/sql/sqliteInt.h | 3 ++-
src/box/sql/update.c | 4 ----
5 files changed, 10 insertions(+), 20 deletions(-)
diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
index 39ae070..747d20e 100644
--- a/src/box/sql/alter.c
+++ b/src/box/sql/alter.c
@@ -161,7 +161,7 @@ sqlite3AlterFinishAddColumn(Parse * pParse, Token * pColDef)
zTab = &pNew->zName[16]; /* Skip the "sqlite_altertab_" prefix on the name */
pCol = &pNew->aCol[pNew->nCol - 1];
- assert(pNew->def);
+ assert(pNew->def != NULL);
pDflt = pNew->def->fields[pNew->nCol - 1].default_value_expr;
pTab = sqlite3HashFind(&db->pSchema->tblHash, zTab);;
assert(pTab);
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 20e9611..f3e9a7f 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -397,7 +397,7 @@ deleteTable(sqlite3 * db, Table * pTable)
sqlite3SelectDelete(db, pTable->pSelect);
sqlite3ExprListDelete(db, pTable->pCheck);
if (pTable->def) {
- /* fields has been allocated on separate region */
+ /* Fields has been allocated independently. */
struct field_def *fields = pTable->def->fields;
space_def_delete(pTable->def);
sqlite3DbFree(db, fields);
@@ -511,7 +511,6 @@ sql_table_new(Parse *pParse, char *zName)
struct space_def *def = space_def_new(0, 0, 0, NULL, 0, NULL, 0,
&space_opts_default, NULL, 0);
if (pTable == NULL || def == NULL) {
- assert(db->mallocFailed);
if (def != NULL)
space_def_delete(def);
sqlite3DbFree(db, pTable);
@@ -521,11 +520,6 @@ sql_table_new(Parse *pParse, char *zName)
}
pTable->def = def;
- /* store allocated fields count */
- def->exact_field_count = 0;
- def->field_count = 0;
- pTable->def->fields = NULL;
-
pTable->zName = zName;
pTable->iPKey = -1;
pTable->iAutoIncPKey = -1;
@@ -640,19 +634,19 @@ sql_field_retrieve(Parse *pParse, Table *pTable, uint32_t id)
if (id >= pTable->def->exact_field_count) {
uint32_t nCol = pTable->def->exact_field_count;
- nCol = (nCol > 0) ? 2*nCol : 1;
+ nCol = (nCol > 0) ? 2 * nCol : 1;
field = sqlite3DbRealloc(db, pTable->def->fields,
nCol * sizeof(pTable->def->fields[0]));
if (field == NULL) {
- assert(db->mallocFailed);
pParse->rc = SQLITE_NOMEM_BKPT;
pParse->nErr++;
return NULL;
}
- for (uint32_t i = nCol/2; i < nCol; i++)
- memcpy(&field[i], &field_def_default,
- sizeof(struct field_def));
+ for (uint32_t i = nCol / 2; i < nCol; i++) {
+ memcpy(&field[i], &field_def_default,
+ sizeof(struct field_def));
+ }
pTable->def->fields = field;
pTable->def->exact_field_count = nCol;
@@ -893,7 +887,7 @@ sqlite3AddDefaultValue(Parse * pParse, ExprSpan * pSpan)
* is required by pragma table_info.
*/
Expr x;
- assert(p->def);
+ assert(p->def != NULL);
struct field_def *field =
&p->def->fields[p->nCol - 1];
sql_expr_free(db, field->default_value_expr, false);
diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
index d196fd4..9b88b5f 100644
--- a/src/box/sql/fkey.c
+++ b/src/box/sql/fkey.c
@@ -36,7 +36,6 @@
#include <box/coll.h>
#include "sqliteInt.h"
#include "box/session.h"
-#include "box/schema.h"
#include "tarantoolInt.h"
#ifndef SQLITE_OMIT_FOREIGN_KEY
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 90b7e08..19efbf3 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -1969,7 +1969,8 @@ 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 */
+ /* Space definition with Tarantool metadata. */
+ struct space_def *def;
};
/*
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index 8d1cfdd..2743053 100644
--- a/src/box/sql/update.c
+++ b/src/box/sql/update.c
@@ -81,12 +81,8 @@ sqlite3ColumnDefault(Vdbe * v, Table * pTab, int i, int iReg)
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;
-
sqlite3ValueFromExpr(sqlite3VdbeDb(v),
expr,
pCol->affinity, &pValue);
--
2.7.4
On 17.04.2018 21:30, Vladislav Shpilevoy wrote:
> And please, rename the branch as I asked you - the
> issue 3051 (lost tuple format) is not linked with the
> problem, that we solve here.
>
> On 17/04/2018 21:29, Vladislav Shpilevoy wrote:
>> 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.
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH v2 1/1] Removed Expr pointer from SQL Column structure.
2018-04-18 7:31 ` Kirill Shcherbatov
@ 2018-04-18 9:47 ` Vladislav Shpilevoy
2018-04-19 12:58 ` n.pettik
0 siblings, 1 reply; 11+ messages in thread
From: Vladislav Shpilevoy @ 2018-04-18 9:47 UTC (permalink / raw)
To: tarantool-patches, Nikita Pettik; +Cc: Kirill Shcherbatov
Hello. Thanks for fixes. I force pushed a pair of new ones into the same commit.
Nikita, please, review the patch.
diff --git a/src/box/sql.c b/src/box/sql.c
index a6713f1f0..6bc82c280 100644
--- a/src/box/sql.c
+++ b/src/box/sql.c
@@ -1466,7 +1466,8 @@ int tarantoolSqlite3MakeTableFormat(Table *pTable, void *buf)
for (i = 0; i < n; i++) {
const char *t;
struct coll *coll = NULL;
- struct Expr *def = aCol[i].pDflt;
+ struct field_def *field = &pTable->def->fields[i];
+ struct Expr *def = field->default_value_expr;
if (aCol[i].zColl != NULL &&
strcasecmp(aCol[i].zColl, "binary") != 0) {
coll = sqlite3FindCollSeq(aCol[i].zColl);
diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
index 129ef823c..cf8be5d41 100644
--- a/src/box/sql/alter.c
+++ b/src/box/sql/alter.c
@@ -35,6 +35,7 @@
*/
#include "sqliteInt.h"
#include "src/box/session.h"
+#include "tarantoolInt.h"
/*
* The code in this file only exists if we are not omitting the
@@ -161,7 +162,9 @@ 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 != NULL);
+ pDflt = space_column_default_expr(SQLITE_PAGENO_TO_SPACEID(pNew->tnum),
+ pNew->nCol - 1);
pTab = sqlite3HashFind(&db->pSchema->tblHash, zTab);;
assert(pTab);
@@ -297,7 +300,6 @@ sqlite3AlterBeginAddColumn(Parse * pParse, SrcList * pSrc)
Column *pCol = &pNew->aCol[i];
pCol->zName = sqlite3DbStrDup(db, pCol->zName);
pCol->zColl = 0;
- pCol->pDflt = 0;
}
pNew->pSchema = db->pSchema;
pNew->addColOffset = pTab->addColOffset;
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 92f3cb607..ab6df467d 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -299,7 +299,6 @@ sqlite3DeleteColumnNames(sqlite3 * db, Table * pTable)
if ((pCol = pTable->aCol) != 0) {
for (i = 0; i < pTable->nCol; i++, pCol++) {
sqlite3DbFree(db, pCol->zName);
- sql_expr_free(db, pCol->pDflt, false);
sqlite3DbFree(db, pCol->zColl);
}
sqlite3DbFree(db, pTable->aCol);
@@ -397,6 +396,12 @@ deleteTable(sqlite3 * db, Table * pTable)
sqlite3DbFree(db, pTable->zColAff);
sqlite3SelectDelete(db, pTable->pSelect);
sqlite3ExprListDelete(db, pTable->pCheck);
+ if (pTable->def != NULL) {
+ /* Fields has been allocated independently. */
+ struct field_def *fields = pTable->def->fields;
+ space_def_delete(pTable->def);
+ sqlite3DbFree(db, fields);
+ }
sqlite3DbFree(db, pTable);
/* Verify that no lookaside memory was used by schema tables */
@@ -490,6 +495,43 @@ 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 is
+ * 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, NULL, 0);
+ if (pTable == NULL || def == NULL) {
+ if (def != NULL)
+ space_def_delete(def);
+ sqlite3DbFree(db, pTable);
+ pParse->rc = SQLITE_NOMEM_BKPT;
+ pParse->nErr++;
+ return NULL;
+ }
+
+ pTable->def = def;
+ pTable->zName = zName;
+ pTable->iPKey = -1;
+ pTable->iAutoIncPKey = -1;
+ pTable->pSchema = db->pSchema;
+ sqlite3HashInit(&pTable->idxHash);
+ pTable->nTabRef = 1;
+ pTable->nRowLogEst = 200;
+ assert(200 == sqlite3LogEst(1048576));
+ return pTable;
+}
+
/*
* Begin constructing a new table representation in memory. This is
* the first of several action routines that get called in response
@@ -547,21 +589,10 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
goto begin_table_error;
}
- pTable = sqlite3DbMallocZero(db, sizeof(Table));
- if (pTable == 0) {
- assert(db->mallocFailed);
- pParse->rc = SQLITE_NOMEM_BKPT;
- pParse->nErr++;
+ pTable = sql_table_new(pParse, zName);
+ if (pTable == NULL)
goto begin_table_error;
- }
- pTable->zName = zName;
- pTable->iPKey = -1;
- pTable->iAutoIncPKey = -1;
- pTable->pSchema = db->pSchema;
- sqlite3HashInit(&pTable->idxHash);
- pTable->nTabRef = 1;
- pTable->nRowLogEst = 200;
- assert(200 == sqlite3LogEst(1048576));
+
assert(pParse->pNewTable == 0);
pParse->pNewTable = pTable;
@@ -585,6 +616,47 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
return;
}
+/**
+ * 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;
+ field = sqlite3DbRealloc(db, pTable->def->fields,
+ nCol * sizeof(pTable->def->fields[0]));
+ if (field == NULL) {
+ pParse->rc = SQLITE_NOMEM_BKPT;
+ pParse->nErr++;
+ return NULL;
+ }
+
+ for (uint32_t i = nCol / 2; i < nCol; i++) {
+ memcpy(&field[i], &field_def_default,
+ sizeof(struct field_def));
+ }
+
+ pTable->def->fields = field;
+ pTable->def->exact_field_count = nCol;
+ }
+
+ field = &pTable->def->fields[id];
+ return field;
+}
+
/*
* Add a new column to the table currently being constructed.
*
@@ -610,6 +682,8 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType)
return;
}
#endif
+ if (sql_field_retrieve(pParse, p, (uint32_t) p->nCol) == NULL)
+ return;
z = sqlite3DbMallocRaw(db, pName->n + 1);
if (z == 0)
return;
@@ -668,6 +742,7 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType)
}
}
p->nCol++;
+ p->def->field_count++;
pParse->constraintName.n = 0;
}
@@ -813,7 +888,11 @@ sqlite3AddDefaultValue(Parse * pParse, ExprSpan * pSpan)
* is required by pragma table_info.
*/
Expr x;
- sql_expr_free(db, pCol->pDflt, false);
+ 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,
@@ -821,7 +900,9 @@ sqlite3AddDefaultValue(Parse * pParse, ExprSpan * pSpan)
pSpan->zStart));
x.pLeft = pSpan->pExpr;
x.flags = EP_Skip;
- pCol->pDflt = sqlite3ExprDup(db, &x, EXPRDUP_REDUCE);
+
+ field->default_value_expr =
+ sqlite3ExprDup(db, &x, EXPRDUP_REDUCE);
sqlite3DbFree(db, x.u.zToken);
}
}
diff --git a/src/box/sql/fkey.c b/src/box/sql/fkey.c
index f56b6d9cd..9b88b5f3a 100644
--- a/src/box/sql/fkey.c
+++ b/src/box/sql/fkey.c
@@ -36,6 +36,7 @@
#include <box/coll.h>
#include "sqliteInt.h"
#include "box/session.h"
+#include "tarantoolInt.h"
#ifndef SQLITE_OMIT_FOREIGN_KEY
#ifndef SQLITE_OMIT_TRIGGER
@@ -1346,8 +1347,12 @@ fkActionTrigger(Parse * pParse, /* Parse context */
&tToCol,
0));
} else if (action == OE_SetDflt) {
+ uint32_t space_id =
+ SQLITE_PAGENO_TO_SPACEID(
+ pFKey->pFrom->tnum);
Expr *pDflt =
- pFKey->pFrom->aCol[iFromCol].pDflt;
+ space_column_default_expr(
+ space_id, (uint32_t)iFromCol);
if (pDflt) {
pNew =
sqlite3ExprDup(db, pDflt,
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index b24d55b07..067f50a0a 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -1801,14 +1801,21 @@ xferOptimization(Parse * pParse, /* Parser context */
}
/* Default values for second and subsequent columns need to match. */
if (i > 0) {
- assert(pDestCol->pDflt == 0
- || pDestCol->pDflt->op == TK_SPAN);
- assert(pSrcCol->pDflt == 0
- || pSrcCol->pDflt->op == TK_SPAN);
- if ((pDestCol->pDflt == 0) != (pSrcCol->pDflt == 0)
- || (pDestCol->pDflt
- && strcmp(pDestCol->pDflt->u.zToken,
- pSrcCol->pDflt->u.zToken) != 0)
+ uint32_t src_space_id =
+ SQLITE_PAGENO_TO_SPACEID(pSrc->tnum);
+ struct space *src_space =
+ space_cache_find(src_space_id);
+ uint32_t dest_space_id =
+ SQLITE_PAGENO_TO_SPACEID(pDest->tnum);
+ struct space *dest_space =
+ space_cache_find(dest_space_id);
+ char *src_expr_str =
+ src_space->def->fields[i].default_value;
+ char *dest_expr_str =
+ dest_space->def->fields[i].default_value;
+ if ((dest_expr_str == NULL) != (src_expr_str == NULL)
+ || (dest_expr_str
+ && strcmp(src_expr_str, dest_expr_str) != 0)
) {
return 0; /* Default values must be the same for all columns */
}
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index b724c9845..3ca94a1c3 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -36,6 +36,7 @@
#include <box/index.h>
#include <box/box.h>
#include <box/tuple.h>
+#include "box/schema.h"
#include "sqliteInt.h"
#include "vdbeInt.h"
#include "box/session.h"
@@ -64,6 +65,7 @@
* ../tool/mkpragmatab.tcl.
*/
#include "pragma.h"
+#include "tarantoolInt.h"
/*
* Interpret the given string as a safety level. Return 0 for OFF,
@@ -370,19 +372,21 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */
i; k++) {
}
}
- assert(pCol->pDflt == 0
- || pCol->pDflt->op == TK_SPAN);
bool nullable = table_column_is_nullable(pTab, i);
+ uint32_t space_id =
+ SQLITE_PAGENO_TO_SPACEID(
+ pTab->tnum);
+ struct space *space =
+ space_cache_find(space_id);
+ char *expr_str = space->
+ def->fields[i].default_value;
sqlite3VdbeMultiLoad(v, 1, "issisi",
i, pCol->zName,
field_type_strs[
sqlite3ColumnType
(pCol)],
nullable == 0,
- pCol->
- pDflt ? pCol->
- pDflt->u.zToken
- : 0, k);
+ expr_str, k);
sqlite3VdbeAddOp2(v, OP_ResultRow, 1,
6);
}
diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
index 59662cf14..7ce5bac5d 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -1877,7 +1877,6 @@ struct Savepoint {
struct Column {
char *zName; /* Name of this column */
enum field_type type; /* Column type. */
- Expr *pDflt; /* Default value of this column */
char *zColl; /* Collating sequence. If NULL, use the default */
enum on_conflict_action notNull; /* An ON_CONFLICT_ACTION code for
* handling a NOT NULL constraint
@@ -1970,6 +1969,8 @@ 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 */
+ /** Space definition with Tarantool metadata. */
+ struct space_def *def;
};
/*
diff --git a/src/box/sql/update.c b/src/box/sql/update.c
index 83c05ab48..274305352 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,14 @@ 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));
+ if (space != NULL && space->def->fields != NULL)
+ expr = space->def->fields[i].default_value_expr;
+ sqlite3ValueFromExpr(sqlite3VdbeDb(v),
+ expr,
pCol->affinity, &pValue);
if (pValue) {
sqlite3VdbeAppendP4(v, pValue, P4_MEM);
^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH v2 1/1] Removed Expr pointer from SQL Column structure.
2018-04-18 9:47 ` Vladislav Shpilevoy
@ 2018-04-19 12:58 ` n.pettik
2018-04-19 14:07 ` Kirill Shcherbatov
0 siblings, 1 reply; 11+ messages in thread
From: n.pettik @ 2018-04-19 12:58 UTC (permalink / raw)
To: Vladislav Shpilevoy, Kirill Shcherbatov; +Cc: tarantool-patches
Hello. Several minor comments. Overall, seems OK to me.
> +/**
> + * 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.
> + * @retval not NULL on success.
> + */
> +static Table *
> +sql_table_new(Parse *pParse, char *zName)
> +{
I would use Tarantool codestyle as well for args and vars
inside function. It is not mandatory, but would look better.
> +/**
> + * 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)
> +{
The same is here: you can use Tarantool codestyle for vars/args.
Also, I strongly recommend you to add some explanation
comments, in particular concerning allocating strategy.
It doesn’t seem to be obvious (at least for independent reviewer).
> + sqlite3 *db = pParse->db;
> + struct field_def *field;
> + assert(pTable->def);
Use explicit != NULL check.
> + assert(pTable->def->exact_field_count >= (uint32_t)pTable->nCol);
> + assert(id < (uint32_t)db->aLimit[SQLITE_LIMIT_COLUMN]);
I have grepped through code, and it seems that this 'limit'
is valid only under SQLITE_MAX_COLUMN macros.
What about removing this macros (i.e. we will make this feature be always enabled),
or surrounding your code with #ifdef directives?
> +
> + if (id >= pTable->def->exact_field_count) {
> + uint32_t nCol = pTable->def->exact_field_count;
> + nCol = (nCol > 0) ? 2 * nCol : 1;
> + field = sqlite3DbRealloc(db, pTable->def->fields,
> + nCol * sizeof(pTable->def->fields[0]));
What about using simple malloc/realloc? It would allow you
to get rid of pParse argument.
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index b24d55b07..067f50a0a 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -1801,14 +1801,21 @@ xferOptimization(Parse * pParse, /* Parser context */
> }
> /* Default values for second and subsequent columns need to match. */
> if (i > 0) {
> - assert(pDestCol->pDflt == 0
> - || pDestCol->pDflt->op == TK_SPAN);
> - assert(pSrcCol->pDflt == 0
> - || pSrcCol->pDflt->op == TK_SPAN);
> - if ((pDestCol->pDflt == 0) != (pSrcCol->pDflt == 0)
> - || (pDestCol->pDflt
> - && strcmp(pDestCol->pDflt->u.zToken,
> - pSrcCol->pDflt->u.zToken) != 0)
> + uint32_t src_space_id =
> + SQLITE_PAGENO_TO_SPACEID(pSrc->tnum);
> + struct space *src_space =
> + space_cache_find(src_space_id);
space_cache_find() can return NULL pointer. I would check it on nullability somehow.
> + uint32_t dest_space_id =
> + SQLITE_PAGENO_TO_SPACEID(pDest->tnum);
> + struct space *dest_space =
> + space_cache_find(dest_space_id);
> + char *src_expr_str =
> + src_space->def->fields[i].default_value;
> + char *dest_expr_str =
> + dest_space->def->fields[i].default_value;
> + if ((dest_expr_str == NULL) != (src_expr_str == NULL)
> + || (dest_expr_str
Codestyle nitpicking: put binary operator on previous line:
… A &&
B
^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH v2 1/1] Removed Expr pointer from SQL Column structure.
2018-04-19 12:58 ` n.pettik
@ 2018-04-19 14:07 ` Kirill Shcherbatov
2018-04-19 15:05 ` n.pettik
0 siblings, 1 reply; 11+ messages in thread
From: Kirill Shcherbatov @ 2018-04-19 14:07 UTC (permalink / raw)
To: tarantool-patches; +Cc: Nikita Pettik, v.shpilevoy
Have taken into account everything except general realloc/free.
sqlite3 Delete Table is mentioned in various contexts with data allocated with SQL allocator; stdlib free in the list of various SQL free is also looks strange.
From e426855d25770f7a2e5ea20f054da971092b7c3f Mon Sep 17 00:00:00 2001
Message-Id: <e426855d25770f7a2e5ea20f054da971092b7c3f.1524146521.git.kshcherbatov@tarantool.org>
From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
Date: Thu, 19 Apr 2018 17:01:53 +0300
Subject: [PATCH] Accounted Nikita's comments
---
src/box/sql/build.c | 74 ++++++++++++++++++++++++++--------------------------
src/box/sql/insert.c | 7 ++---
2 files changed, 41 insertions(+), 40 deletions(-)
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index ab6df46..3346a5b 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -497,39 +497,39 @@ sqlite3PrimaryKeyIndex(Table * pTab)
/**
* Create and initialize a new SQL Table object.
- * @param pParse SQL Parser object.
- * @param zName Table to create name.
+ * @param parser SQL Parser object.
+ * @param name Table to create name.
* @retval NULL on memory allocation error, Parser state is
* changed.
* @retval not NULL on success.
*/
static Table *
-sql_table_new(Parse *pParse, char *zName)
+sql_table_new(Parse *parser, char *name)
{
- sqlite3 *db = pParse->db;
+ sqlite3 *db = parser->db;
- Table *pTable = sqlite3DbMallocZero(db, sizeof(Table));
+ Table *table = sqlite3DbMallocZero(db, sizeof(Table));
struct space_def *def = space_def_new(0, 0, 0, NULL, 0, NULL, 0,
&space_opts_default, NULL, 0);
- if (pTable == NULL || def == NULL) {
+ if (table == NULL || def == NULL) {
if (def != NULL)
space_def_delete(def);
- sqlite3DbFree(db, pTable);
- pParse->rc = SQLITE_NOMEM_BKPT;
- pParse->nErr++;
+ sqlite3DbFree(db, table);
+ parser->rc = SQLITE_NOMEM_BKPT;
+ parser->nErr++;
return NULL;
}
- pTable->def = def;
- pTable->zName = zName;
- pTable->iPKey = -1;
- pTable->iAutoIncPKey = -1;
- pTable->pSchema = db->pSchema;
- sqlite3HashInit(&pTable->idxHash);
- pTable->nTabRef = 1;
- pTable->nRowLogEst = 200;
+ table->def = def;
+ table->zName = name;
+ table->iPKey = -1;
+ table->iAutoIncPKey = -1;
+ table->pSchema = db->pSchema;
+ sqlite3HashInit(&table->idxHash);
+ table->nTabRef = 1;
+ table->nRowLogEst = 200;
assert(200 == sqlite3LogEst(1048576));
- return pTable;
+ return table;
}
/*
@@ -618,42 +618,42 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
/**
* Get field by id. Allocate memory if needed.
- * @param pParse SQL Parser object.
- * @param pTable SQL Table object.
+ * @param parser SQL Parser object.
+ * @param table 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)
+sql_field_retrieve(Parse *parser, Table *table, uint32_t id)
{
- sqlite3 *db = pParse->db;
+ sqlite3 *db = parser->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;
- field = sqlite3DbRealloc(db, pTable->def->fields,
- nCol * sizeof(pTable->def->fields[0]));
+ 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]));
if (field == NULL) {
- pParse->rc = SQLITE_NOMEM_BKPT;
- pParse->nErr++;
+ parser->rc = SQLITE_NOMEM_BKPT;
+ parser->nErr++;
return NULL;
}
- for (uint32_t i = nCol / 2; i < nCol; i++) {
+ for (uint32_t i = columns / 2; i < columns; i++) {
memcpy(&field[i], &field_def_default,
sizeof(struct field_def));
}
- pTable->def->fields = field;
- pTable->def->exact_field_count = nCol;
+ table->def->fields = field;
+ table->def->exact_field_count = columns;
}
- field = &pTable->def->fields[id];
+ field = &table->def->fields[id];
return field;
}
diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
index 067f50a..adc752b 100644
--- a/src/box/sql/insert.c
+++ b/src/box/sql/insert.c
@@ -1809,13 +1809,14 @@ xferOptimization(Parse * pParse, /* Parser context */
SQLITE_PAGENO_TO_SPACEID(pDest->tnum);
struct space *dest_space =
space_cache_find(dest_space_id);
+ assert(src_space != NULL && dest_space != NULL);
char *src_expr_str =
src_space->def->fields[i].default_value;
char *dest_expr_str =
dest_space->def->fields[i].default_value;
- if ((dest_expr_str == NULL) != (src_expr_str == NULL)
- || (dest_expr_str
- && strcmp(src_expr_str, dest_expr_str) != 0)
+ if ((dest_expr_str == NULL) != (src_expr_str == NULL) ||
+ (dest_expr_str &&
+ strcmp(src_expr_str, dest_expr_str) != 0)
) {
return 0; /* Default values must be the same for all columns */
}
--
2.7.4
On 19.04.2018 15:58, n.pettik wrote:
> Hello. Several minor comments. Overall, seems OK to me.
>
>> +/**
>> + * 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.
>> + * @retval not NULL on success.
>> + */
>> +static Table *
>> +sql_table_new(Parse *pParse, char *zName)
>> +{
>
> I would use Tarantool codestyle as well for args and vars
> inside function. It is not mandatory, but would look better.
>
>> +/**
>> + * 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)
>> +{
>
> The same is here: you can use Tarantool codestyle for vars/args.
> Also, I strongly recommend you to add some explanation
> comments, in particular concerning allocating strategy.
> It doesn’t seem to be obvious (at least for independent reviewer).
>
>> + sqlite3 *db = pParse->db;
>> + struct field_def *field;
>> + assert(pTable->def);
>
> Use explicit != NULL check.
>
>> + assert(pTable->def->exact_field_count >= (uint32_t)pTable->nCol);
>> + assert(id < (uint32_t)db->aLimit[SQLITE_LIMIT_COLUMN]);
>
> I have grepped through code, and it seems that this 'limit'
> is valid only under SQLITE_MAX_COLUMN macros.
> What about removing this macros (i.e. we will make this feature be always enabled),
> or surrounding your code with #ifdef directives?
>
>> +
>> + if (id >= pTable->def->exact_field_count) {
>> + uint32_t nCol = pTable->def->exact_field_count;
>> + nCol = (nCol > 0) ? 2 * nCol : 1;
>> + field = sqlite3DbRealloc(db, pTable->def->fields,
>> + nCol * sizeof(pTable->def->fields[0]));
>
> What about using simple malloc/realloc? It would allow you
> to get rid of pParse argument.
>
>> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
>> index b24d55b07..067f50a0a 100644
>> --- a/src/box/sql/insert.c
>> +++ b/src/box/sql/insert.c
>> @@ -1801,14 +1801,21 @@ xferOptimization(Parse * pParse, /* Parser context */
>> }
>> /* Default values for second and subsequent columns need to match. */
>> if (i > 0) {
>> - assert(pDestCol->pDflt == 0
>> - || pDestCol->pDflt->op == TK_SPAN);
>> - assert(pSrcCol->pDflt == 0
>> - || pSrcCol->pDflt->op == TK_SPAN);
>> - if ((pDestCol->pDflt == 0) != (pSrcCol->pDflt == 0)
>> - || (pDestCol->pDflt
>> - && strcmp(pDestCol->pDflt->u.zToken,
>> - pSrcCol->pDflt->u.zToken) != 0)
>> + uint32_t src_space_id =
>> + SQLITE_PAGENO_TO_SPACEID(pSrc->tnum);
>> + struct space *src_space =
>> + space_cache_find(src_space_id);
>
> space_cache_find() can return NULL pointer. I would check it on nullability somehow.
>
>> + uint32_t dest_space_id =
>> + SQLITE_PAGENO_TO_SPACEID(pDest->tnum);
>> + struct space *dest_space =
>> + space_cache_find(dest_space_id);
>> + char *src_expr_str =
>> + src_space->def->fields[i].default_value;
>> + char *dest_expr_str =
>> + dest_space->def->fields[i].default_value;
>> + if ((dest_expr_str == NULL) != (src_expr_str == NULL)
>> + || (dest_expr_str
>
> Codestyle nitpicking: put binary operator on previous line:
> … A &&
> B
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH v2 1/1] Removed Expr pointer from SQL Column structure.
2018-04-19 14:07 ` Kirill Shcherbatov
@ 2018-04-19 15:05 ` n.pettik
2018-04-21 8:51 ` Kirill Yukhin
0 siblings, 1 reply; 11+ messages in thread
From: n.pettik @ 2018-04-19 15:05 UTC (permalink / raw)
To: tarantool-patches; +Cc: Kirill Shcherbatov
> On 19 Apr 2018, at 17:07, Kirill Shcherbatov <kshcherbatov@tarantool.org> wrote:
>
> Have taken into account everything except general realloc/free.
Thanks. LGTM.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [tarantool-patches] Re: [PATCH v2 1/1] Removed Expr pointer from SQL Column structure.
2018-04-19 15:05 ` n.pettik
@ 2018-04-21 8:51 ` Kirill Yukhin
0 siblings, 0 replies; 11+ messages in thread
From: Kirill Yukhin @ 2018-04-21 8:51 UTC (permalink / raw)
To: tarantool-patches; +Cc: Kirill Shcherbatov
Hello,
On 19 апр 18:05, n.pettik wrote:
>
> > On 19 Apr 2018, at 17:07, Kirill Shcherbatov <kshcherbatov@tarantool.org> wrote:
> >
> > Have taken into account everything except general realloc/free.
>
> Thanks. LGTM.
I've checked the patch into 2.1 branch.
--
Regards, Kirill Yukhin
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-04-21 8:51 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-16 16:35 [tarantool-patches] [PATCH v2 1/1] Removed Expr pointer from SQL Column structure Kirill Shcherbatov
2018-04-16 19:23 ` [tarantool-patches] " Vladislav Shpilevoy
2018-04-17 11:02 ` Kirill Shcherbatov
2018-04-17 18:29 ` Vladislav Shpilevoy
2018-04-17 18:30 ` Vladislav Shpilevoy
2018-04-18 7:31 ` Kirill Shcherbatov
2018-04-18 9:47 ` Vladislav Shpilevoy
2018-04-19 12:58 ` n.pettik
2018-04-19 14:07 ` Kirill Shcherbatov
2018-04-19 15:05 ` n.pettik
2018-04-21 8:51 ` Kirill Yukhin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox