[tarantool-patches] Re: [PATCH 3/7] sql: remove struct Table from analyze routine

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

Thanks for the patch! See 1 comment below.

On 23/08/2018 19:55, Nikita Pettik wrote:
> Instead of using struct Table which is found by lookup in internal hash
> lets use struct space from Tarantool DD.
> 
> Part of #3561
> ---
>   src/box/sql/analyze.c          | 185 +++++++++++++++++++++++------------------
>   test/sql-tap/analyze1.test.lua |   2 +-
>   2 files changed, 107 insertions(+), 80 deletions(-)
> 
> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
> index abed9d31b..e835a8697 100644
> --- a/src/box/sql/analyze.c
> +++ b/src/box/sql/analyze.c
> @@ -770,61 +764,62 @@ callStatGet(Vdbe * v, int regStat4, int iParam, int regOut)
>   	sqlite3VdbeChangeP5(v, 2);
>   }
>   
> -/*
> +/**
>    * Generate code to do an analysis of all indices associated with
>    * a single table.
> + *
> + * @param pParse Current parsing context.
> + * @param space Space to be analyzed.
> + * @param stat_cursor Cursor pointing to spaces containing
> + *        statistics: _sql_stat1 (stat_cursor) and
> + *        _sql_stat4 (stat_cursor + 1).
>    */
>   static void
> -analyzeOneTable(Parse * pParse,	/* Parser context */
> -		Table * pTab,	/* Table whose indices are to be analyzed */
> -		int iStatCur,	/* Index of VdbeCursor that writes the _sql_stat1 table */
> -		int iMem,	/* Available memory locations begin here */
> -		int iTab	/* Next available cursor */
> -    )
> +vdbe_emit_analyze_space(struct Parse *pParse, struct space *space,
> +			int stat_cursor)

1. Once you started this function formatting, please, finish it.
Use parse instead of pParse. Use reg_stat4 instead of regStat4 etc.

>   {
>   	sqlite3 *db = pParse->db;	/* Database handle */
>   	int iIdxCur;		/* Cursor open on index being analyzed */
>   	int iTabCur;		/* Table cursor */
>   	Vdbe *v;		/* The virtual machine being built up */
>   	int i;			/* Loop counter */
> -	int regStat4 = iMem++;	/* Register to hold Stat4Accum object */
> -	int regChng = iMem++;	/* Index of changed index field */
> -	int regKey = iMem++;	/* Key argument passed to stat_push() */
> -	int regTemp = iMem++;	/* Temporary use register */
> -	int regTabname = iMem++;	/* Register containing table name */
> -	int regIdxname = iMem++;	/* Register containing index name */
> -	int regStat1 = iMem++;	/* Value for the stat column of _sql_stat1 */
> -	int regPrev = iMem;	/* MUST BE LAST (see below) */
> -
> -	pParse->nMem = MAX(pParse->nMem, iMem);
> +	/* Register to hold Stat4Accum object. */
> +	int regStat4 = ++pParse->nMem;
> +	/* Index of changed index field. */
> +	int regChng = ++pParse->nMem;
> +	/* Key argument passed to stat_push(). */
> +	int regKey = ++pParse->nMem;
> +	/* Temporary use register. */
> +	int regTemp = ++pParse->nMem;
> +	/* Register containing table name. */
> +	int regTabname = ++pParse->nMem;
> +	/* Register containing index name. */
> +	int regIdxname = ++pParse->nMem;
> +	/* Value for the stat column of _sql_stat1. */
> +	int regStat1 = ++pParse->nMem;
> +	/* MUST BE LAST (see below). */
> +	int regPrev = ++pParse->nMem;
> +
> @@ -1076,27 +1078,43 @@ loadAnalysis(Parse * pParse)
>   	}
>   }
>   
> -/*
> - * Generate code that will do an analysis of an entire database
> +/**
> + * Wrapper to pass args to space_foreach callback.
> + */
> +struct analyze_data {
> +	struct Parse *parse_context;
> +	/**
> +	 * Cursor numbers pointing to stat spaces:
> +	 * stat_cursor is opened on _sql_stat1 and
> +	 * stat_cursor + 1 - on _sql_stat4.
> +	 */
> +	int stat_cursor;
> +};
> +
> +static int
> +sql_space_foreach_analyze(struct space *space, void *data)
> +{
> +	if (space->def->opts.sql == NULL || space->def->opts.is_view)
> +		return 0;
> +	struct analyze_data *anal_data = (struct analyze_data *) data;

Lol

> +	vdbe_emit_analyze_space(anal_data->parse_context, space,
> +				anal_data->stat_cursor);
> +	return 0;
> +}
> +