From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id E02CF27445 for ; Fri, 13 Jul 2018 13:07:49 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id EGbkPxcthEBa for ; Fri, 13 Jul 2018 13:07:49 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id E49FB271A6 for ; Fri, 13 Jul 2018 13:07:48 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v1 1/1] sql: assertion fault on select after analyze References: <84f7314ef474adcd50bd68738f2df301dbc08662.1530883770.git.kshcherbatov@tarantool.org> From: Kirill Shcherbatov Message-ID: <3d062632-7542-6694-f986-3f8074eba6d9@tarantool.org> Date: Fri, 13 Jul 2018 20:07:45 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, Nikita Pettik 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" -- }) + +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, { + -- + 21 + -- +}) + +test:do_execsql_test( + "analyze-6.1.2", + [[ + SELECT * FROM "_sql_stat1" where "tbl"='T1' and "idx"='I1' LIMIT 1; + ]], { + -- + "T1", "I1", "221 221 221 221 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; + ]], { + -- + "T1", "I1", "221 221 221 1", "0 0 0 10", "0 0 0 10" + -- +}) + +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; + ]], { + -- + "T1", "I1", "221 221 221 1", "0 0 0 99", "0 0 0 99" + -- +}) + -- # 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( ]] , { -- - "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" -- }) 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" -- }) @@ -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" -- }) -- 2.7.4