From: imeevma@tarantool.org
To: tarantool-patches@freelists.org, korablev@tarantool.org
Subject: [tarantool-patches] [PATCH v3 2/6] sql: fix "PRAGMA parser_trace" result
Date: Wed, 26 Dec 2018 21:18:12 +0300 [thread overview]
Message-ID: <efa1434cc4186a026f416f7506319f89009fd029.1545844480.git.imeevma@gmail.com> (raw)
In-Reply-To: <cover.1545844480.git.imeevma@gmail.com>
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 <imeevma@gmail.com>
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 <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..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
next prev parent reply other threads:[~2018-12-26 18:18 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-26 18:17 [tarantool-patches] [PATCH v3 0/6] sql: set column types for EXPLAIN and PRAGMA imeevma
2018-12-26 18:17 ` [tarantool-patches] [PATCH v3 1/6] sql: remove unused macros from pragma.c and pragma.h imeevma
2019-01-16 15:34 ` [tarantool-patches] " n.pettik
2018-12-26 18:18 ` imeevma [this message]
2019-01-16 15:35 ` [tarantool-patches] Re: [PATCH v3 2/6] sql: fix "PRAGMA parser_trace" result n.pettik
2018-12-26 18:18 ` [tarantool-patches] [PATCH v3 3/6] sql: Show currently set sql_default_engine imeevma
2018-12-26 18:18 ` [tarantool-patches] [PATCH v3 4/6] sql: fix "PRAGMA case_sensitive_like" result imeevma
2019-01-16 15:35 ` [tarantool-patches] " n.pettik
2018-12-26 18:18 ` [tarantool-patches] [PATCH v3 5/6] sql: 'PRAGMA' result in the appropriate format imeevma
2019-01-16 15:35 ` [tarantool-patches] " n.pettik
2018-12-26 18:18 ` [tarantool-patches] [PATCH v3 6/6] sql: set column types for EXPLAIN and PRAGMA imeevma
2019-01-16 15:35 ` [tarantool-patches] " n.pettik
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=efa1434cc4186a026f416f7506319f89009fd029.1545844480.git.imeevma@gmail.com \
--to=imeevma@tarantool.org \
--cc=korablev@tarantool.org \
--cc=tarantool-patches@freelists.org \
--subject='Re: [tarantool-patches] [PATCH v3 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