Tarantool development patches archive
 help / color / mirror / Atom feed
From: Kirill Shcherbatov <kshcherbatov@tarantool.org>
To: tarantool-patches@freelists.org, Nikita Pettik <korablev@tarantool.org>
Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: assertion fault on select after analyze
Date: Fri, 13 Jul 2018 20:07:45 +0300	[thread overview]
Message-ID: <3d062632-7542-6694-f986-3f8074eba6d9@tarantool.org> (raw)
In-Reply-To: <FB63880C-F71E-4686-AE17-1D6DE63455D8@tarantool.org>

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

  reply	other threads:[~2018-07-13 17:07 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-07-06 13:30 [tarantool-patches] " Kirill Shcherbatov
2018-07-13 15:43 ` [tarantool-patches] " n.pettik
2018-07-13 17:07   ` Kirill Shcherbatov [this message]
2018-07-13 19:19     ` n.pettik
2018-07-17 10:56 ` Kirill Yukhin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3d062632-7542-6694-f986-3f8074eba6d9@tarantool.org \
    --to=kshcherbatov@tarantool.org \
    --cc=korablev@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='[tarantool-patches] Re: [PATCH v1 1/1] sql: assertion fault on select after analyze' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox