[tarantool-patches] Re: [PATCH v2 1/1] Removed Expr pointer from SQL Column structure.

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

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