[tarantool-patches] Re: [PATCH v5 2/3] sql: remove SQL fields from Table and Column

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

> 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.
+                        */