Tarantool development patches archive
 help / color / mirror / Atom feed
From: Imeev Mergen <imeevma@tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>,
	tarantool-patches@freelists.org
Cc: korablev@tarantool.org
Subject: [tarantool-patches] Re: [PATCH v5 2/6] sql: fix "PRAGMA parser_trace" result
Date: Thu, 31 Jan 2019 17:56:43 +0300	[thread overview]
Message-ID: <e592b0f4-420d-0d23-316c-e491fee2d1fe@tarantool.org> (raw)
In-Reply-To: <3e0c44e3-21fc-8288-c877-da8fe5996758@tarantool.org>

Hi Thank you for review! Answer, fixes and patch below.


On 1/30/19 4:57 PM, Vladislav Shpilevoy wrote:
>
>
> On 29/01/2019 17:29, imeevma@tarantool.org wrote:
>> Currently PRAGMA parser_trace returns an empty table. This seems
>> wrong, since other similar pragmas return their status. Fixed in
>> the current patch.
>> ---
>>   src/box/sql/pragma.c        | 59 
>> +++++++++++++++++----------------------------
>>   src/box/sql/pragma.h        |  7 +++---
>>   src/box/sql/sqliteInt.h     |  2 ++
>>   test/sql/sql-debug.result   | 29 ++++++++++++++++++++++
>>   test/sql/sql-debug.test.lua | 14 +++++++++++
>>   test/sql/suite.ini          |  2 +-
>>   6 files changed, 71 insertions(+), 42 deletions(-)
>>   create mode 100644 test/sql/sql-debug.result
>>   create mode 100644 test/sql/sql-debug.test.lua
>> > 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')
>
> Why do you need an engine here? This test
> never touches the data dictionary.

Fixed.


Diff:

commit 69976f705d2509c48c6a467f51b1745cda2a50e7
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Thu Jan 31 16:30:18 2019 +0300

     Temporary: Review fix

diff --git a/test/sql/sql-debug.result b/test/sql/sql-debug.result
index 0c9ac97..9388578 100644
--- a/test/sql/sql-debug.result
+++ b/test/sql/sql-debug.result
@@ -4,12 +4,6 @@ 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
diff --git a/test/sql/sql-debug.test.lua b/test/sql/sql-debug.test.lua
index f946306..721ef19 100644
--- a/test/sql/sql-debug.test.lua
+++ b/test/sql/sql-debug.test.lua
@@ -1,7 +1,5 @@
  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


Patch:

commit dbd8a2f9024a4e6bdc4a6e2573e18251e61a141c
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 82738b0..9a3ae8f 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..9388578
--- /dev/null
+++ b/test/sql/sql-debug.result
@@ -0,0 +1,23 @@
+remote = require('net.box')
+---
+...
+test_run = require('test_run').new()
+---
+...
+--
+-- 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..721ef19
--- /dev/null
+++ b/test/sql/sql-debug.test.lua
@@ -0,0 +1,12 @@
+remote = require('net.box')
+test_run = require('test_run').new()
+
+--
+-- 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

  reply	other threads:[~2019-01-31 14:56 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-29 14:29 [tarantool-patches] [PATCH v5 0/6] sql: set column types for EXPLAIN and PRAGMA imeevma
2019-01-29 14:29 ` [tarantool-patches] [PATCH v5 1/6] sql: remove unused macros from pragma.c and pragma.h imeevma
2019-01-30 13:57   ` [tarantool-patches] " Vladislav Shpilevoy
2019-01-31 14:56     ` Imeev Mergen
2019-01-29 14:29 ` [tarantool-patches] [PATCH v5 2/6] sql: fix "PRAGMA parser_trace" result imeevma
2019-01-30 13:57   ` [tarantool-patches] " Vladislav Shpilevoy
2019-01-31 14:56     ` Imeev Mergen [this message]
2019-02-04 13:06       ` Vladislav Shpilevoy
2019-02-09 10:08         ` Mergen Imeev
2019-01-29 14:29 ` [tarantool-patches] [PATCH v5 3/6] sql: Show currently set sql_default_engine imeevma
2019-01-30 13:57   ` [tarantool-patches] " Vladislav Shpilevoy
2019-01-31 14:56     ` Imeev Mergen
2019-01-29 14:29 ` [tarantool-patches] [PATCH v5 4/6] sql: fix "PRAGMA case_sensitive_like" result imeevma
2019-01-30 13:56   ` [tarantool-patches] " Vladislav Shpilevoy
2019-01-31 14:56     ` Imeev Mergen
2019-01-29 14:29 ` [tarantool-patches] [PATCH v5 5/6] sql: get results of PRAGMA statement in YAML format imeevma
2019-01-30 13:56   ` [tarantool-patches] " Vladislav Shpilevoy
2019-01-31 14:56     ` Imeev Mergen
2019-02-04 13:08       ` Vladislav Shpilevoy
2019-02-09 10:11         ` Mergen Imeev
2019-01-29 14:29 ` [tarantool-patches] [PATCH v5 6/6] sql: set column types for EXPLAIN and PRAGMA imeevma
2019-01-30 13:59 ` [tarantool-patches] Re: [PATCH v5 0/6] " Vladislav Shpilevoy
2019-01-31 14:56   ` Imeev Mergen
2019-02-15 20:44 ` Vladislav Shpilevoy

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=e592b0f4-420d-0d23-316c-e491fee2d1fe@tarantool.org \
    --to=imeevma@tarantool.org \
    --cc=korablev@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='[tarantool-patches] Re: [PATCH v5 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