[tarantool-patches] Re: [PATCH v1 1/1] sql: assertion fault on select after analyze

[n.pettik]

> On 6 Jul 2018, at 16:30, Kirill Shcherbatov <kshcherbatov at tarantool.org> wrote:
> 
> Binary search in whereKeyStats function that use routine

Typo: uses

> sqlite3VdbeRecordCompareMsgpack returned invalid result,
> because it made wrong assumption about the number of
> coinciding index columns based on _sql_stat4.
> Bat all collected index statistics were wrong.

Typo: But.

> 
> The analyzeOneTable routine used to call sqlite3GetTempRange
> and sqlite3ReleaseTempRange functions inside of cycle intensively
> changes Parser->nMem value. Thanks this pattern routine
> sqlite3ReleaseTempRange could push nMem variable onto aTempReg
> queue. As Parser->nMem cound be bigger next time, this

Cound —> count?

> variable may be used twice:
> 
>          [74, 'Column', 4, 1, 11, '', '00', 'r[11]=']
>          [75, 'Column', 4, 2, 12, '', '00', 'r[12]=']
> |->       [76, 'Column', 4, 3, 13, '', '00', 'r[13]=']
>          [77, 'Column', 4, 4, 14, '', '00', 'r[14]=']
> !rewrite  [78, 'Column', 4, 0, 13, '', '00', 'r[13]=ID']
>          [79, 'MakeRecord', 13, 1, 6, '', '00',
>           'r[6]=mkrec(r[13])']
> 
> Thus all assumptions about data below should be invalid and
> some artifacts could exhibit.
> 
> Example:
> \set language sql
> CREATE TABLE t1(id INTEGER PRIMARY KEY AUTOINCREMENT, a,
> 		b, c, f, d, e);
> CREATE INDEX i1 ON t1(a, b, c, f, d);
> \set language lua
> for i = 0, 100 do
>    box.sql.execute(string.format("INSERT INTO t1 VALUES(null,
> 		    'x', 'y', 'z', 'a', %s, %s);", i,
> 		    math.floor(i / 2)))
> end
> \set language sql
> SELECT * FROM "_sql_stat4";
> ---
> ['T1', 'I1', '101 101 1 1 1', '0 0 44 44 44', '0 0 44 44 44',
> ['x', 'y', 'z', 'a', 44]]
> ---
> 
> Expected:
> ---
> ['T1', 'I1', '101 101 101 101 1', '0 0 0 0 44', '0 0 0 0 44',
> ['x', 'y', 'z', 'a', 44]]
> ---
> 
> Resolves #2847.
> —

Thanks for investigating and fixing this bug. But problem seems to be deeper
(as you can notice). Am OK with this particular fix, but we must unify
usage of nMem and range of registers (i.e. whether use everywhere raw
nMem and get rid of TempReg function or vice versa).
Please, open an issue describing misusage of sqlite3GetTempReg() and parser->nMem.

> diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
> index 5f73f02..e8eec76 100644
> --- a/src/box/sql/analyze.c
> +++ b/src/box/sql/analyze.c
> @@ -1024,7 +1024,10 @@ analyzeOneTable(Parse * pParse,	/* Parser context */
> 		Index *pPk = sqlite3PrimaryKeyIndex(pIdx->pTable);
> 		int j, k, regKeyStat;
> 		int nPkColumn = (int)index_column_count(pPk);
> -		regKeyStat = sqlite3GetTempRange(pParse, nPkColumn);
> +		/* Allocate memory for array. */
> +		pParse->nMem = MAX(pParse->nMem, regPrev + nColTest + nPkColumn);
> +		regKeyStat = regPrev + nColTest;
> +
> 		for (j = 0; j < nPkColumn; j++) {
> 			k = pPk->aiColumn[j];
> 			assert(k >= 0 && k < (int)pTab->def->field_count);
> @@ -1034,7 +1037,6 @@ analyzeOneTable(Parse * pParse,	/* Parser context */
> 		}
> 		sqlite3VdbeAddOp3(v, OP_MakeRecord, regKeyStat,
> 				  nPkColumn, regKey);
> -		sqlite3ReleaseTempRange(pParse, regKeyStat, nPkColumn);
> 
> 		assert(regChng == (regStat4 + 1));
> diff --git a/test/sql/analyze.test.lua b/test/sql/analyze.test.lua

Don’t create separate file for one issue - there already exist
analyze[1-F] tests. Put this to one of them.