From: imeevma@tarantool.org
To: tarantool-patches@freelists.org, korablev@tarantool.org
Subject: [tarantool-patches] [PATCH v4 2/6] sql: fix "PRAGMA parser_trace" result
Date: Sat, 19 Jan 2019 15:37:35 +0300 [thread overview]
Message-ID: <61bfea175fbd91b86e45874ae123c411754d165d.1547899933.git.imeevma@gmail.com> (raw)
In-Reply-To: <cover.1547899933.git.imeevma@gmail.com>
Hi! Thank you for review. My answers, diff between versions and
new version below.
On 1/16/19 6:35 PM, n.pettik wrote:
>
>> 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) {
>
> Since you have already aligned code, I propose to fix it
> according to our codestyle:
>
> zRight == NULL
>
Fixed.
>> + setPragmaResultColumnNames(v, pPragma);
>> + returnSingleInt(v, (user_session->sql_flags &
>> + pPragma->iArg) != 0);
>> + } else {
>> + int mask = pPragma->iArg; /* Mask of bits to set
>> + * or clear.
>> + */
>
> Smth wrong with comment.
>
Fixed.
>> + bool is_value_true = sqlite3GetBoolean(zRight, 0);
>
> Bad name imho. I would call it “is_set” or “is_pragma_set” or “pragma_status”.
> Explanation: variable itself is of type bool and “value” says nothing.
>
Fixed, new name is "is_pragma_set".
>> 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.
>
> These three comments say nothing.
> Results and mismatches can be seen from .result and
> .reject files.
>
>
Fixed.
Diff between versions:
commit 4508278fa4b9100e0e1794f185d27a221bf7f50c
Author: Mergen Imeev <imeevma@gmail.com>
Date: Thu Jan 17 11:48:52 2019 +0300
Temporary: Review fixes
diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index 6122986..476771d 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -466,23 +466,22 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */
switch (pPragma->ePragTyp) {
case PragTyp_FLAG:{
- if (zRight == 0) {
+ if (zRight == NULL) {
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);
+ /* Mask of bits to set or clear. */
+ int mask = pPragma->iArg;
+ bool is_pragma_set = sqlite3GetBoolean(zRight, 0);
- if (is_value_true)
+ if (is_pragma_set)
user_session->sql_flags |= mask;
else
user_session->sql_flags &= ~mask;
-#ifndef NDEBUG
+#if defined(SQLITE_DEBUG)
if (mask == PARSER_TRACE_FLAG) {
- if (is_value_true)
+ if (is_pragma_set)
sqlite3ParserTrace(stdout, "parser: ");
else
sqlite3ParserTrace(0, 0);
diff --git a/test/sql/sql-debug.result b/test/sql/sql-debug.result
index 692fa82..0c9ac97 100644
--- a/test/sql/sql-debug.result
+++ b/test/sql/sql-debug.result
@@ -17,16 +17,13 @@ box.sql.execute('pragma sql_default_engine=\''..engine..'\'')
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
index 66f47b3..f946306 100644
--- a/test/sql/sql-debug.test.lua
+++ b/test/sql/sql-debug.test.lua
@@ -9,9 +9,6 @@ box.sql.execute('pragma sql_default_engine=\''..engine..'\'')
-- 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])
New version:
commit 61bfea175fbd91b86e45874ae123c411754d165d
Author: Mergen Imeev <imeevma@gmail.com>
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..476771d 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -466,33 +466,30 @@ 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 == NULL) {
+ setPragmaResultColumnNames(v, pPragma);
+ returnSingleInt(v, (user_session->sql_flags &
+ pPragma->iArg) != 0);
+ } else {
+ /* Mask of bits to set or clear. */
+ int mask = pPragma->iArg;
+ bool is_pragma_set = sqlite3GetBoolean(zRight, 0);
+
+ if (is_pragma_set)
+ user_session->sql_flags |= mask;
+ else
+ user_session->sql_flags &= ~mask;
+#if defined(SQLITE_DEBUG)
+ if (mask == PARSER_TRACE_FLAG) {
+ if (is_pragma_set)
+ sqlite3ParserTrace(stdout, "parser: ");
+ else
+ sqlite3ParserTrace(0, 0);
}
- break;
+#endif
}
+ break;
+ }
case PragTyp_TABLE_INFO:
sql_pragma_table_info(pParse, zRight);
@@ -569,18 +566,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 fd76b49..4837923 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 970a422..740db9f 100644
--- a/src/box/sql/sqliteInt.h
+++ b/src/box/sql/sqliteInt.h
@@ -1547,6 +1547,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..0c9ac97
--- /dev/null
+++ b/test/sql/sql-debug.result
@@ -0,0 +1,29 @@
+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')
+---
+...
+box.sql.execute('PRAGMA parser_trace = 1')
+---
+...
+box.sql.execute('PRAGMA parser_trace')
+---
+- - [1]
+...
+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..f946306
--- /dev/null
+++ b/test/sql/sql-debug.test.lua
@@ -0,0 +1,14 @@
+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')
+box.sql.execute('PRAGMA parser_trace = 1')
+box.sql.execute('PRAGMA parser_trace')
+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
next prev parent reply other threads:[~2019-01-19 12:37 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-19 12:37 [tarantool-patches] [PATCH v4 0/6] sql: set column types for EXPLAIN and PRAGMA imeevma
2019-01-19 12:37 ` [tarantool-patches] [PATCH v4 1/6] sql: remove unused macros from pragma.c and pragma.h imeevma
2019-01-20 0:16 ` [tarantool-patches] " n.pettik
2019-01-24 14:52 ` Imeev Mergen
2019-01-19 12:37 ` imeevma [this message]
2019-01-19 12:37 ` [tarantool-patches] [PATCH v4 3/6] sql: Show currently set sql_default_engine imeevma
2019-01-19 12:37 ` [tarantool-patches] [PATCH v4 4/6] sql: fix "PRAGMA case_sensitive_like" result imeevma
2019-01-19 12:37 ` [tarantool-patches] [PATCH v4 5/6] sql: 'PRAGMA' result in YAML format imeevma
2019-01-20 0:19 ` [tarantool-patches] " n.pettik
2019-01-24 14:53 ` Imeev Mergen
2019-01-19 12:37 ` [tarantool-patches] [PATCH v4 6/6] sql: set column types for EXPLAIN and PRAGMA imeevma
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=61bfea175fbd91b86e45874ae123c411754d165d.1547899933.git.imeevma@gmail.com \
--to=imeevma@tarantool.org \
--cc=korablev@tarantool.org \
--cc=tarantool-patches@freelists.org \
--subject='Re: [tarantool-patches] [PATCH v4 2/6] sql: fix "PRAGMA parser_trace" result' \
/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