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 05C1820FC7 for ; Wed, 26 Dec 2018 13:18:15 -0500 (EST) 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 DRYIoM1PmYTH for ; Wed, 26 Dec 2018 13:18:14 -0500 (EST) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 92473210E4 for ; Wed, 26 Dec 2018 13:18:14 -0500 (EST) From: imeevma@tarantool.org Subject: [tarantool-patches] [PATCH v3 2/6] sql: fix "PRAGMA parser_trace" result Date: Wed, 26 Dec 2018 21:18:12 +0300 Message-Id: MIME-Version: 1.0 In-Reply-To: References: Content-Type: text/plain; charset="utf-8" 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, korablev@tarantool.org Hi! Thank you for review! My answers, diff between versions and new version below. On 12/24/18 5:01 PM, n.pettik wrote: > Why is this patch part of #3832? > Couldn’t you implement #3832 without this patch? Removed this from commit-message. > Why can’t you simply set to this pragma type ‘flag’? > It works almost that way. Done. > Let's avoid using ’sqlite’ prefix in any added code. Fixed. > Personally I'd rather create new test file for this (debug) purpose. > Also, your test doesn’t check value which has been set (it checks > only that it is bool): > > Pragma parser_trace=1 > pragma parser_trace — this must return 1, not 0. Done. I created new test file. Diff between versions: commit 4b1229191fbf15fd9d0d9bb178e8ab8de2c3280f Author: Mergen Imeev Date: Tue Dec 25 18:19:35 2018 +0300 Temporary: review fixes diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c index b8edc76..6122986 100644 --- a/src/box/sql/pragma.c +++ b/src/box/sql/pragma.c @@ -466,33 +466,31 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ switch (pPragma->ePragTyp) { case PragTyp_FLAG:{ - if (zRight == 0) { - setPragmaResultColumnNames(v, pPragma); - returnSingleInt(v, - (user_session-> - sql_flags & pPragma->iArg) != - 0); - } else { - int mask = pPragma->iArg; /* Mask of bits to set - * or clear. - */ - - if (sqlite3GetBoolean(zRight, 0)) { - user_session->sql_flags |= mask; - } else { - user_session->sql_flags &= ~mask; - } - - /* Many of the flag-pragmas modify the code - * generated by the SQL * compiler (eg. - * count_changes). So add an opcode to expire - * all * compiled SQL statements after - * modifying a pragma value. - */ - sqlite3VdbeAddOp0(v, OP_Expire); + if (zRight == 0) { + setPragmaResultColumnNames(v, pPragma); + returnSingleInt(v, (user_session->sql_flags & + pPragma->iArg) != 0); + } else { + int mask = pPragma->iArg; /* Mask of bits to set + * or clear. + */ + bool is_value_true = sqlite3GetBoolean(zRight, 0); + + if (is_value_true) + user_session->sql_flags |= mask; + else + user_session->sql_flags &= ~mask; +#ifndef NDEBUG + if (mask == PARSER_TRACE_FLAG) { + if (is_value_true) + sqlite3ParserTrace(stdout, "parser: "); + else + sqlite3ParserTrace(0, 0); } - break; +#endif } + break; + } case PragTyp_TABLE_INFO: sql_pragma_table_info(pParse, zRight); @@ -569,24 +567,6 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ } break; } -#ifndef NDEBUG - case PragTyp_PARSER_TRACE:{ - int mask = pPragma->iArg; - if (zRight == NULL) { - returnSingleInt(v, - (user_session->sql_flags & mask) != 0); - } else { - if (sqlite3GetBoolean(zRight, 0)) { - sqlite3ParserTrace(stdout, "parser: "); - user_session->sql_flags |= mask; - } else { - sqlite3ParserTrace(0, 0); - user_session->sql_flags &= ~mask; - } - } - break; - } -#endif /* * Reinstall the LIKE and functions. The variant diff --git a/src/box/sql/pragma.h b/src/box/sql/pragma.h index 59e0e1e..ae8e030 100644 --- a/src/box/sql/pragma.h +++ b/src/box/sql/pragma.h @@ -14,7 +14,6 @@ #define PragTyp_INDEX_LIST 11 #define PragTyp_STATS 15 #define PragTyp_TABLE_INFO 17 -#define PragTyp_PARSER_TRACE 24 #define PragTyp_DEFAULT_ENGINE 25 #define PragTyp_COMPOUND_SELECT_LIMIT 26 @@ -137,10 +136,10 @@ static const PragmaName aPragmaName[] = { /* iArg: */ 0}, #if defined(SQLITE_DEBUG) { /* zName: */ "parser_trace", - /* ePragTyp: */ PragTyp_PARSER_TRACE, + /* ePragTyp: */ PragTyp_FLAG, /* ePragFlg: */ PragFlg_Result0 | PragFlg_NoColumns1, /* ColNames: */ 0, 0, - /* iArg: */ SQLITE_ParserTrace}, + /* iArg: */ PARSER_TRACE_FLAG}, #endif { /* zName: */ "query_only", /* ePragTyp: */ PragTyp_FLAG, diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 212f75a..a484486 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -1564,7 +1564,7 @@ struct sqlite3 { */ #define SQLITE_VdbeTrace 0x00000001 /* True to trace VDBE execution */ /* Debug print info about SQL query as it parsed */ -#define SQLITE_ParserTrace 0x00000002 +#define PARSER_TRACE_FLAG 0x00000002 #define SQLITE_FullColNames 0x00000004 /* Show full column names on SELECT */ #define SQLITE_ShortColNames 0x00000040 /* Show short columns names */ #define SQLITE_CountRows 0x00000080 /* Count rows changed by INSERT, */ diff --git a/test/sql/errinj.result b/test/sql/errinj.result index 6ba7e72..cb993f8 100644 --- a/test/sql/errinj.result +++ b/test/sql/errinj.result @@ -280,22 +280,3 @@ errinj.set("ERRINJ_WAL_IO", false) box.sql.execute("DROP TABLE t3;") --- ... --- --- gh-3832: Some statements do not return column type --- This test placed here because it should be skipped in release --- build. --- Check that "PRAGMA parser_trace" returns 0 or 1 if called --- without parameter. -result = box.sql.execute('PRAGMA parser_trace') ---- -... --- Should be TRUE. -result[1][1] == 0 or result[1][1] == 1 ---- -- true -... --- Check that "PRAGMA parser_trace" returns nothing if called --- with parameter. -box.sql.execute('PRAGMA parser_trace = '.. result[1][1]) ---- -... diff --git a/test/sql/errinj.test.lua b/test/sql/errinj.test.lua index a938eda..fa7f9f2 100644 --- a/test/sql/errinj.test.lua +++ b/test/sql/errinj.test.lua @@ -97,19 +97,3 @@ box.sql.execute("ALTER TABLE t3 DROP CONSTRAINT fk1;") box.sql.execute("INSERT INTO t3 VALUES(1, 1, 3);") errinj.set("ERRINJ_WAL_IO", false) box.sql.execute("DROP TABLE t3;") - --- --- gh-3832: Some statements do not return column type - --- This test placed here because it should be skipped in release --- build. - --- Check that "PRAGMA parser_trace" returns 0 or 1 if called --- without parameter. -result = box.sql.execute('PRAGMA parser_trace') --- Should be TRUE. -result[1][1] == 0 or result[1][1] == 1 - --- Check that "PRAGMA parser_trace" returns nothing if called --- with parameter. -box.sql.execute('PRAGMA parser_trace = '.. result[1][1]) diff --git a/test/sql/sql-debug.result b/test/sql/sql-debug.result new file mode 100644 index 0000000..692fa82 --- /dev/null +++ b/test/sql/sql-debug.result @@ -0,0 +1,32 @@ +remote = require('net.box') +--- +... +test_run = require('test_run').new() +--- +... +engine = test_run:get_cfg('engine') +--- +... +box.sql.execute('pragma sql_default_engine=\''..engine..'\'') +--- +... +-- +-- gh-3832: Some statements do not return column type +-- Check that "PRAGMA parser_trace" returns 0 or 1 if called +-- without parameter. +result = box.sql.execute('PRAGMA parser_trace') +--- +... +-- Should be nothing. +box.sql.execute('PRAGMA parser_trace = 1') +--- +... +-- Should be 1. +box.sql.execute('PRAGMA parser_trace') +--- +- - [1] +... +-- Should be nothing. +box.sql.execute('PRAGMA parser_trace = '.. result[1][1]) +--- +... diff --git a/test/sql/sql-debug.test.lua b/test/sql/sql-debug.test.lua new file mode 100644 index 0000000..66f47b3 --- /dev/null +++ b/test/sql/sql-debug.test.lua @@ -0,0 +1,17 @@ +remote = require('net.box') +test_run = require('test_run').new() +engine = test_run:get_cfg('engine') +box.sql.execute('pragma sql_default_engine=\''..engine..'\'') + +-- +-- gh-3832: Some statements do not return column type + +-- Check that "PRAGMA parser_trace" returns 0 or 1 if called +-- without parameter. +result = box.sql.execute('PRAGMA parser_trace') +-- Should be nothing. +box.sql.execute('PRAGMA parser_trace = 1') +-- Should be 1. +box.sql.execute('PRAGMA parser_trace') +-- Should be nothing. +box.sql.execute('PRAGMA parser_trace = '.. result[1][1]) diff --git a/test/sql/suite.ini b/test/sql/suite.ini index 4504731..ce6ccb7 100644 --- a/test/sql/suite.ini +++ b/test/sql/suite.ini @@ -6,4 +6,4 @@ use_unix_sockets = True config = engine.cfg is_parallel = True lua_libs = lua/sql_tokenizer.lua -release_disabled = errinj.test.lua view_delayed_wal.test.lua +release_disabled = errinj.test.lua view_delayed_wal.test.lua sql-debug.test.lua New version: commit efa1434cc4186a026f416f7506319f89009fd029 Author: Mergen Imeev Date: Wed Dec 12 22:16:33 2018 +0300 sql: fix "PRAGMA parser_trace" result Currently PRAGMA parser_trace returns an empty table. This seems wrong, since other similar pragmas return their status. Fixed in the current patch. diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c index 5729fe6..6122986 100644 --- a/src/box/sql/pragma.c +++ b/src/box/sql/pragma.c @@ -466,33 +466,31 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ switch (pPragma->ePragTyp) { case PragTyp_FLAG:{ - if (zRight == 0) { - setPragmaResultColumnNames(v, pPragma); - returnSingleInt(v, - (user_session-> - sql_flags & pPragma->iArg) != - 0); - } else { - int mask = pPragma->iArg; /* Mask of bits to set - * or clear. - */ - - if (sqlite3GetBoolean(zRight, 0)) { - user_session->sql_flags |= mask; - } else { - user_session->sql_flags &= ~mask; - } - - /* Many of the flag-pragmas modify the code - * generated by the SQL * compiler (eg. - * count_changes). So add an opcode to expire - * all * compiled SQL statements after - * modifying a pragma value. - */ - sqlite3VdbeAddOp0(v, OP_Expire); + if (zRight == 0) { + setPragmaResultColumnNames(v, pPragma); + returnSingleInt(v, (user_session->sql_flags & + pPragma->iArg) != 0); + } else { + int mask = pPragma->iArg; /* Mask of bits to set + * or clear. + */ + bool is_value_true = sqlite3GetBoolean(zRight, 0); + + if (is_value_true) + user_session->sql_flags |= mask; + else + user_session->sql_flags &= ~mask; +#ifndef NDEBUG + if (mask == PARSER_TRACE_FLAG) { + if (is_value_true) + sqlite3ParserTrace(stdout, "parser: "); + else + sqlite3ParserTrace(0, 0); } - break; +#endif } + break; + } case PragTyp_TABLE_INFO: sql_pragma_table_info(pParse, zRight); @@ -569,18 +567,6 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ } break; } -#ifndef NDEBUG - case PragTyp_PARSER_TRACE:{ - if (zRight) { - if (sqlite3GetBoolean(zRight, 0)) { - sqlite3ParserTrace(stdout, "parser: "); - } else { - sqlite3ParserTrace(0, 0); - } - } - break; - } -#endif /* * Reinstall the LIKE and functions. The variant diff --git a/src/box/sql/pragma.h b/src/box/sql/pragma.h index 84ab478..ae8e030 100644 --- a/src/box/sql/pragma.h +++ b/src/box/sql/pragma.h @@ -14,7 +14,6 @@ #define PragTyp_INDEX_LIST 11 #define PragTyp_STATS 15 #define PragTyp_TABLE_INFO 17 -#define PragTyp_PARSER_TRACE 24 #define PragTyp_DEFAULT_ENGINE 25 #define PragTyp_COMPOUND_SELECT_LIMIT 26 @@ -137,10 +136,10 @@ static const PragmaName aPragmaName[] = { /* iArg: */ 0}, #if defined(SQLITE_DEBUG) { /* zName: */ "parser_trace", - /* ePragTyp: */ PragTyp_PARSER_TRACE, - /* ePragFlg: */ 0, + /* ePragTyp: */ PragTyp_FLAG, + /* ePragFlg: */ PragFlg_Result0 | PragFlg_NoColumns1, /* ColNames: */ 0, 0, - /* iArg: */ 0}, + /* iArg: */ PARSER_TRACE_FLAG}, #endif { /* zName: */ "query_only", /* ePragTyp: */ PragTyp_FLAG, diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 4110a59..a484486 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -1563,6 +1563,8 @@ struct sqlite3 { * Possible values for the sqlite3.flags. */ #define SQLITE_VdbeTrace 0x00000001 /* True to trace VDBE execution */ +/* Debug print info about SQL query as it parsed */ +#define PARSER_TRACE_FLAG 0x00000002 #define SQLITE_FullColNames 0x00000004 /* Show full column names on SELECT */ #define SQLITE_ShortColNames 0x00000040 /* Show short columns names */ #define SQLITE_CountRows 0x00000080 /* Count rows changed by INSERT, */ diff --git a/test/sql/sql-debug.result b/test/sql/sql-debug.result new file mode 100644 index 0000000..692fa82 --- /dev/null +++ b/test/sql/sql-debug.result @@ -0,0 +1,32 @@ +remote = require('net.box') +--- +... +test_run = require('test_run').new() +--- +... +engine = test_run:get_cfg('engine') +--- +... +box.sql.execute('pragma sql_default_engine=\''..engine..'\'') +--- +... +-- +-- gh-3832: Some statements do not return column type +-- Check that "PRAGMA parser_trace" returns 0 or 1 if called +-- without parameter. +result = box.sql.execute('PRAGMA parser_trace') +--- +... +-- Should be nothing. +box.sql.execute('PRAGMA parser_trace = 1') +--- +... +-- Should be 1. +box.sql.execute('PRAGMA parser_trace') +--- +- - [1] +... +-- Should be nothing. +box.sql.execute('PRAGMA parser_trace = '.. result[1][1]) +--- +... diff --git a/test/sql/sql-debug.test.lua b/test/sql/sql-debug.test.lua new file mode 100644 index 0000000..66f47b3 --- /dev/null +++ b/test/sql/sql-debug.test.lua @@ -0,0 +1,17 @@ +remote = require('net.box') +test_run = require('test_run').new() +engine = test_run:get_cfg('engine') +box.sql.execute('pragma sql_default_engine=\''..engine..'\'') + +-- +-- gh-3832: Some statements do not return column type + +-- Check that "PRAGMA parser_trace" returns 0 or 1 if called +-- without parameter. +result = box.sql.execute('PRAGMA parser_trace') +-- Should be nothing. +box.sql.execute('PRAGMA parser_trace = 1') +-- Should be 1. +box.sql.execute('PRAGMA parser_trace') +-- Should be nothing. +box.sql.execute('PRAGMA parser_trace = '.. result[1][1]) diff --git a/test/sql/suite.ini b/test/sql/suite.ini index 4504731..ce6ccb7 100644 --- a/test/sql/suite.ini +++ b/test/sql/suite.ini @@ -6,4 +6,4 @@ use_unix_sockets = True config = engine.cfg is_parallel = True lua_libs = lua/sql_tokenizer.lua -release_disabled = errinj.test.lua view_delayed_wal.test.lua +release_disabled = errinj.test.lua view_delayed_wal.test.lua sql-debug.test.lua -- 2.7.4