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

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

Thank you for review.

> Typo: becomes.
Ok, tnx.

> You already have argument ’table_name’, so it looks confusing.
> Lets name it like ‘stat_names’. The same for space_ids.
-       const char *space_names[] = {"_sql_stat1", "_sql_stat4"};
-       const uint32_t space_ids[] = {BOX_SQL_STAT1_ID, BOX_SQL_STAT4_ID};
+       const char *stat_names[] = {"_sql_stat1", "_sql_stat4"};
+       const uint32_t stat_ids[] = {BOX_SQL_STAT1_ID, BOX_SQL_STAT4_ID};

> But comment below says that ’tables already exist since they are system’.
Hmm, loks like some legacy. 
-
-       /*
-        * Create new statistic tables if they do not exist, or
-        * clear them if they do already exist.
-        */

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

-               if (table_name != NULL || index_name != NULL) {
+               if (table_name != NULL) {

/**
 * Generate code that will do an analysis of a single table in
 * a database.
 *
 * @param parse Parser context.
 * @param table Target table to analyze.
 */
static void
vdbe_emit_analyze_table(struct Parse *parse, struct Table *table)
{
	assert(table != NULL);
	sql_set_multi_write(parse, false);
	int stat_cursor = parse->nTab;
	parse->nTab += 3;
	vdbe_emit_stat_space_open(parse, stat_cursor, NULL, table->def->name);
	analyzeOneTable(parse, table, NULL, stat_cursor, parse->nMem + 1,
			parse->nTab);
	loadAnalysis(parse);
}


> OP_Clear uses only first operand, so you can use sqlite3VdbeAddOp1();
-                       sqlite3VdbeAddOp2(v, OP_Clear, stat_ids[i], 0);
+                       sqlite3VdbeAddOp1(v, OP_Clear, stat_ids[i]);