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

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

[Kirill Shcherbatov]

Binary search in whereKeyStats function that use routine
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.

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

https://github.com/tarantool/tarantool/compare/kshch/gh-2847-assertion-fault-on-analyze
https://github.com/tarantool/tarantool/issues/2847

 src/box/sql/analyze.c          |   6 +-
 test/sql-tap/analyze4.test.lua |   2 +-
 test/sql-tap/analyze9.test.lua |  16 ++---
 test/sql/analyze.result        | 152 +++++++++++++++++++++++++++++++++++++++++
 test/sql/analyze.test.lua      |  28 ++++++++
 5 files changed, 193 insertions(+), 11 deletions(-)
 create mode 100644 test/sql/analyze.result
 create mode 100644 test/sql/analyze.test.lua

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));
 		sqlite3VdbeAddOp4(v, OP_Function0, 1, regStat4, regTemp,
diff --git a/test/sql-tap/analyze4.test.lua b/test/sql-tap/analyze4.test.lua
index f71d394..c2cc190 100755
--- a/test/sql-tap/analyze4.test.lua
+++ b/test/sql-tap/analyze4.test.lua
@@ -115,7 +115,7 @@ test:do_execsql_test(
     ]]
     , {
         -- <analyze4-1.3>
-        "T1","128 1", "T1A", "128 1", "T1B", "128 128", "T1BCD", "128 128 4 2", "T1CBD", "128 4 4 2", "T1CDB", "128 4 2 1"
+        "T1","128 1", "T1A", "128 1", "T1B", "128 128", "T1BCD", "128 128 4 2", "T1CBD", "128 4 4 2", "T1CDB", "128 4 2 2"
         -- </analyze4-1.3>
     })
 
diff --git a/test/sql-tap/analyze9.test.lua b/test/sql-tap/analyze9.test.lua
index 28a6c20..1dbfe5d 100755
--- a/test/sql-tap/analyze9.test.lua
+++ b/test/sql-tap/analyze9.test.lua
@@ -287,13 +287,13 @@ test:do_execsql_test(
             FROM "_sql_stat4" WHERE "idx" = 'I1' ORDER BY "sample" LIMIT 16;
     ]], {
         -- <4.3>
-        "10 10 1","0 0 8","0 0 8","0 0 0","10 10 1","10 10 10","1 1 10","1 1 1","10 10 1","20 20 28",
-        "2 2 28","2 2 2","10 10 1","30 30 33","3 3 33","3 3 3","10 10 1","40 40 45","4 4 45","4 4 4",
-        "10 10 1","50 50 51","5 5 51","5 5 5","10 10 1","60 60 67","6 6 67","6 6 6","10 10 1","70 70 74",
-        "7 7 74","7 7 7","10 10 1","80 80 82","8 8 82","8 8 8","10 10 1","90 90 96","9 9 96","9 9 9",
-        "10 10 1","100 100 101","10 10 101","10 10 10","10 10 1","110 110 116","11 11 116","11 11 11",
-        "10 10 1","120 120 122","12 12 122","12 12 12","10 10 1","130 130 135","13 13 135","13 13 13",
-        "10 10 1","140 140 148","14 14 148","14 14 14","10 10 1","150 150 151","15 15 151","15 15 15"
+        "10 10 10","0 0 0","0 0 0","0 0 0","10 10 10","10 10 10","1 1 1","1 1 1","10 10 10","20 20 20",
+        "2 2 2","2 2 2","10 10 10","30 30 30","3 3 3","3 3 3","10 10 10","40 40 40","4 4 4","4 4 4",
+        "10 10 10","50 50 50","5 5 5","5 5 5","10 10 10","60 60 60","6 6 6","6 6 6","10 10 10","70 70 70",
+        "7 7 7","7 7 7","10 10 10","80 80 80","8 8 8","8 8 8","10 10 10","90 90 90","9 9 9","9 9 9",
+        "10 10 10","100 100 100","10 10 10","10 10 10","10 10 10","110 110 110","11 11 11","11 11 11",
+        "10 10 10","120 120 120","12 12 12","12 12 12","10 10 10","130 130 130","13 13 13","13 13 13",
+        "10 10 10","140 140 140","14 14 14","14 14 14","10 10 10","150 150 150","15 15 15","15 15 15"
         -- </4.3>
     })
 
@@ -304,7 +304,7 @@ test:do_execsql_test(
         FROM "_sql_stat4" WHERE "idx" = 'I1' ORDER BY "sample" DESC LIMIT 2;
     ]], {
         -- <4.4>
-        "2 1 1","295 296 296","120 122 296","201 4 h","5 3 1","290 290 291","119 119 291","200 1 b"
+        "2 1 1","295 296 296","120 122 125","201 4 h","5 3 1","290 290 291","119 119 120","200 1 b"
         -- </4.4>
     })
 
diff --git a/test/sql/analyze.result b/test/sql/analyze.result
new file mode 100644
index 0000000..cbe7971
--- /dev/null
+++ b/test/sql/analyze.result
@@ -0,0 +1,152 @@
+test_run = require('test_run').new()
+---
+...
+engine = test_run:get_cfg('engine')
+---
+...
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+--
+-- gh-2847: assertion fault on select after analyze
+--
+box.sql.execute('pragma sql_default_engine=\''..engine..'\'')
+
+
+box.cfg{};
+---
+...
+box.sql.execute("DROP TABLE IF EXISTS t1;");
+---
+...
+box.sql.execute("CREATE TABLE t1(id INTEGER PRIMARY KEY AUTOINCREMENT, a, b, c, d, e);");
+---
+...
+box.sql.execute("CREATE INDEX i1 ON t1(a, b, c, d);");
+---
+...
+box.sql.execute("CREATE INDEX i2 ON t1(e);");
+---
+...
+for i = 0, 100 do
+    box.sql.execute(string.format("INSERT INTO t1 VALUES(null, 'x', 'y', 'z', %s, %s);", i, math.floor(i / 2)))
+end;
+---
+...
+for i = 0, 20 do
+    box.sql.execute("INSERT INTO t1 VALUES(null, 'x', 'y', 'z', 101, "..i..");")
+end;
+---
+...
+for i = 102, 200 do
+    box.sql.execute(string.format("INSERT INTO t1 VALUES(null, 'x', 'y', 'z', %s, %s);", i, math.floor(i / 2)))
+end;
+---
+...
+box.sql.execute("ANALYZE;")
+box.sql.execute("SELECT * FROM \"_sql_stat1\"");
+---
+...
+box.sql.execute("SELECT * FROM \"_sql_stat4\"");
+---
+- - ['T1', 'I1', '221 221 221 1', '0 0 0 10', '0 0 0 10', !!binary lKF4oXmhego=]
+  - ['T1', 'I1', '221 221 221 1', '0 0 0 24', '0 0 0 24', !!binary lKF4oXmhehg=]
+  - ['T1', 'I1', '221 221 221 1', '0 0 0 25', '0 0 0 25', !!binary lKF4oXmhehk=]
+  - ['T1', 'I1', '221 221 221 1', '0 0 0 32', '0 0 0 32', !!binary lKF4oXmheiA=]
+  - ['T1', 'I1', '221 221 221 1', '0 0 0 35', '0 0 0 35', !!binary lKF4oXmheiM=]
+  - ['T1', 'I1', '221 221 221 1', '0 0 0 44', '0 0 0 44', !!binary lKF4oXmheiw=]
+  - ['T1', 'I1', '221 221 221 1', '0 0 0 49', '0 0 0 49', !!binary lKF4oXmhejE=]
+  - ['T1', 'I1', '221 221 221 1', '0 0 0 56', '0 0 0 56', !!binary lKF4oXmhejg=]
+  - ['T1', 'I1', '221 221 221 1', '0 0 0 62', '0 0 0 62', !!binary lKF4oXmhej4=]
+  - ['T1', 'I1', '221 221 221 1', '0 0 0 66', '0 0 0 66', !!binary lKF4oXmhekI=]
+  - ['T1', 'I1', '221 221 221 1', '0 0 0 74', '0 0 0 74', !!binary lKF4oXmheko=]
+  - ['T1', 'I1', '221 221 221 1', '0 0 0 80', '0 0 0 80', !!binary lKF4oXmhelA=]
+  - ['T1', 'I1', '221 221 221 1', '0 0 0 99', '0 0 0 99', !!binary lKF4oXmhemM=]
+  - ['T1', 'I1', '221 221 221 21', '0 0 0 101', '0 0 0 101', !!binary lKF4oXmhemU=]
+  - ['T1', 'I1', '221 221 221 1', '0 0 0 124', '0 0 0 104', !!binary lKF4oXmhemg=]
+  - ['T1', 'I1', '221 221 221 1', '0 0 0 149', '0 0 0 129', !!binary lKF4oXmhesyB]
+  - ['T1', 'I1', '221 221 221 1', '0 0 0 157', '0 0 0 137', !!binary lKF4oXmhesyJ]
+  - ['T1', 'I1', '221 221 221 1', '0 0 0 174', '0 0 0 154', !!binary lKF4oXmhesya]
+  - ['T1', 'I1', '221 221 221 1', '0 0 0 176', '0 0 0 156', !!binary lKF4oXmhesyc]
+  - ['T1', 'I1', '221 221 221 1', '0 0 0 185', '0 0 0 165', !!binary lKF4oXmhesyl]
+  - ['T1', 'I1', '221 221 221 1', '0 0 0 192', '0 0 0 172', !!binary lKF4oXmhesys]
+  - ['T1', 'I1', '221 221 221 1', '0 0 0 199', '0 0 0 179', !!binary lKF4oXmhesyz]
+  - ['T1', 'I1', '221 221 221 1', '0 0 0 210', '0 0 0 190', !!binary lKF4oXmhesy+]
+  - ['T1', 'I1', '221 221 221 1', '0 0 0 219', '0 0 0 199', !!binary lKF4oXmheszH]
+  - ['T1', 'I2', '3', '0', '0', !!binary kQA=]
+  - ['T1', 'I2', '3', '3', '1', !!binary kQE=]
+  - ['T1', 'I2', '3', '6', '2', !!binary kQI=]
+  - ['T1', 'I2', '3', '12', '4', !!binary kQQ=]
+  - ['T1', 'I2', '3', '15', '5', !!binary kQU=]
+  - ['T1', 'I2', '3', '18', '6', !!binary kQY=]
+  - ['T1', 'I2', '3', '21', '7', !!binary kQc=]
+  - ['T1', 'I2', '3', '24', '8', !!binary kQg=]
+  - ['T1', 'I2', '3', '27', '9', !!binary kQk=]
+  - ['T1', 'I2', '3', '30', '10', !!binary kQo=]
+  - ['T1', 'I2', '3', '33', '11', !!binary kQs=]
+  - ['T1', 'I2', '3', '36', '12', !!binary kQw=]
+  - ['T1', 'I2', '3', '39', '13', !!binary kQ0=]
+  - ['T1', 'I2', '3', '42', '14', !!binary kQ4=]
+  - ['T1', 'I2', '3', '45', '15', !!binary kQ8=]
+  - ['T1', 'I2', '3', '48', '16', !!binary kRA=]
+  - ['T1', 'I2', '3', '57', '19', !!binary kRM=]
+  - ['T1', 'I2', '3', '60', '20', !!binary kRQ=]
+  - ['T1', 'I2', '2', '73', '26', !!binary kRo=]
+  - ['T1', 'I2', '2', '99', '39', !!binary kSc=]
+  - ['T1', 'I2', '2', '124', '52', !!binary kTQ=]
+  - ['T1', 'I2', '2', '148', '64', !!binary kUA=]
+  - ['T1', 'I2', '2', '174', '77', !!binary kU0=]
+  - ['T1', 'I2', '2', '198', '89', !!binary kVk=]
+  - ['T1', 'T1', '1', '2', '2', !!binary kQM=]
+  - ['T1', 'T1', '1', '6', '6', !!binary kQc=]
+  - ['T1', 'T1', '1', '12', '12', !!binary kQ0=]
+  - ['T1', 'T1', '1', '24', '24', !!binary kRk=]
+  - ['T1', 'T1', '1', '31', '31', !!binary kSA=]
+  - ['T1', 'T1', '1', '33', '33', !!binary kSI=]
+  - ['T1', 'T1', '1', '34', '34', !!binary kSM=]
+  - ['T1', 'T1', '1', '43', '43', !!binary kSw=]
+  - ['T1', 'T1', '1', '49', '49', !!binary kTI=]
+  - ['T1', 'T1', '1', '66', '66', !!binary kUM=]
+  - ['T1', 'T1', '1', '74', '74', !!binary kUs=]
+  - ['T1', 'T1', '1', '94', '94', !!binary kV8=]
+  - ['T1', 'T1', '1', '99', '99', !!binary kWQ=]
+  - ['T1', 'T1', '1', '101', '101', !!binary kWY=]
+  - ['T1', 'T1', '1', '108', '108', !!binary kW0=]
+  - ['T1', 'T1', '1', '112', '112', !!binary kXE=]
+  - ['T1', 'T1', '1', '124', '124', !!binary kX0=]
+  - ['T1', 'T1', '1', '127', '127', !!binary kcyA]
+  - ['T1', 'T1', '1', '149', '149', !!binary kcyW]
+  - ['T1', 'T1', '1', '152', '152', !!binary kcyZ]
+  - ['T1', 'T1', '1', '153', '153', !!binary kcya]
+  - ['T1', 'T1', '1', '174', '174', !!binary kcyv]
+  - ['T1', 'T1', '1', '187', '187', !!binary kcy8]
+  - ['T1', 'T1', '1', '199', '199', !!binary kczI]
+...
+box.sql.execute("SELECT * FROM t1 WHERE a='x' AND b='y' AND c='z' AND d=101");
+---
+- - [102, 'x', 'y', 'z', 101, 0]
+  - [103, 'x', 'y', 'z', 101, 1]
+  - [104, 'x', 'y', 'z', 101, 2]
+  - [105, 'x', 'y', 'z', 101, 3]
+  - [106, 'x', 'y', 'z', 101, 4]
+  - [107, 'x', 'y', 'z', 101, 5]
+  - [108, 'x', 'y', 'z', 101, 6]
+  - [109, 'x', 'y', 'z', 101, 7]
+  - [110, 'x', 'y', 'z', 101, 8]
+  - [111, 'x', 'y', 'z', 101, 9]
+  - [112, 'x', 'y', 'z', 101, 10]
+  - [113, 'x', 'y', 'z', 101, 11]
+  - [114, 'x', 'y', 'z', 101, 12]
+  - [115, 'x', 'y', 'z', 101, 13]
+  - [116, 'x', 'y', 'z', 101, 14]
+  - [117, 'x', 'y', 'z', 101, 15]
+  - [118, 'x', 'y', 'z', 101, 16]
+  - [119, 'x', 'y', 'z', 101, 17]
+  - [120, 'x', 'y', 'z', 101, 18]
+  - [121, 'x', 'y', 'z', 101, 19]
+  - [122, 'x', 'y', 'z', 101, 20]
+...
+box.sql.execute("DROP TABLE t1");
+---
+...
diff --git a/test/sql/analyze.test.lua b/test/sql/analyze.test.lua
new file mode 100644
index 0000000..efc1a68
--- /dev/null
+++ b/test/sql/analyze.test.lua
@@ -0,0 +1,28 @@
+test_run = require('test_run').new()
+engine = test_run:get_cfg('engine')
+test_run:cmd("setopt delimiter ';'")
+box.sql.execute('pragma sql_default_engine=\''..engine..'\'')
+
+--
+-- gh-2847: assertion fault on select after analyze
+--
+
+box.cfg{};
+box.sql.execute("DROP TABLE IF EXISTS t1;");
+box.sql.execute("CREATE TABLE t1(id INTEGER PRIMARY KEY AUTOINCREMENT, a, b, c, d, e);");
+box.sql.execute("CREATE INDEX i1 ON t1(a, b, c, d);");
+box.sql.execute("CREATE INDEX i2 ON t1(e);");
+for i = 0, 100 do
+    box.sql.execute(string.format("INSERT INTO t1 VALUES(null, 'x', 'y', 'z', %s, %s);", i, math.floor(i / 2)))
+end;
+for i = 0, 20 do
+    box.sql.execute("INSERT INTO t1 VALUES(null, 'x', 'y', 'z', 101, "..i..");")
+end;
+for i = 102, 200 do
+    box.sql.execute(string.format("INSERT INTO t1 VALUES(null, 'x', 'y', 'z', %s, %s);", i, math.floor(i / 2)))
+end;
+box.sql.execute("ANALYZE;")
+box.sql.execute("SELECT * FROM \"_sql_stat1\"");
+box.sql.execute("SELECT * FROM \"_sql_stat4\"");
+box.sql.execute("SELECT * FROM t1 WHERE a='x' AND b='y' AND c='z' AND d=101");
+box.sql.execute("DROP TABLE t1");
-- 
2.7.4

[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@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.

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

[Kirill Shcherbatov]

Hi! Tnx for review.

> Typo: uses
> Typo: But.
fixed.
> Cound —> count?
could. fixed.

> 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.
https://github.com/tarantool/tarantool/issues/3533

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

============================================


Binary search in whereKeyStats function that uses routine
sqlite3VdbeRecordCompareMsgpack returned invalid result,
because it made wrong assumption about the number of
coinciding index columns based on _sql_stat4.
But all collected index statistics were wrong.

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 could be bigger next time, this
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.
---
 src/box/sql/analyze.c          |  6 +++--
 test/sql-tap/analyze1.test.lua | 59 +++++++++++++++++++++++++++++++++++++++++-
 test/sql-tap/analyze4.test.lua |  2 +-
 test/sql-tap/analyze9.test.lua | 16 ++++++------
 4 files changed, 71 insertions(+), 12 deletions(-)

diff --git a/src/box/sql/analyze.c b/src/box/sql/analyze.c
index ea872f4..ea93bfd 100644
--- a/src/box/sql/analyze.c
+++ b/src/box/sql/analyze.c
@@ -997,7 +997,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);
@@ -1007,7 +1010,6 @@ analyzeOneTable(Parse * pParse,	/* Parser context */
 		}
 		sqlite3VdbeAddOp3(v, OP_MakeRecord, regKeyStat,
 				  nPkColumn, regKey);
-		sqlite3ReleaseTempRange(pParse, regKeyStat, nPkColumn);
 
 		assert(regChng == (regStat4 + 1));
 		sqlite3VdbeAddOp4(v, OP_Function0, 1, regStat4, regTemp,
diff --git a/test/sql-tap/analyze1.test.lua b/test/sql-tap/analyze1.test.lua
index 308f6b9..dab7255 100755
--- a/test/sql-tap/analyze1.test.lua
+++ b/test/sql-tap/analyze1.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(34)
+test:plan(38)
 
 --!./tcltestrunner.lua
 -- 2005 July 22
@@ -489,6 +489,63 @@ test:do_execsql_test(
         "T4"
         -- </analyze-5.5>
     })
+
+test:do_test(
+    "analyze-6.1.1",
+    function()
+        test:execsql("DROP TABLE IF EXISTS t1 ")
+        test:execsql("CREATE TABLE t1(id INTEGER PRIMARY KEY AUTOINCREMENT, a, b, c, d, e);")
+        test:execsql("CREATE INDEX i1 ON t1(a, b, c, d);")
+        test:execsql("CREATE INDEX i2 ON t1(e);")
+
+        for i = 0, 100 do
+            box.sql.execute(string.format("INSERT INTO t1 VALUES(null, 'x', 'y', 'z', %s, %s);", i, math.floor(i / 2)))
+        end;
+        for i = 0, 20 do
+            box.sql.execute("INSERT INTO t1 VALUES(null, 'x', 'y', 'z', 101, "..i..");")
+        end;
+        for i = 102, 200 do
+            box.sql.execute(string.format("INSERT INTO t1 VALUES(null, 'x', 'y', 'z', %s, %s);", i, math.floor(i / 2)))
+        end;
+
+        test:execsql("ANALYZE;")
+        return test:execsql("SELECT COUNT(* )FROM t1 WHERE a='x' AND b='y' AND c='z' AND d=101;;")
+    end, {
+    -- <analyze-6.1.1>
+    21
+    -- </analyze-6.1.1>
+})
+
+test:do_execsql_test(
+    "analyze-6.1.2",
+    [[
+            SELECT * FROM "_sql_stat1" where "tbl"='T1' and "idx"='I1' LIMIT 1;
+    ]], {
+    -- <analyze-6.1.2>
+    "T1", "I1", "221 221 221 221 2"
+    -- </analyze-6.1.2>
+})
+
+test:do_execsql_test(
+    "analyze-6.1.3",
+    [[
+            SELECT "tbl", "idx", "neq", "nlt", "ndlt" FROM "_sql_stat4" where "tbl"='T1' and "idx"='I1' ORDER BY "nlt" LIMIT 1;
+    ]], {
+    -- <analyze-6.1.3>
+    "T1", "I1", "221 221 221 1", "0 0 0 10", "0 0 0 10"
+    -- </analyze-6.1.3>
+})
+
+test:do_execsql_test(
+    "analyze-6.1.4",
+    [[
+            SELECT "tbl", "idx", "neq", "nlt", "ndlt" FROM "_sql_stat4" where "tbl"='T1' and "idx"='I1' ORDER BY "nlt" DESC LIMIT 1;
+    ]], {
+    -- <analyze-6.1.4>
+    "T1", "I1", "221 221 221 1", "0 0 0 99", "0 0 0 99"
+    -- </analyze-6.1.4>
+})
+
 -- # This test corrupts the database file so it must be the last test
 -- # in the series.
 -- #
diff --git a/test/sql-tap/analyze4.test.lua b/test/sql-tap/analyze4.test.lua
index f71d394..c2cc190 100755
--- a/test/sql-tap/analyze4.test.lua
+++ b/test/sql-tap/analyze4.test.lua
@@ -115,7 +115,7 @@ test:do_execsql_test(
     ]]
     , {
         -- <analyze4-1.3>
-        "T1","128 1", "T1A", "128 1", "T1B", "128 128", "T1BCD", "128 128 4 2", "T1CBD", "128 4 4 2", "T1CDB", "128 4 2 1"
+        "T1","128 1", "T1A", "128 1", "T1B", "128 128", "T1BCD", "128 128 4 2", "T1CBD", "128 4 4 2", "T1CDB", "128 4 2 2"
         -- </analyze4-1.3>
     })
 
diff --git a/test/sql-tap/analyze9.test.lua b/test/sql-tap/analyze9.test.lua
index 28a6c20..1dbfe5d 100755
--- a/test/sql-tap/analyze9.test.lua
+++ b/test/sql-tap/analyze9.test.lua
@@ -287,13 +287,13 @@ test:do_execsql_test(
             FROM "_sql_stat4" WHERE "idx" = 'I1' ORDER BY "sample" LIMIT 16;
     ]], {
         -- <4.3>
-        "10 10 1","0 0 8","0 0 8","0 0 0","10 10 1","10 10 10","1 1 10","1 1 1","10 10 1","20 20 28",
-        "2 2 28","2 2 2","10 10 1","30 30 33","3 3 33","3 3 3","10 10 1","40 40 45","4 4 45","4 4 4",
-        "10 10 1","50 50 51","5 5 51","5 5 5","10 10 1","60 60 67","6 6 67","6 6 6","10 10 1","70 70 74",
-        "7 7 74","7 7 7","10 10 1","80 80 82","8 8 82","8 8 8","10 10 1","90 90 96","9 9 96","9 9 9",
-        "10 10 1","100 100 101","10 10 101","10 10 10","10 10 1","110 110 116","11 11 116","11 11 11",
-        "10 10 1","120 120 122","12 12 122","12 12 12","10 10 1","130 130 135","13 13 135","13 13 13",
-        "10 10 1","140 140 148","14 14 148","14 14 14","10 10 1","150 150 151","15 15 151","15 15 15"
+        "10 10 10","0 0 0","0 0 0","0 0 0","10 10 10","10 10 10","1 1 1","1 1 1","10 10 10","20 20 20",
+        "2 2 2","2 2 2","10 10 10","30 30 30","3 3 3","3 3 3","10 10 10","40 40 40","4 4 4","4 4 4",
+        "10 10 10","50 50 50","5 5 5","5 5 5","10 10 10","60 60 60","6 6 6","6 6 6","10 10 10","70 70 70",
+        "7 7 7","7 7 7","10 10 10","80 80 80","8 8 8","8 8 8","10 10 10","90 90 90","9 9 9","9 9 9",
+        "10 10 10","100 100 100","10 10 10","10 10 10","10 10 10","110 110 110","11 11 11","11 11 11",
+        "10 10 10","120 120 120","12 12 12","12 12 12","10 10 10","130 130 130","13 13 13","13 13 13",
+        "10 10 10","140 140 140","14 14 14","14 14 14","10 10 10","150 150 150","15 15 15","15 15 15"
         -- </4.3>
     })
 
@@ -304,7 +304,7 @@ test:do_execsql_test(
         FROM "_sql_stat4" WHERE "idx" = 'I1' ORDER BY "sample" DESC LIMIT 2;
     ]], {
         -- <4.4>
-        "2 1 1","295 296 296","120 122 296","201 4 h","5 3 1","290 290 291","119 119 291","200 1 b"
+        "2 1 1","295 296 296","120 122 125","201 4 h","5 3 1","290 290 291","119 119 120","200 1 b"
         -- </4.4>
     })
 
-- 
2.7.4

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

[n.pettik]

LGTM.

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

[Kirill Yukhin]

Hello,
On 06 июл 16:30, Kirill Shcherbatov wrote:
> Resolves #2847.
> ---
> 
> https://github.com/tarantool/tarantool/compare/kshch/gh-2847-assertion-fault-on-analyze
> https://github.com/tarantool/tarantool/issues/2847
I've checked your patch into 2-0 branch.

--
Regards, Kirill Yukhin