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 F29C721614 for ; Wed, 26 Dec 2018 13:18:21 -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 uOPb4DeBP3Nh for ; Wed, 26 Dec 2018 13:18:21 -0500 (EST) 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 8679B210F0 for ; Wed, 26 Dec 2018 13:18:21 -0500 (EST) From: imeevma@tarantool.org Subject: [tarantool-patches] [PATCH v3 5/6] sql: 'PRAGMA' result in the appropriate format Date: Wed, 26 Dec 2018 21:18:19 +0300 Message-Id: <1b0d86990ecf8c9b310cdfdc58ec9006c77c038a.1545844480.git.imeevma@gmail.com> 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:02 PM, n.pettik wrote: > There is no ’Tarantool format’... Changed to "more appropriate". > Why do you consider it as wrong? AFAIR I made it exclusively to > extend debug facilities. > > Things like > - ['index_info', null] > - ['index_list', null] > > look quite strange IMO. What is more, it may be misleading: > index_list with null arg looks like there is no indexes at all. Changed rows with values == NULL. Now instean of NULL they shows "No value". > Btw I don’t understand all these efforts to make pragma > better. As I already said we are going to remove it anyway. I thought it would be correct, since it can be used by the user. > Name seems to be a mess, lets rename it to ’sql_pragma_status’ or > ‘vdbe_emit_pragma_status’. Changed name to ‘vdbe_emit_pragma_status’. > This test look ridiculous: number of elements in res > doesn’t mean anything; first element with ‘busy_timeout’ > value as well. I guess you are willing to avoid fixing > this test each time number/content of pragmes are changed. > And it this acceptable solution. On the other hand, > it barely tests anything. So I propose to display full > content of pragma. I changed the test and moved it to the previously created file sql-debug.test.lua. The new test shows a table with the first column of the command result. I have not included the values ​​here, because they may differ from run to run. Diff between version: commit 8ec19ea1a9fe10a5329ae047369ec8ebd3980258 Author: Mergen Imeev Date: Tue Dec 25 19:28:20 2018 +0300 Temporary: review fixes 3 diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c index 62ba43b..2ad5806 100644 --- a/src/box/sql/pragma.c +++ b/src/box/sql/pragma.c @@ -169,7 +169,7 @@ pragmaLocate(const char *zName) } static void -sql_pragma_active_pragmas(struct Parse *parse) +vdbe_emit_pragma_status(struct Parse *parse) { struct Vdbe *v = sqlite3GetVdbe(parse); struct session *user_session = current_session(); @@ -186,8 +186,6 @@ sql_pragma_active_pragmas(struct Parse *parse) sqlite3VdbeAddOp4(v, OP_String8, 0, 1, 0, aPragmaName[i].zName, 0); switch (aPragmaName[i].ePragTyp) { - case PragTyp_PARSER_TRACE: - case PragTyp_CASE_SENSITIVE_LIKE: case PragTyp_FLAG: { const char *value; if ((user_session->sql_flags & @@ -219,8 +217,11 @@ sql_pragma_active_pragmas(struct Parse *parse) sqlite3VdbeAddOp2(v, OP_Cast, 2, AFFINITY_TEXT); break; } - default: - sqlite3VdbeAddOp2(v, OP_Null, 0, 2); + default: { + const char *value = "No value"; + sqlite3VdbeAddOp4(v, OP_String8, 0, 2, 0, value, + 0); + } } sqlite3VdbeAddOp2(v, OP_ResultRow, 1, 2); } @@ -451,7 +452,7 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ zLeft = sqlite3NameFromToken(db, pId); if (!zLeft) { - sql_pragma_active_pragmas(pParse); + vdbe_emit_pragma_status(pParse); return; } diff --git a/test/sql/misc.result b/test/sql/misc.result index bcbdec1..66f5a7b 100644 --- a/test/sql/misc.result +++ b/test/sql/misc.result @@ -60,15 +60,3 @@ box.sql.execute('PRAGMA case_sensitive_like') box.sql.execute('PRAGMA case_sensitive_like = '.. result[1][1]) --- ... --- Make command "PRAGMA" return result in Tarantool format. -res = box.sql.execute('PRAGMA') ---- -... -#res > 0 ---- -- true -... -res[1][1] ---- -- busy_timeout -... diff --git a/test/sql/misc.test.lua b/test/sql/misc.test.lua index 87224c4..cc31a5d 100644 --- a/test/sql/misc.test.lua +++ b/test/sql/misc.test.lua @@ -24,8 +24,3 @@ box.sql.execute('PRAGMA case_sensitive_like = 1') box.sql.execute('PRAGMA case_sensitive_like') -- Should be nothing. box.sql.execute('PRAGMA case_sensitive_like = '.. result[1][1]) - --- Make command "PRAGMA" return result in Tarantool format. -res = box.sql.execute('PRAGMA') -#res > 0 -res[1][1] diff --git a/test/sql/sql-debug.result b/test/sql/sql-debug.result index 692fa82..9c5c9b3 100644 --- a/test/sql/sql-debug.result +++ b/test/sql/sql-debug.result @@ -30,3 +30,43 @@ box.sql.execute('PRAGMA parser_trace') box.sql.execute('PRAGMA parser_trace = '.. result[1][1]) --- ... +-- +-- Make PRAGMA command return the result in a more appropriate +-- format. +-- +result = box.sql.execute('PRAGMA') +--- +... +for _,v in pairs(result) do v[2] = nil end +--- +... +result +--- +- - ['busy_timeout'] + - ['case_sensitive_like'] + - ['collation_list'] + - ['count_changes'] + - ['defer_foreign_keys'] + - ['foreign_key_list'] + - ['full_column_names'] + - ['index_info'] + - ['index_list'] + - ['parser_trace'] + - ['query_only'] + - ['read_uncommitted'] + - ['recursive_triggers'] + - ['reverse_unordered_selects'] + - ['select_trace'] + - ['short_column_names'] + - ['sql_compound_select_limit'] + - ['sql_default_engine'] + - ['sql_trace'] + - ['stats'] + - ['table_info'] + - ['vdbe_addoptrace'] + - ['vdbe_debug'] + - ['vdbe_eqp'] + - ['vdbe_listing'] + - ['vdbe_trace'] + - ['where_trace'] +... diff --git a/test/sql/sql-debug.test.lua b/test/sql/sql-debug.test.lua index 66f47b3..e145d0c 100644 --- a/test/sql/sql-debug.test.lua +++ b/test/sql/sql-debug.test.lua @@ -15,3 +15,11 @@ box.sql.execute('PRAGMA parser_trace = 1') box.sql.execute('PRAGMA parser_trace') -- Should be nothing. box.sql.execute('PRAGMA parser_trace = '.. result[1][1]) + +-- +-- Make PRAGMA command return the result in a more appropriate +-- format. +-- +result = box.sql.execute('PRAGMA') +for _,v in pairs(result) do v[2] = nil end +result New version: commit 1b0d86990ecf8c9b310cdfdc58ec9006c77c038a Author: Mergen Imeev Date: Thu Dec 13 21:07:31 2018 +0300 sql: 'PRAGMA' result in the appropriate format Currently, box.sql.execute('PRAGMA') returns nothing but it prints some information on stdout. This looks wrong. This patch makes the command to return result in more appropriate format. diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c index 67defed..2ad5806 100644 --- a/src/box/sql/pragma.c +++ b/src/box/sql/pragma.c @@ -168,48 +168,62 @@ pragmaLocate(const char *zName) return lwr > upr ? 0 : &aPragmaName[mid]; } -#ifdef PRINT_PRAGMA -#undef PRINT_PRAGMA -#endif -#define PRINT_PRAGMA(pragma_name, pragma_flag) do { \ - int nCoolSpaces = 30 - strlen(pragma_name); \ - if (user_session->sql_flags & (pragma_flag)) { \ - printf("%s %*c -- [true] \n", pragma_name, nCoolSpaces, ' '); \ - } else { \ - printf("%s %*c -- [false] \n", pragma_name, nCoolSpaces, ' ');\ - } \ -} while (0) - -#define PRINT_STR_PRAGMA(pragma_name, str_value) do { \ - int nCoolSpaces = 30 - strlen(pragma_name); \ - printf("%s %*c -- '%s' \n", pragma_name, nCoolSpaces, ' ', str_value);\ -} while (0) - static void -printActivePragmas(struct session *user_session) +vdbe_emit_pragma_status(struct Parse *parse) { - int i; - for (i = 0; i < ArraySize(aPragmaName); ++i) { + struct Vdbe *v = sqlite3GetVdbe(parse); + struct session *user_session = current_session(); + + sqlite3VdbeSetNumCols(v, 2); + sqlite3VdbeSetColName(v, 0, COLNAME_NAME, "pragma_name", SQLITE_STATIC); + sqlite3VdbeSetColName(v, 0, COLNAME_DECLTYPE, "TEXT", SQLITE_STATIC); + sqlite3VdbeSetColName(v, 1, COLNAME_NAME, "pragma_value", + SQLITE_STATIC); + sqlite3VdbeSetColName(v, 1, COLNAME_DECLTYPE, "TEXT", SQLITE_STATIC); + + parse->nMem = 2; + for (int i = 0; i < ArraySize(aPragmaName); ++i) { + sqlite3VdbeAddOp4(v, OP_String8, 0, 1, 0, aPragmaName[i].zName, + 0); switch (aPragmaName[i].ePragTyp) { - case PragTyp_FLAG: - PRINT_PRAGMA(aPragmaName[i].zName, aPragmaName[i].iArg); + case PragTyp_FLAG: { + const char *value; + if ((user_session->sql_flags & + aPragmaName[i].iArg) != 0) + value = "true"; + else + value = "false"; + sqlite3VdbeAddOp4(v, OP_String8, 0, 2, 0, value, + 0); break; + } case PragTyp_DEFAULT_ENGINE: { - const char *engine_name = - sql_storage_engine_strs[ - current_session()-> - sql_default_engine]; - PRINT_STR_PRAGMA(aPragmaName[i].zName, - engine_name); + const char *value = sql_storage_engine_strs[ + user_session->sql_default_engine]; + sqlite3VdbeAddOp4(v, OP_String8, 0, 2, 0, value, + 0); + break; + } + case PragTyp_BUSY_TIMEOUT: { + int value = parse->db->busyTimeout; + sqlite3VdbeAddOp2(v, OP_Integer, value, 2); + sqlite3VdbeAddOp2(v, OP_Cast, 2, AFFINITY_TEXT); + break; + } + case PragTyp_COMPOUND_SELECT_LIMIT: { + int value = sqlite3_limit(parse->db, + SQL_LIMIT_COMPOUND_SELECT, -1); + sqlite3VdbeAddOp2(v, OP_Integer, value, 2); + sqlite3VdbeAddOp2(v, OP_Cast, 2, AFFINITY_TEXT); break; } + default: { + const char *value = "No value"; + sqlite3VdbeAddOp4(v, OP_String8, 0, 2, 0, value, + 0); + } } - } - - printf("Other available pragmas: \n"); - for (i = 0; i < ArraySize(aPragmaName); ++i) { - if (aPragmaName[i].ePragTyp != PragTyp_FLAG) - printf("-- %s \n", aPragmaName[i].zName); + sqlite3VdbeAddOp2(v, OP_ResultRow, 1, 2); } } @@ -438,7 +452,7 @@ sqlite3Pragma(Parse * pParse, Token * pId, /* First part of [schema.]id field */ zLeft = sqlite3NameFromToken(db, pId); if (!zLeft) { - printActivePragmas(user_session); + vdbe_emit_pragma_status(pParse); return; } diff --git a/test/sql/sql-debug.result b/test/sql/sql-debug.result index 692fa82..9c5c9b3 100644 --- a/test/sql/sql-debug.result +++ b/test/sql/sql-debug.result @@ -30,3 +30,43 @@ box.sql.execute('PRAGMA parser_trace') box.sql.execute('PRAGMA parser_trace = '.. result[1][1]) --- ... +-- +-- Make PRAGMA command return the result in a more appropriate +-- format. +-- +result = box.sql.execute('PRAGMA') +--- +... +for _,v in pairs(result) do v[2] = nil end +--- +... +result +--- +- - ['busy_timeout'] + - ['case_sensitive_like'] + - ['collation_list'] + - ['count_changes'] + - ['defer_foreign_keys'] + - ['foreign_key_list'] + - ['full_column_names'] + - ['index_info'] + - ['index_list'] + - ['parser_trace'] + - ['query_only'] + - ['read_uncommitted'] + - ['recursive_triggers'] + - ['reverse_unordered_selects'] + - ['select_trace'] + - ['short_column_names'] + - ['sql_compound_select_limit'] + - ['sql_default_engine'] + - ['sql_trace'] + - ['stats'] + - ['table_info'] + - ['vdbe_addoptrace'] + - ['vdbe_debug'] + - ['vdbe_eqp'] + - ['vdbe_listing'] + - ['vdbe_trace'] + - ['where_trace'] +... diff --git a/test/sql/sql-debug.test.lua b/test/sql/sql-debug.test.lua index 66f47b3..e145d0c 100644 --- a/test/sql/sql-debug.test.lua +++ b/test/sql/sql-debug.test.lua @@ -15,3 +15,11 @@ box.sql.execute('PRAGMA parser_trace = 1') box.sql.execute('PRAGMA parser_trace') -- Should be nothing. box.sql.execute('PRAGMA parser_trace = '.. result[1][1]) + +-- +-- Make PRAGMA command return the result in a more appropriate +-- format. +-- +result = box.sql.execute('PRAGMA') +for _,v in pairs(result) do v[2] = nil end +result -- 2.7.4