[tarantool-patches] Re: [PATCH v2 1/1] sql: remove zName and nColumn from SQL

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

Hello. Thanks for contributing! See below my 14 comments.

On 19/04/2018 15:47, Kirill Shcherbatov wrote:
> 1. Removed zName from SQL Column.
> 2. Removed zColumns from SQL Table.
> 3. Refactored Parser to use def_expression directly.
> 4. Introduced sql_table_def_rebuild intended for collect
> fragmented with sql_field_retrieve space_def into memory
> located in one allocation.
> 
> Needed for #3272.
> ---
>   src/box/space_def.c     |  29 ++++----
>   src/box/sql.c           |  39 +++++++---
>   src/box/sql.h           |  22 ++++++
>   src/box/sql/alter.c     |  31 +++++---
>   src/box/sql/analyze.c   |   5 +-
>   src/box/sql/build.c     | 194 +++++++++++++++++++++++-------------------------
>   src/box/sql/delete.c    |   6 +-
>   src/box/sql/expr.c      |  11 +--
>   src/box/sql/fkey.c      |  20 ++---
>   src/box/sql/insert.c    |  53 +++++++------
>   src/box/sql/pragma.c    |  24 +++---
>   src/box/sql/resolve.c   |  16 ++--
>   src/box/sql/select.c    |  96 +++++++++++++-----------
>   src/box/sql/sqliteInt.h |  15 +++-
>   src/box/sql/update.c    |  29 ++++----
>   src/box/sql/where.c     |   6 +-
>   src/box/sql/wherecode.c |   2 +-
>   src/box/sql/whereexpr.c |   4 +-
>   18 files changed, 341 insertions(+), 261 deletions(-)
> 
> diff --git a/src/box/space_def.c b/src/box/space_def.c
> index 22bd3ca..77c0e02 100644
> --- a/src/box/space_def.c
> +++ b/src/box/space_def.c
>   
> @@ -116,12 +117,13 @@ space_def_dup(const struct space_def *src)
>   			if (src->fields[i].default_value != NULL) {
>   				ret->fields[i].default_value = strs_pos;
>   				strs_pos += strlen(strs_pos) + 1;
> -
> -				struct Expr *e =
> -					src->fields[i].default_value_expr;
> -				assert(e != NULL);
> +			}
> +			struct Expr *e =
> +				src->fields[i].default_value_expr;
> +			if (e != NULL) {
>   				char *expr_pos_old = expr_pos;
> -				e = sql_expr_dup(sql_get(), e, 0, &expr_pos);
> +				e = sql_expr_dup(sql_get(), e, 0,
> +						 &expr_pos);

1. Unnecessary diff.

>   				assert(e != NULL);
>   				/* Note: due to SQL legacy
>   				 * duplicactor pointer is not
> @@ -201,12 +203,13 @@ space_def_new(uint32_t id, uint32_t uid, uint32_t exact_field_count,
>   				       fields[i].default_value, len);
>   				def->fields[i].default_value[len] = 0;
>   				strs_pos += len + 1;
> -
> -				struct Expr *e =
> -					fields[i].default_value_expr;
> -				assert(e != NULL);
> +			}
> +			struct Expr *e =
> +				fields[i].default_value_expr;
> +			if (e != NULL) {
>   				char *expr_pos_old = expr_pos;
> -				e = sql_expr_dup(sql_get(), e, 0, &expr_pos);
> +				e = sql_expr_dup(sql_get(), e, 0,
> +						 &expr_pos);

2. Same as 1.

> diff --git a/src/box/sql.c b/src/box/sql.c
> index 6bc82c2..e3ae82c 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -1712,3 +1710,26 @@ space_column_default_expr(uint32_t space_id, uint32_t fieldno)
>   
>   	return space->def->fields[fieldno].default_value_expr;
>   }
> +
> +int
> +sql_table_def_rebuild(struct sqlite3 *db, struct Table *pTable)
> +{
> +	struct space_def *old_def = pTable->def;
> +	struct space_def *new_def = NULL;
> +	new_def = space_def_new(old_def->id, old_def->uid,
> +				old_def->field_count, old_def->name,
> +				strlen(old_def->name), old_def->engine_name,
> +				strlen(old_def->engine_name), &old_def->opts,
> +				old_def->fields, old_def->field_count);
> +	if (new_def == NULL) {
> +		sqlite3OomFault(db);
> +		return 1;

3. Lets return -1, it is a common practice. And as I can understand, here
old_def is fragmented still, so it will not be fully deleted by
space_def_delete later in deleteTable. Fix this, please. As far as I can
remember, I proposed to do space_def rebuild on each sql_field_retrieve until
we start to use region in parser.


> diff --git a/src/box/sql.h b/src/box/sql.h
> index db92d80..2a9b59f 100644
> --- a/src/box/sql.h
> +++ b/src/box/sql.h
> @@ -65,6 +65,7 @@ sql_get();
>   struct Expr;
>   struct Parse;
>   struct Select;
> +struct Table;
>   
>   /**
>    * Perform parsing of provided expression. This is done by
> @@ -143,6 +144,27 @@ sql_expr_dup(struct sqlite3 *db, struct Expr *p, int flags, char **buffer);
>   void
>   sql_expr_free(struct sqlite3 *db, struct Expr *expr, bool extern_alloc);
>   
> +/**
> + * Create and initialize a new ephemeric SQL Table object.

4. 'Ephemeral', not 'ephemeric' - they mean completely different things.

> + * @param pParse SQL Parser object.
> + * @param zName Table to create name.
> + * @retval NULL on memory allocation error, Parser state changed.
> + * @retval not NULL on success.
> + */
> +struct Table *
> +sql_ephemeric_table_new(struct Parse *pParse);

5. Lets use Tarantool code style for parameters.

> +
> +/**
> + * Rebuilds struct def in Table with memory allocated on region.

6. Function comment must not describe it as an object. It must describe
what the function does. So not 'rebuilds', but 'rebuild'.

And this function does not use region. What do you mean?

> + * Fields and strings are expected to be allocated with sqlite3DbMalloc.
> + * @param db The database connection.
> + * @param pTable The Table with fragmented memory in def to rebuild..
> + * @retval 1 on memory allocation error
> + * @retval 0 on success
> + */
> +int
> +sql_table_def_rebuild(struct sqlite3 *db, struct Table *pTable);
> +
>   #if defined(__cplusplus)
>   } /* extern "C" { */
>   #endif
> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
> index cf8be5d..47ee5e8 100644
> --- a/src/box/sql/alter.c
> +++ b/src/box/sql/alter.c
> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
> index 665bfbc..350bc8c 100644
> --- a/src/box/sql/analyze.c
> +++ b/src/box/sql/analyze.c
> @@ -1028,9 +1028,10 @@ analyzeOneTable(Parse * pParse,	/* Parser context */
>   		regKeyStat = sqlite3GetTempRange(pParse, nPkColumn);
>   		for (j = 0; j < nPkColumn; j++) {
>   			k = pPk->aiColumn[j];
> -			assert(k >= 0 && k < pTab->nCol);
> +			assert(k >= 0 && k < (int)pTab->def->field_count);
>   			sqlite3VdbeAddOp3(v, OP_Column, iIdxCur, k, regKeyStat + j);
> -			VdbeComment((v, "%s", pTab->aCol[pPk->aiColumn[j]].zName));
> +			VdbeComment((v, "%s",
> +				pTab->def->fields[pPk->aiColumn[j]].name));

7. Please, align the new line.
> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index ab6df46..4ac421c 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -297,8 +297,7 @@ sqlite3DeleteColumnNames(sqlite3 * db, Table * pTable)
>   	Column *pCol;
>   	assert(pTable != 0);
>   	if ((pCol = pTable->aCol) != 0) {
> -		for (i = 0; i < pTable->nCol; i++, pCol++) {
> -			sqlite3DbFree(db, pCol->zName);
> +		for (i = 0; i < (int)pTable->def->field_count; i++, pCol++) {
>   			sqlite3DbFree(db, pCol->zColl);

8. Please, rebase on latest 2.0 - this place is slightly changed already. And do
not put 'for' body into {}, when it consists of a single line.

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

9. Looks like you force pushed your patch over my review fixes.
Here 'is' is needed.
> @@ -624,13 +627,12 @@ sqlite3StartTable(Parse *pParse, Token *pName, int noErr)
>    * @retval not NULL on success.
>    * @retval NULL on out of memory.
>    */
> -static struct field_def *
> +struct field_def *

10. Same - you removed my fixes. It is used in build.c only and can be static.

>   sql_field_retrieve(Parse *pParse, Table *pTable, uint32_t id)
>   {
>   	sqlite3 *db = pParse->db;
>   	struct field_def *field;
> -	assert(pTable->def);
> -	assert(pTable->def->exact_field_count >= (uint32_t)pTable->nCol);
> +	assert(pTable->def != NULL);
>   	assert(id < (uint32_t)db->aLimit[SQLITE_LIMIT_COLUMN]);
>   
>   	if (id >= pTable->def->exact_field_count) {
> @@ -638,11 +640,8 @@ sql_field_retrieve(Parse *pParse, Table *pTable, uint32_t id)
>   		nCol = (nCol > 0) ? 2 * nCol : 1;
>   		field = sqlite3DbRealloc(db, pTable->def->fields,
>   					 nCol * sizeof(pTable->def->fields[0]));
> -		if (field == NULL) {
> -			pParse->rc = SQLITE_NOMEM_BKPT;
> -			pParse->nErr++;
> -			return NULL;
> -		}
> +		if (field == NULL)
> +			goto error;

11. Why you moved it to the bottom and added label? I can see only one
goto error, so it makes no sense to move this code.
> @@ -1375,7 +1366,7 @@ estimateTableWidth(Table * pTab)
>   	unsigned wTable = 0;
>   	const Column *pTabCol;
>   	int i;
> -	for (i = pTab->nCol, pTabCol = pTab->aCol; i > 0; i--, pTabCol++) {
> +	for (i = pTab->def->field_count, pTabCol = pTab->aCol; i > 0; i--, pTabCol++) {

12. Out of 80 symbols.
> @@ -2950,7 +2944,7 @@ sqlite3CreateIndex(Parse * pParse,	/* All information about this parse */
>   	 */
>   	if (pList == 0) {
>   		Token prevCol;
> -		sqlite3TokenInit(&prevCol, pTab->aCol[pTab->nCol - 1].zName);
> +		sqlite3TokenInit(&prevCol, pTab->def->fields[pTab->def->field_count - 1].name);

13. Same as 12.
> diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h
> index 7ce5bac..df26bcc 100644
> --- a/src/box/sql/sqliteInt.h
> +++ b/src/box/sql/sqliteInt.h
> @@ -4156,4 +4154,15 @@ table_column_nullable_action(struct Table *tab, uint32_t column);
>   bool
>   table_column_is_nullable(struct Table *tab, uint32_t column);
>   
> +/**
> + * 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.
> + */
> +struct field_def *
> +sql_field_retrieve(Parse *pParse, Table *pTable, uint32_t id);

14. Why? This function is used in build.c only and can be static.