[tarantool-patches] Re: [PATCH v3 4/4] sql: Region-based allocations.

[Vladislav Shpilevoy]

Hello. Thanks for contributing! See below 16 comments.

On 25/04/2018 19:52, Kirill Shcherbatov wrote:
> Function sql_field_retrieve could leak names
> in error cases. Let allocate all memory with
> region allocator.
> 
> Needed for #3272.

1. It is not only 3272. It is a part of the separate region
allocations issue. I do not remember the number, please, find and
write here 'Part of #nnnn#' on new line.

> diff --git a/src/box/sql.c b/src/box/sql.c
> index 6d4255e..2893d70 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -1685,22 +1685,43 @@ space_column_default_expr(uint32_t space_id, uint32_t fieldno)
>   	return space->def->fields[fieldno].default_value_expr;
>   }
>   
> +struct space_def *
> +sql_ephemeral_space_def_new(Parse *parser)
> +{
> +	struct space_def *def = NULL;
> +	struct region *region = &fiber()->gc;
> +	size_t size = sizeof(struct space_def) + 1;
> +	def = (struct space_def *)region_alloc(region, size);
> +	if (def != NULL) {
> +		memset(def, 0, size);
> +		def->dict =
> +			(struct tuple_dictionary *)
> +				region_alloc(region,
> +					     sizeof(struct tuple_dictionary));

2. Why do you need a dictionary? It must not be used in SQL anywhere. And
please, do not mess checks on allocations error - try to check it right after
allocation. It is hard to review code, where after alloc its result is checked
multiple times on ==/!= NULL. If you have multiple allocations, and after fail of
each one you must destroy some results of the previous ones, or set some error
codes, you can use this way:

	if (alloc1_failed)
		goto error1;
	...
	if (alloc2_failed)
		goto error2;
	...
error3:
	do_clean3...
error2:
	do_clean2...
error1:
	do_clean1...
	return -1;

See the good example in vy_lsm_new() function.

> +	}
> +	if (def == NULL || def->dict == NULL) {
> +		parser->rc = SQLITE_NOMEM_BKPT;
> +		parser->nErr++;
> +		return NULL;
> +	}
> +	def->dict->refs = 1;
> +	def->opts.temporary = true;
> +	return def;
> +}
> +
>   Table *
>   sql_ephemeral_table_new(Parse *parser)
>   {
>   	sqlite3 *db = parser->db;
>   	struct space_def *def = NULL;
>   	Table *table = sqlite3DbMallocZero(db, sizeof(Table));
> -	if (table != NULL) {
> -		def = space_def_new(0, 0, 0, NULL, 0, NULL, 0,
> -				    &space_opts_default, NULL, 0);
> -	}
> +	if (table != NULL)
> +		def = sql_ephemeral_space_def_new(parser);
>   	if (def == NULL) {
>   		sqlite3DbFree(db, table);
> -		parser->rc = SQLITE_NOMEM_BKPT;
> -		parser->nErr++;

3. Why did you remove it? Region allocation error is the same, as
malloc allocation error. See details in 6.

>   		return NULL;
>   	}
> +

4. Garbage diff.
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 584e6b1..4db4356 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -386,7 +386,9 @@ deleteTable(sqlite3 * db, Table * pTable)
>   	sqlite3DbFree(db, pTable->zColAff);
>   	sqlite3SelectDelete(db, pTable->pSelect);
>   	sqlite3ExprListDelete(db, pTable->pCheck);
> -	if (pTable->def != NULL)
> +	/* Do not delete pTable->def allocated not on region. */
> +	assert(pTable->def != NULL);
> +	if (pTable->def->opts.temporary == false)

5. Boolean variables checking on true/false can be omitted.

> @@ -603,29 +606,33 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
>   static struct field_def *
>   sql_field_retrieve(Parse *parser, Table *table, uint32_t id)
>   {
> -	sqlite3 *db = parser->db;
>   	struct field_def *field;
>   	assert(table->def != NULL);
>   	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]));
> +		uint32_t columns_new = table->def->exact_field_count;
> +		columns_new = (columns_new > 0) ? 2 * columns_new : 1;
> +		struct region *region = &fiber()->gc;
> +		field = region_alloc(region,
> +				     columns_new * sizeof(table->def->fields[0]))

6. On region error please do diag_set error. Region error is Tarantool error, not
sqlite. And on such error you must return not SQLITE_NOMEM_BKPT, but
SQL_TARANTOOL_ERROR. It leads to returning to a user an error, that is set in diag.

>   		if (field == NULL) {
>   			parser->rc = SQLITE_NOMEM_BKPT;
>   			parser->nErr++;
>   			return NULL;
>   		}
>   
> -		for (uint32_t i = columns / 2; i < columns; i++) {
> +		for (uint32_t i = 0; i < table->def->exact_field_count; i++) {
> +			memcpy(&field[i], &table->def->fields[i],
> +			       sizeof(struct field_def));
> +		}

7. It would be good to note here, that field names are not on the same
region allocation, and it is ok to memcpy pointers on them.

> +		for (uint32_t i = columns_new / 2; i < columns_new; i++) {
>   			memcpy(&field[i], &field_def_default,
>   			       sizeof(struct field_def));
>   		}
>   
>   		table->def->fields = field;
> -		table->def->exact_field_count = columns;
> +		table->def->exact_field_count = columns_new;

8. This diff, and other columns->columns_new diff are garbage. Why did you
rename it? The previous name was ok.
> @@ -677,10 +685,8 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType)
>   		aNew =
>   		    sqlite3DbRealloc(db, p->aCol,
>   				     (p->def->field_count + 8) * sizeof(p->aCol[0]));
> -		if (aNew == 0) {
> -			sqlite3DbFree(db, z);
> +		if (aNew == 0)
>   			return;

9. Please, use == NULL for pointers. I see, that it is left from the original
code, but if you modify it, then use Tarantool code style in the new code please.
> diff --git a/src/box/sql/prepare.c b/src/box/sql/prepare.c
> index 2ab8751..7dc14f6 100644
> --- a/src/box/sql/prepare.c
> +++ b/src/box/sql/prepare.c
> @@ -211,17 +211,19 @@ sqlite3InitDatabase(sqlite3 * db)
>   void
>   sqlite3ParserReset(Parse * pParse)
>   {
> -	if (pParse) {
> -		sqlite3 *db = pParse->db;
> -		sqlite3DbFree(db, pParse->aLabel);
> -		sqlite3ExprListDelete(db, pParse->pConstExpr);
> -		if (db) {
> -			assert(db->lookaside.bDisable >=
> -			       pParse->disableLookaside);
> -			db->lookaside.bDisable -= pParse->disableLookaside;
> -		}
> -		pParse->disableLookaside = 0;
> +	if (pParse == NULL)
> +		return;
> +	sqlite3 *db = pParse->db;
> +	sqlite3DbFree(db, pParse->aLabel);
> +	sqlite3ExprListDelete(db, pParse->pConstExpr);
> +	if (db) {

10. Same as 9.
> diff --git a/src/box/sql/select.c b/src/box/sql/select.c
> index 9eeff8e..03bfcf9 100644
> --- a/src/box/sql/select.c
> +++ b/src/box/sql/select.c
> @@ -1822,9 +1822,20 @@ sqlite3ColumnsFromExprList(Parse * pParse,	/* Parsing context */
>   		aCol = 0;
>   	}
>   	assert(nCol == (i16) nCol);
> +
> +	struct region *region = &fiber()->gc;
>   	assert(pTable->def->fields == NULL);
> +	if (pTable->def->opts.temporary == false) {

11. At first, same as 5. At second, why? I can not see this in the original code.
> @@ -1877,9 +1888,16 @@ sqlite3ColumnsFromExprList(Parse * pParse,	/* Parsing context */
>   			if (cnt > 3)
>   				sqlite3_randomness(sizeof(cnt), &cnt);
>   		}
> -		pTable->def->fields[i].name = zName;
> -		if (zName && sqlite3HashInsert(&ht, zName, pCol) == pCol) {
> +		uint32_t zNameLen = (uint32_t)strlen(zName);

12. For new code please use Tarantool code style - no Camel case.
zNameLen -> name_len.

> +		if (zName && sqlite3HashInsert(&ht, zName, pCol) == pCol)

13. Same as 9.
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 94d5c80..520b74c 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -2933,6 +2933,8 @@ struct Parse {
>   	u8 eTriggerOp;		/* TK_UPDATE, TK_INSERT or TK_DELETE */
>   	u8 eOrconf;		/* Default ON CONFLICT policy for trigger steps */
>   	u8 disableTriggers;	/* True to disable triggers */
> +	/** Region size at the Parser launch */

14. Please, put a dot at the end of the comment.
> diff --git a/src/box/sql/tokenize.c b/src/box/sql/tokenize.c
> index c77aa9b..9055bd0 100644
> --- a/src/box/sql/tokenize.c
> +++ b/src/box/sql/tokenize.c
> @@ -664,6 +664,9 @@ sql_expr_compile(sqlite3 *db, const char *expr, struct Expr **result)
>   	memset(&parser, 0, sizeof(parser));
>   	parser.db = db;
>   	parser.parse_only = true;
> +	struct region *region = &fiber()->gc;
> +	parser.region_initial_size = region_used(region);

15. It is a second place, where a parser is created. How about creating a function
sql_parser_create(struct Parser *parser), that will do this?

16. I do not see, that you removed def rebuilding from sqlite3ColumnsFromExprList -
it is not needed anymore.