[tarantool-patches] Re: [PATCH v1 1/2] sql: get rid off sqlite3NestedParse in clean stats

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

Hello. Thanks for the patch!

I have pushed my review fixes on the branch. Please, squash, if
you agree, and debug if the tests fail. Below you may find 3
comments that I fixed. Style violations I did not mentioned here.
Just fixed on the branch.

Also I have found that vdbe_emit_open_cursor() has the second
parameter named 'index_id', but in some places the function
takes real index_id, in other places it takes tnum, and in vdbe
it is interpreted as tnum. Please, fix this mess in a separate
commit. I think, we should always pass index_id.

On 04/07/2018 20:17, Kirill Shcherbatov wrote:
> Now we manually generate AST structures to drop outdated
> stats from _sql_stat1 and _sql_stat4 spaces instead of
> starting sqlite3NestedParse. This function become totally
> useless and could be removed.
> 
> Part of #3496.
> ---
>   src/box/sql/analyze.c   | 39 +++++++++++------------
>   src/box/sql/build.c     | 82 ++++++++++++++++++++++++++++++++++++-------------
>   src/box/sql/sqliteInt.h | 13 ++++++++
>   3 files changed, 94 insertions(+), 40 deletions(-)
> 
> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
> index 5f73f02..e08c151 100644
> --- a/src/box/sql/analyze.c
> +++ b/src/box/sql/analyze.c
> @@ -116,7 +116,7 @@
>   #include "tarantoolInt.h"
>   #include "vdbeInt.h"
>   
> -/*
> +/**
>    * This routine generates code that opens the sql_statN tables.
>    * The _sql_stat1 table is always relevant. _sql_stat4 is only opened when
>    * appropriate compile-time options are provided.
> @@ -160,10 +163,9 @@ openStatTable(Parse * pParse,	/* Parsing context */
>   		assert(pStat != NULL);
>   		aRoot[i] = pStat->tnum;
>   		aCreateTbl[i] = 0;

1. Unused array.

2. You do not need to lookup pStat for tnum in non-debug build since
you know stat tables id from schema_def.h. This HashFind is useful for
debug assertion only.

> diff --git a/src/box/sql/build.c b/src/box/sql/build.c
> index 0072f84..ac53906 100644
> --- a/src/box/sql/build.c
> +++ b/src/box/sql/build.c
> @@ -2050,6 +2050,63 @@ sql_store_select(struct Parse *parse_context, struct Select *select)
>   }
>   
>   /**
> + * Create expression record of with struct ID EQ STRING.
> + *
> + * @param parse The parsing context.
> + * @param col_type_name Name of column.
> + * @param col_name Name of row.
> + * @retval not NULL on success.
> + * @retval NULL on failure.
> + */
> +static struct Expr *
> +sql_id_eq_str_expr(struct Parse *parse, const char *col_type_name,
> +		   const char *col_name)
> +{
> +	struct sqlite3 *db = parse->db;
> +
> +	struct Expr *col_type_expr =
> +		sqlite3Expr(db, TK_ID, col_type_name);
> +	struct Expr *col_name_expr =
> +		sqlite3Expr(db, TK_STRING, col_name);
> +	struct Expr *col_eq_expr =
> +		sqlite3PExpr(parse, TK_EQ, col_type_expr, col_name_expr);
> +	if (col_type_expr == NULL || col_name_expr == NULL) {

3. If col_eq_expr == NULL here, then col_type/name_expr leak.

> +		sql_expr_delete(db, col_eq_expr, false);
> +		col_eq_expr = NULL;
> +	}
> +	return col_eq_expr;
> +}
> +