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

["n.pettik" <korablev@tarantool.org>]

> On 10 Jul 2018, at 20:08, Kirill Shcherbatov <kshcherbatov@tarantool.org> 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

Typo: becomes.

> 
> --- a/src/box/sql/analyze.c
> +++ b/src/box/sql/analyze.c
> @@ -116,71 +116,53 @@
> +vdbe_emit_stat_space_open(struct Parse *parse, int stat_cursor,
> +			  const char *index_name, const char *table_name)
> {
> -	const char *aTable[] = {
> -		"_sql_stat1",
> -		"_sql_stat4",
> -		NULL};
> -	int i;
> -	sqlite3 *db = pParse->db;
> -	Vdbe *v = sqlite3GetVdbe(pParse);
> -	int aRoot[ArraySize(aTable)];
> -	u8 aCreateTbl[ArraySize(aTable)];
> +	const char *space_names[] = {"_sql_stat1", "_sql_stat4"};

You already have argument ’table_name’, so it looks confusing.
Lets name it like ‘stat_names’. The same for space_ids.

> +	const uint32_t space_ids[] = {BOX_SQL_STAT1_ID, BOX_SQL_STAT4_ID};
> +	struct Vdbe *v = sqlite3GetVdbe(parse);
> +	assert(v != NULL);
> +	assert(sqlite3VdbeDb(v) == parse->db);
> 
> -	if (v == 0)
> -		return;
> -	assert(sqlite3VdbeDb(v) == db);
> -
> -	/* Create new statistic tables if they do not exist, or clear them
> -	 * if they do already exist.
> +	/*
> +	 * Create new statistic tables if they do not exist, or

But comment below says that ’tables already exist since they are system’.

> +	 * clear them if they do already exist.
> 	 */
> -	for (i = 0; aTable[i]; i++) {
> -		const char *zTab = aTable[i];
> -		Table *pStat;
> -		/* The table already exists, because it is a system space */
> -		pStat = sqlite3HashFind(&db->pSchema->tblHash, zTab);
> -		assert(pStat != NULL);
> -		aRoot[i] = pStat->tnum;
> -		aCreateTbl[i] = 0;
> -		if (zWhere) {
> -			sqlite3NestedParse(pParse,
> -					   "DELETE FROM \"%s\" WHERE \"%s\"=%Q",
> -					   zTab, zWhereType, zWhere);
> +	for (uint i = 0; i < lengthof(space_names); ++i) {
> +		const char *space_name = space_names[i];
> +		/*
> +		 * The table already exists, because it is a
> +		 * system space.
> +		 */
> +		assert(sqlite3HashFind(&parse->db->pSchema->tblHash,
> +				       space_name) != NULL);
> +		if (table_name != NULL || index_name != NULL) {

I don’t understand situation when index_name != NULL but table_name == NULL.
In our SQL index name is local to table, (i.e. indexes with the same name could
exist within different tables). I see that vdbe_emit_stat_space_open() is called twice
in analyzeTable() and analyzeTable() in turn always called with  pOnlyIdx == NULL.
Hence, in vdbe_emit_stat_space_open() index_name is always == NULL.
Lets remove this dead code and excess arg.

> +			vdbe_emit_stat_space_clear(parse, space_name,
> +						   index_name, table_name);
> 		} else {
> -			/*
> -			 * The sql_stat[134] table already exists.
> -			 * Delete all rows.
> -			 */
> -			sqlite3VdbeAddOp2(v, OP_Clear,
> -					  SQLITE_PAGENO_TO_SPACEID(aRoot[i]), 0);
> +			sqlite3VdbeAddOp2(v, OP_Clear, space_ids[i], 0);

OP_Clear uses only first operand, so you can use sqlite3VdbeAddOp1();