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

  • From: "n.pettik" <korablev@xxxxxxxxxxxxx>
  • To: tarantool-patches@xxxxxxxxxxxxx
  • Date: Fri, 13 Jul 2018 18:43:50 +0300


On 6 Jul 2018, at 16:30, Kirill Shcherbatov <kshcherbatov@xxxxxxxxxxxxx> 
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.


Other related posts:

  1. Home
  2. tarantool-patches
  3. Archive
  4. 07-2018