[tarantool-patches] Re: [PATCH v5 2/3] sql: remove SQL fields from Table and Column
Kirill Shcherbatov
kshcherbatov at tarantool.org
Tue May 15 18:56:03 MSK 2018
> 1. Wrong fix. As I can see sql_table_def_rebuild does not depend on allocation
> way of its arguments. So the function declaration comment must be fixed too.@@ -166,8 +166,7 @@ sql_ephemeral_space_def_new(struct Parse *parser, const char *name);
/**
* Rebuild struct def in Table with memory allocated on a single
- * malloc. Fields and strings are expected to be allocated with
- * sqlite3DbMalloc.
+ * malloc.
> 2. OutOfMemory takes variable name in the last argument. Not caller function name.
> 3. Same.
@@ -661,7 +661,7 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType)
diag_set(OutOfMemory, pName->n + 1,
- "region_alloc", "sqlite3AddColumn");
+ "region_alloc", "z");
pParse->rc = SQL_TARANTOOL_ERROR;
pParse->nErr++;
return;
@@ -871,7 +871,7 @@ sqlite3AddDefaultValue(Parse * pParse, ExprSpan * pSpan)
diag_set(OutOfMemory, default_length + 1,
"region_alloc",
- "sqlite3AddDefaultValue");
+ "field->default_value");
> 4. Looks like you removed sql_column_collation. It would be good to see the diff
> under the comment.
@@ -1053,45 +1030,13 @@ sqlite3AddCollateType(Parse * pParse, Token * pToken)
- if (pIdx->aiColumn[0] == i)
- pIdx->coll_array[0] = sql_column_collation(p, i);
+ if (pIdx->aiColumn[0] == i) {
+ pIdx->coll_array[0] =
+ coll_by_id(p->def->fields[i].coll_id);
+ }
}
@@ -3066,7 +3074,7 @@ sqlite3CreateIndex(Parse * pParse, /* All information about this parse */
- coll = sql_column_collation(pTab, j);
+ coll = coll_by_id(pTab->def->fields[j].coll_id);
@@ -179,13 +182,14 @@ sql_expr_coll(Parse *parse, Expr *p, bool *is_found)
- coll = sql_column_collation(p->pTab, j);
+ coll = coll_by_id(
+ p->space_def->fields[j].coll_id);
@@ -298,14 +299,14 @@ sqlite3FkLocateIndex(Parse * pParse, /* Parse context to store any error in */
- def_coll = sql_column_collation(pParent,
- iCol);
+ def_coll = coll_by_id(
+ pParent->def->fields[iCol].coll_id);
@@ -298,14 +299,14 @@ sqlite3FkLocateIndex(Parse * pParse, /* Parse context to store any error in */
- def_coll = sql_column_collation(pParent,
- iCol);
+ def_coll = coll_by_id(
+ pParent->def->fields[iCol].coll_id);
@@ -1784,26 +1795,29 @@ xferOptimization(Parse * pParse, /* Parser context */
- if (sql_column_collation(pDest, i) !=
- sql_column_collation(pSrc, i)) {
+ if (coll_by_id(pDest->def->fields[i].coll_id) !=
+ coll_by_id(pSrc->def->fields[i].coll_id))
@@ -3529,8 +3499,6 @@ void sqlite3AddCollateType(Parse *, Token *);
-struct coll *
-sql_column_collation(Table *, uint32_t);
> 5. Out of 80 symbols. Please, do self-review of a patch before sending.
@@ -2110,7 +2110,8 @@ sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable)
- sqlite3ColumnsFromExprList(pParse, pTable->pCheck, pTable);
+ sqlite3ColumnsFromExprList(pParse, pTable->pCheck,
+ pTable);
> 6. I looked at sqlite3ColumnsFromExprList, and looks like it does not use
> opts.temporary flag. So you can remove this opts.temporary flag manipulation, it is not?
@@ -1742,7 +1742,6 @@ sql_ephemeral_table_new(Parse *parser, const char *name)
int
sql_table_def_rebuild(struct sqlite3 *db, struct Table *pTable)
{
- assert(pTable->def->opts.temporary);
struct space_def *old_def = pTable->def;
@@ -2108,12 +2108,9 @@ sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable)
* a VIEW it holds the list of column names.
*/
struct space_def *old_def = pTable->def;
- /* Manage def memory manually. */
- old_def->opts.temporary = true;
sqlite3ColumnsFromExprList(pParse, pTable->pCheck,
pTable);
if (sql_table_def_rebuild(db, pTable) != 0) {
- old_def->opts.temporary = false;
>
> Here I meant this:
@@ -2131,8 +2131,7 @@ sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable)
- struct space_def *new_def;
- new_def =
+ struct space_def *new_def =
sql_ephemeral_space_def_new(pParse,
> 7. Apply this:
@@ -2969,10 +2968,9 @@ sqlite3CreateIndex(Parse * pParse, /* All information about this parse */
*/
if (pList == 0) {
Token prevCol;
- sqlite3TokenInit(&prevCol,
- pTab->def->fields[
- pTab->def->field_count - 1].
- name);
+ struct field_def *field =
+ &pTab->def->fields[pTab->def->field_count - 1];
+ sqlite3TokenInit(&prevCol, field->name);
> 8. When you added cleanup label, you can reuse it for another OOM on line 1904.
@@ -1842,7 +1842,7 @@ sqlite3ColumnsFromExprList(Parse * pParse, /* Parsing context */
- for (i = 0, pCol = aCol; i < nCol && !db->mallocFailed; i++, pCol++) {
+ for (i = 0, pCol = aCol; i < nCol; i++, pCol++) {
@@ -1902,6 +1902,7 @@ sqlite3ColumnsFromExprList(Parse * pParse, /* Parsing context */
if (pTable->def->fields[i].name == NULL) {
sqlite3OomFault(db);
+ goto cleanup;
} else {
> 9. Please, do not ignore comments.
> Looks like it can be on malloc sometimes, so you should write a comment about it -
> why do you need this cleaning.
@@ -1912,6 +1912,10 @@ cleanup:
sqlite3HashClear(&ht);
int rc = db->mallocFailed ? SQLITE_NOMEM_BKPT : SQLITE_OK;
if (rc != SQLITE_OK) {
+ /*
+ * pTable->def could be not temporal in
+ * sqlite3ViewGetColumnNames so we need clean-up.
+ */
> 10. Still out of 66. And wrong end: '* */' must be '*/'.@@ -4714,9 +4718,9 @@ selectExpander(Walker * pWalker, Select * p)
- * Will be overwritten with pointer as unique
- * identifier.
- * */
+ * Will be overwritten with pointer as
+ * unique identifier.
+ */
> 11. Out of 66.
@@ -4729,15 +4732,17 @@ selectExpander(Walker * pWalker, Select * p)
- /* Rewrite old name with correct pointer. */
+ /*
+ * Rewrite old name with correct pointer.
+ */
More information about the Tarantool-patches
mailing list