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

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

> 1. Lets remove 'to': nullable_action_is_nullable. It is too long too, but I
> can not think up anotherstatic inline bool
-nullable_action_to_is_nullable(enum on_conflict_action nullable_action)
+nullable_action_is_nullable(enum on_conflict_action nullable_action)

> 4. Use space_def_sizeof for this please.
diff --git a/src/box/space_def.c b/src/box/space_def.c
index 77c0e02..1fa3345 100644
--- a/src/box/space_def.c
+++ b/src/box/space_def.c
@@ -47,20 +47,7 @@ const struct opt_def space_opts_reg[] = {
 	OPT_END,
 };
 
-/**
- * Size of the space_def.
- * @param name_len Length of the space name.
- * @param fields Fields array of space format.
- * @param field_count Space field count.
- * @param[out] names_offset Offset from the beginning of a def to
- *             a field names memory.
- * @param[out] fields_offset Offset from the beginning of a def to
- *             a fields array.
- * @param[out] def_expr_offset Offset from the beginning of a def
- *             to a def_value_expr array.
- * @retval Size in bytes.
- */
-static inline size_t
+size_t
 space_def_sizeof(uint32_t name_len, const struct field_def *fields,
 		 uint32_t field_count, uint32_t *names_offset,
 		 uint32_t *fields_offset, uint32_t *def_expr_offset)

diff --git a/src/box/space_def.h b/src/box/space_def.h
index 4add1e7..52447b6 100644
--- a/src/box/space_def.h
+++ b/src/box/space_def.h
@@ -157,6 +157,24 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count,
 	      const struct space_opts *opts, const struct field_def *fields,
 	      uint32_t field_count);
 
+/**
+ * Size of the space_def.
+ * @param name_len Length of the space name.
+ * @param fields Fields array of space format.
+ * @param field_count Space field count.
+ * @param[out] names_offset Offset from the beginning of a def to
+ *             a field names memory.
+ * @param[out] fields_offset Offset from the beginning of a def to
+ *             a fields array.
+ * @param[out] def_expr_offset Offset from the beginning of a def
+ *             to a def_value_expr array.
+ * @retval Size in bytes.
+ */
+size_t
+space_def_sizeof(uint32_t name_len, const struct field_def *fields,
+		 uint32_t field_count, uint32_t *names_offset,
+		 uint32_t *fields_offset, uint32_t *def_expr_offset);
+

diff --git a/src/box/sql.c b/src/box/sql.c
index 7d48cdc..d8a3ef3 100644

@@ -1704,7 +1704,8 @@ sql_ephemeral_space_def_new(Parse *parser, const char *name)
 	struct space_def *def = NULL;
 	struct region *region = &fiber()->gc;
 	size_t name_len = name != NULL ? strlen(name) : 0;
-	size_t size = sizeof(struct space_def) + name_len + 1;
+	uint32_t dummy;
+	size_t size = space_def_sizeof(name_len, NULL, 0, &dummy, &dummy, &dummy);
 	def = (struct space_def *)region_alloc(region, size);
 	if (def == NULL) {
 		diag_set(OutOfMemory, sizeof(struct tuple_dictionary),

> 5. If you return on def == NULL, then either move all the other code
> under 'else' body, or remove the 'else'.

@@ -1712,9 +1713,9 @@ sql_ephemeral_space_def_new(Parse *parser, const char *name)
 		parser->rc = SQL_TARANTOOL_ERROR;
 		parser->nErr++;
 		return NULL;
-	} else {
-		memset(def, 0, size);
 	}
+
+	memset(def, 0, size);

> 6. Same as in the previous review - ephemeric -> ephemeral.
+++ b/src/box/sql.h
@@ -145,7 +145,7 @@ void
 sql_expr_free(struct sqlite3 *db, struct Expr *expr, bool extern_alloc);
 
 /**
- * Create and initialize a new ephemeric SQL Table object.
+ * Create and initialize a new ephemeral SQL Table object.

> 7. Same.
@@ -155,7 +155,7 @@ struct Table *
sql_ephemeral_table_new(struct Parse *parser, const char *name);

/**
- * 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. */
> 10. pDflt == NULL.
+++ b/src/box/sql/alter.c
@@ -200,11 +200,12 @@ sqlite3AlterFinishAddColumn(Parse * pParse, Token * pColDef)
 		return;
 	}
 	assert(pNew->def->fields[pNew->def->field_count - 1].is_nullable ==
-	       nullable_action_to_is_nullable(
-		pNew->def->fields[pNew->def->field_count - 1].nullable_action));
+		       nullable_action_is_nullable(
+			       pNew->def->fields[pNew->def->field_count -
+						 1].nullable_action));
 
-	if (pNew->def->fields[pNew->def->field_count - 1].nullable_action
-	    && !pDflt) {
+	if (!pNew->def->fields[pNew->def->field_count - 1].is_nullable
+	    && pDflt == NULL) {

> 11. Forgot to set sqlite3OomFault(db). I know, that this code is
> unreachable, but in the future it will be not.
@@ -296,6 +297,7 @@ sqlite3AlterBeginAddColumn(Parse * pParse, SrcList * pSrc)
 	pNew->def = space_def_dup(pTab->def);
 	if (pNew->def == NULL) {
 		sqlite3DbFree(db, pNew);
+		sqlite3OomFault(db);

> 12. Nice. Now this function consists of a single DbFree - lets just
> inline it and remove sqlite3DeleteColumnNames.
@@ -381,7 +370,7 @@ deleteTable(sqlite3 * db, Table * pTable)
 	/* Delete the Table structure itself.
 	 */
 	sqlite3HashClear(&pTable->idxHash);
-	sqlite3DeleteColumnNames(db, pTable);
+	sqlite3DbFree(db, pTable->aCol);

@@ -2212,10 +2213,9 @@ sqliteViewResetAll(sqlite3 * db)
 		Table *pTab = sqliteHashData(i);
 		assert(pTab->def->opts.is_view == (pTab->pSelect != NULL));
 		if (pTab->def->opts.is_view) {
-			sqlite3DeleteColumnNames(db, pTab);
+			sqlite3DbFree(db, pTab->aCol);

+++ b/src/box/sql/sqliteInt.h
@@ -3503,7 +3503,6 @@ int sqlite3InitCallback(void *, int, char **, char **);
 void sqlite3CommitInternalChanges();
-void sqlite3DeleteColumnNames(sqlite3 *, Table *);

+++ b/src/box/sql/build.c
@@ -285,17 +285,6 @@ sqlite3CommitInternalChanges()
 }
 
 /*
- * Delete memory allocated for the column names of a table or view (the
- * Table.aCol[] array).
- */
-void
-sqlite3DeleteColumnNames(sqlite3 * db, Table * pTable)
-{
-	assert(pTable != 0);
-	sqlite3DbFree(db, pTable->aCol);
-}


> 14. Out of 80 symbols.
> 15. Why you can not do one memcpy(field, table->def->fields,
> 				sizeof(*field) * table->def->exact_field_count); ?@@ -616,17 +605,15 @@ sql_field_retrieve(Parse *parser, Table *table, uint32_t id)
 		field = region_alloc(region, columns_new *
 				     sizeof(table->def->fields[0]));
 		if (field == NULL) {
-			diag_set(OutOfMemory, columns_new * sizeof(table->def->fields[0]),
+			diag_set(OutOfMemory, columns_new *
+				sizeof(table->def->fields[0]),
 				"region_alloc", "sql_field_retrieve");
 			parser->rc = SQL_TARANTOOL_ERROR;
 			parser->nErr++;
 			return NULL;
 		}
-
-		for (uint32_t i = 0; i < table->def->exact_field_count; i++) {
-			memcpy(&field[i], &table->def->fields[i],
-			       sizeof(struct field_def));
-		}
+		memcpy(field, table->def->fields,
+		       sizeof(*field) * table->def->exact_field_count);


> 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");
+		pParse->rc = SQL_TARANTOOL_ERROR;
+		pParse->nErr++;
 		return;
+	}

> 18. Now you have nullable_action_to_is_nullable.
@@ -745,7 +738,7 @@ sqlite3AddNotNull(Parse * pParse, int onError)
 		return;
 	p->def->fields[p->def->field_count - 1].nullable_action = (u8)onError;
 	p->def->fields[p->def->field_count - 1].is_nullable =
-		(onError == ON_CONFLICT_ACTION_NONE);
+		nullable_action_is_nullable(onError);

> 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");
+				pParse->rc = SQL_TARANTOOL_ERROR;
 				pParse->nErr++;
 				return;
 			}
-			strncpy(field->default_value, (char *)pSpan->zStart,
+			strncpy(field->default_value, pSpan->zStart,
 				default_length);

> 21. It is simpler and shorter to return coll_by_id() with no saving
> into the variable.
> 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);
+			if (sql_table_def_rebuild(db, pTable) != 0) {
+				old_def->opts.temporary = false;
 				nErr++;
-			space_def_delete(old_def);
+			} else {
+				space_def_delete(old_def);
+			}


> 28. It is not guaranteed. You must check for def == NULL and set
> sqlite3OomFault(db). And do not reset pTab def, if you can not create a new one.
@@ -2223,8 +2223,12 @@ sqliteViewResetAll(sqlite3 * db)
 						  strlen(old_def->engine_name),
 						  &old_def->opts,
 						  NULL, 0);
-			assert(pTab->def);
-			space_def_delete(old_def);
+			if (pTab->def == NULL) {
+				sqlite3OomFault(db);
+				pTab->def = old_def;
+			} else {
+				space_def_delete(old_def);
+			}

> 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);

> 30. strdup can fail. See the sqlite3HashInsert what to do in such a case.
+++ b/src/box/sql/hash.c
@@ -293,7 +293,8 @@ sqlite3HashInsert(Hash * pH, const char *pKey, void *data)
 			removeElementGivenHash(pH, elem, h);
 		} else {
 			elem->data = data;
-			elem->pKey = strdup(pKey);
+			assert(elem->pKey != NULL);
+			assert(strcmp(elem->pKey, pKey) == 0);
 		}
 		return old_data;
 	}
@@ -303,6 +304,10 @@ sqlite3HashInsert(Hash * pH, const char *pKey, void *data)
 	if (new_elem == 0)
 		return data;
 	new_elem->pKey = strdup(pKey);
+	if (new_elem->pKey == NULL) {
+		sqlite3_free(new_elem);
+		return data;
+	}
 	new_elem->data = data;
 	pH->count++;


>> +void
>> +sql_parser_free(Parse *parser)
>> +{
>> +	if (parser == NULL)
>> +		
> 34. It is never == NULL as I can see.
 void
-sql_parser_free(Parse *parser)
+sql_parser_destroy(Parse *parser)
 {
-	if (parser == NULL)
-		return;
+	assert(parser != NULL);

> 36. existent -> existing.
+++ b/src/box/sql/select.c
@@ -1825,12 +1825,16 @@ sqlite3ColumnsFromExprList(Parse * pParse,	/* Parsing context */
-	 * names for existent table.
+	 * names for existing table.

> 37. Lets better check def->opts.is_temporary. field == NULL means nothing.
sqlite3ViewGetColumnNames calls sqlite3ColumnsFromExprList with non-temporal table in 
pSel = sqlite3SelectDup(db, pTable->pSelect, 0);
	if (pSel) {
branch.
It is ok, as we manually manage memory in this context.

> 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:
 	sqlite3HashClear(&ht);
 	int rc = db->mallocFailed ? SQLITE_NOMEM_BKPT : SQLITE_OK;


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

> 43. Out convention is 'create/destroy'. Please, respect it.
--- a/src/box/sql/tokenize.c
+++ b/src/box/sql/tokenize.c
@@ -671,6 +671,6 @@ sql_expr_compile(sqlite3 *db, const char *expr, struct Expr **result)
 		return -1;
 	}
 	*result = parser.parsed_expr;
-	sql_parser_free(&parser);
+	sql_parser_destroy(&parser);

+++ b/src/box/sql/trigger.c
@@ -935,7 +935,7 @@ codeRowTrigger(Parse * pParse,	/* Current parse context */
 
 	assert(!pSubParse->pZombieTab);
 	assert(!pSubParse->pTriggerPrg && !pSubParse->nMaxArg);
-	sql_parser_free(pSubParse);
+	sql_parser_destroy(pSubParse);

+++ b/src/box/sql/sqliteInt.h
@@ -4160,6 +4159,6 @@ sql_parser_create(struct Parse *parser)
  * @param parser object to release.
  */
 void
-sql_parser_free(struct Parse *parser);
+sql_parser_destroy(struct Parse *parser);