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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hello. Thanks for fixes. See below 11 comments.
> 
> /**
> - * Create and initialize a new ephemeric space_def object.
> + * Create and initialize a new ephemeral space_def object.
> 
>> 8. In the function body: "/* All allocations are on region. */".
>> Please, fix the comment.
> 
> @@ -1742,8 +1743,6 @@ int
>   sql_table_def_rebuild(struct sqlite3 *db, struct Table *pTable)
>   {
>   	assert(pTable->def->opts.temporary);
> -
> -	/* All allocations are on region. */


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.

And please, do not skip my comments (you skipped 9, 13, ...).
> 
>> 16. Out of 80 symbols.
>> 17. Missed diag_set + pParse->nErr + pParse->rc.
> @@ -667,12 +654,18 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType)
>   		return;
>   	}
>   #endif
> -	if (sql_field_retrieve(pParse, p, (uint32_t) p->def->field_count) == NULL)
> +	if (sql_field_retrieve(pParse, p,
> +			       (uint32_t) p->def->field_count) == NULL)
>   		return;
>   	struct region *region = &fiber()->gc;
>   	z = region_alloc(region, pName->n + 1);
> -	if (z == 0)
> +	if (z == NULL) {
> +		diag_set(OutOfMemory, pName->n + 1,
> +			 "region_alloc", "sqlite3AddColumn");

2. OutOfMemory takes variable name in the last argument. Not caller function name.
>> 19. Lets use diag + SQL_TARANTOOL_ERROR.
>> 20. Why do you case to char*? zStart is already const char* and strncpy takes it ok
> @@ -876,11 +869,14 @@ sqlite3AddDefaultValue(Parse * pParse, ExprSpan * pSpan)
>   			field->default_value = region_alloc(region,
>   							    default_length + 1);
>   			if (field->default_value == NULL) {
> -				pParse->rc = SQLITE_NOMEM_BKPT;
> +				diag_set(OutOfMemory, default_length + 1,
> +					 "region_alloc",
> +					 "sqlite3AddDefaultValue");

3. Same.
> 
>> 21. It is simpler and shorter to return coll_by_id() with no saving
>> into the variable.

4. Looks like you removed sql_column_collation. It would be good to see the diff
under the comment.

>> 22. On error pTable->def is not reset, so here you would delete actually
>> old pTable->def, that is on a region. Do this space_def_delete in 'else' branch.
> @@ -2142,13 +2136,16 @@ sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable)
>   			 * normally holds CHECK constraints on an ordinary table, but for
>   			 * a VIEW it holds the list of column names.
>   			 */
> -			sqlite3ColumnsFromExprList(pParse, pTable->pCheck, pTable);
>   			struct space_def *old_def = pTable->def;
> -			/* Delete it manually. */
> +			/* Manage def memory manually. */
>   			old_def->opts.temporary = true;
> -			if (sql_table_def_rebuild(db, pTable) != 0)
> +			sqlite3ColumnsFromExprList(pParse, pTable->pCheck, pTable);

5. Out of 80 symbols. Please, do self-review of a patch before sending.

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?

Below you skipped 5 comments in row((

>> +            struct space_def *old_def = pTable->def;
>> +            struct space_def *new_def = NULL;

> 23. Useless initialization - you reset this variable on the next line. 

Here I meant this:

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 33815b8a6..8c7e9d646 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -2133,8 +2133,7 @@ sqlite3ViewGetColumnNames(Parse * pParse, Table * pTable)
                         assert(pSelTab->def->opts.temporary);
  
                         struct space_def *old_def = pTable->def;
-                       struct space_def *new_def;
-                       new_def =
+                       struct space_def *new_def =
                                 sql_ephemeral_space_def_new(pParse,
                                                             old_def->name);
                         if (new_def == NULL) {

>> 29. Please, do not wrap '->'. And it is out of 80 symbols anyway.
> @@ -2997,8 +3001,8 @@ sqlite3CreateIndex(Parse * pParse,	/* All information about this parse */
>   	if (pList == 0) {
>   		Token prevCol;
>   		sqlite3TokenInit(&prevCol,
> -				 pTab->def->
> -					 fields[pTab->def->field_count - 1].name);
> +				 pTab->def->fields[
> +				 	pTab->def->field_count - 1].name);

7. Apply this:

@@ -2971,10 +2970,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);
                 pList = sqlite3ExprListAppend(pParse, 0,
                                               sqlite3ExprAlloc(db, TK_ID,
                                                                &prevCol, 0));

> 
>> 38. region_alloc can fail - set
>> sqlite3OomFault().
>   	struct region *region = &fiber()->gc;
>   	pTable->def->fields =
>   		region_alloc(region, nCol * sizeof(pTable->def->fields[0]));
> +	if (pTable->def->fields == NULL) {
> +		sqlite3OomFault(db);
> +		goto cleanup;
> +	}
>   	memset(pTable->def->fields, 0, nCol * sizeof(pTable->def->fields[0]));
>   	/* NULL nullable_action should math is_nullable flag. */
>   	for (int i = 0; i < nCol; i++)
> @@ -1899,6 +1903,7 @@ sqlite3ColumnsFromExprList(Parse * pParse,	/* Parsing context */
>   			pTable->def->fields[i].name[name_len] = '\0';
>   		}
>   	}
> +cleanup:

8. When you added cleanup label, you can reuse it for another OOM on line 1904.

9. Please, do not ignore comments.

>> -        *pnCol = 0;
>> -        return SQLITE_NOMEM_BKPT;
>> +        pTable->def->fields = NULL;
>> +        pTable->def->field_count = 0;

>39. pTable->def is on region and is not deleted together with table. You
>can skip this cleaning. 

Looks like it can be on malloc sometimes, so you should write a comment about it -
why do you need this cleaning.

>> 40. Out of 66.
>> 41. Will *be* overwritten.
>> 42. Same in the previous review - please, start a sentence from a capital
>> letter, and finish with dot.
> -			/* Will overwritten with pointer as unique identifier. */
> +			/*
> +			 * Will be overwritten with pointer as unique
> +			 * identifier.
> +			 * */

10. Still out of 66. And wrong end: '* */' must be '*/'.

>   			const char *name = "sqlite_sq_DEADBEAFDEADBEAF";
>   			pFrom->pTab = pTab =
>   				sql_ephemeral_table_new(pParse, name);
>   			if (pTab == NULL)
>   				return WRC_Abort;
> -			/* rewrite old name with correct pointer */
> +			/* Rewrite old name with correct pointer. */

11. Out of 66.