Tarantool development patches archive
 help / color / mirror / Atom feed
From: imeevma@tarantool.org
To: tarantool-patches@freelists.org, korablev@tarantool.org
Subject: [tarantool-patches] [PATCH v4 5/6] sql: 'PRAGMA' result in YAML format
Date: Sat, 19 Jan 2019 15:37:42 +0300	[thread overview]
Message-ID: <a3e6e457680e9aef5af78ca7a92f1572fcfbdef4.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:
>
>>> There is no ’Tarantool format’...
>> Changed to "more appropriate”.
>
> Cmon, it is called YAML...
>
Thanks, fixed.

>>
>>> 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”.
>
> Now it is even worse.
> I vote for displaying only flag pragmas and make
> second field of resulting filed to be of type bool. 
> You can ask other members for their opinion.
>
I did as you suggested since there were no suggestions from dev
chat.

>>> 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.
>
> It can be used by users until we remove it. That’s it.
>
>> commit 1b0d86990ecf8c9b310cdfdc58ec9006c77c038a
>> Author: Mergen Imeev <imeevma@gmail.com>
>> 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.
>
> Nit: be specific: not ’some information’ but list of pragmas
> and their statuses.
>
Fixed.

> Nit: print to stdout.
>
Fixed.

>> This looks wrong.
>
> "Such strategy is considered to be wrong since output
> of this command would be unavailable for users who
> redirect stdout, use net box connection etc."
>
Thanks, fixed.

>> This patch makes the
>>    command to return result in more appropriate format.
>
> "In yaml format as the rest of SQL commands.”
>
Fixed.

> Sorry for being too meticulous.
>
It would be more appropriate for me to say thank you for this, I
think.

>>
>> 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
>
> Why did you move test to this file?
> PRAGMA itself is not debug-only command.
>
>
I did this because the result of the command in the debug build is
different from the result in the non-debug build.


Diff between versions:

commit 6a94e1371e4691d19a96ed8ac1aa76915239c86f
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Sat Jan 19 12:08:10 2019 +0300

    Temporary: Review fix

diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index 7851a57..de81c28 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -179,50 +179,16 @@ vdbe_emit_pragma_status(struct Parse *parse)
 	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);
+	sqlite3VdbeSetColName(v, 1, COLNAME_DECLTYPE, "INTEGER", SQLITE_STATIC);
 
 	parse->nMem = 2;
 	for (int i = 0; i < ArraySize(aPragmaName); ++i) {
+		if (aPragmaName[i].ePragTyp != PragTyp_FLAG)
+			continue;
 		sqlite3VdbeAddOp4(v, OP_String8, 0, 1, 0, aPragmaName[i].zName,
 				  0);
-		switch (aPragmaName[i].ePragTyp) {
-			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 *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);
-			}
-		}
+		int val = (user_session->sql_flags & aPragmaName[i].iArg) != 0;
+		sqlite3VdbeAddOp2(v, OP_Integer, val, 2);
 		sqlite3VdbeAddOp2(v, OP_ResultRow, 1, 2);
 	}
 }
diff --git a/test/sql/sql-debug.result b/test/sql/sql-debug.result
index fdf4241..6f1d889 100644
--- a/test/sql/sql-debug.result
+++ b/test/sql/sql-debug.result
@@ -28,42 +28,26 @@ box.sql.execute('PRAGMA parser_trace = '.. result[1][1])
 ---
 ...
 --
--- Make PRAGMA command return the result in a more appropriate
--- format.
+-- Make PRAGMA command return the result in YAML 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']
+box.sql.execute('PRAGMA')
+---
+- - ['case_sensitive_like', 0]
+  - ['count_changes', 0]
+  - ['defer_foreign_keys', 0]
+  - ['full_column_names', 0]
+  - ['parser_trace', 0]
+  - ['query_only', 0]
+  - ['read_uncommitted', 0]
+  - ['recursive_triggers', 1]
+  - ['reverse_unordered_selects', 0]
+  - ['select_trace', 0]
+  - ['short_column_names', 1]
+  - ['sql_trace', 0]
+  - ['vdbe_addoptrace', 0]
+  - ['vdbe_debug', 0]
+  - ['vdbe_eqp', 0]
+  - ['vdbe_listing', 0]
+  - ['vdbe_trace', 0]
+  - ['where_trace', 0]
 ...
diff --git a/test/sql/sql-debug.test.lua b/test/sql/sql-debug.test.lua
index 6173b3a..0cbc747 100644
--- a/test/sql/sql-debug.test.lua
+++ b/test/sql/sql-debug.test.lua
@@ -14,9 +14,6 @@ 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.
+-- Make PRAGMA command return the result in YAML format.
 --
-result = box.sql.execute('PRAGMA')
-for _,v in pairs(result) do v[2] = nil end
-result
+box.sql.execute('PRAGMA')


New version:

commit a3e6e457680e9aef5af78ca7a92f1572fcfbdef4
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Thu Dec 13 21:07:31 2018 +0300

    sql: 'PRAGMA' result in YAML format
    
    Currently box.sql.execute ('PRAGMA') returns nothing, but prints
    list of pragmas and their statuses to stdout. Such strategy is
    considered to be wrong since output of this command would be
    unavailable for users who redirect stdout, use net box connection
    etc. This patch makes the command to return result as the rest of
    SQL commands. The result contains only FLAG-type pragmas and their
    statuses in YAML format.

diff --git a/src/box/sql/pragma.c b/src/box/sql/pragma.c
index a610345..de81c28 100644
--- a/src/box/sql/pragma.c
+++ b/src/box/sql/pragma.c
@@ -168,48 +168,28 @@ 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) {
-		switch (aPragmaName[i].ePragTyp) {
-			case PragTyp_FLAG:
-				PRINT_PRAGMA(aPragmaName[i].zName, aPragmaName[i].iArg);
-				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);
-				break;
-			}
-		}
-	}
+	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, "INTEGER", SQLITE_STATIC);
 
-	printf("Other available pragmas: \n");
-	for (i = 0; i < ArraySize(aPragmaName); ++i) {
+	parse->nMem = 2;
+	for (int i = 0; i < ArraySize(aPragmaName); ++i) {
 		if (aPragmaName[i].ePragTyp != PragTyp_FLAG)
-			printf("-- %s \n", aPragmaName[i].zName);
+			continue;
+		sqlite3VdbeAddOp4(v, OP_String8, 0, 1, 0, aPragmaName[i].zName,
+				  0);
+		int val = (user_session->sql_flags & aPragmaName[i].iArg) != 0;
+		sqlite3VdbeAddOp2(v, OP_Integer, val, 2);
+		sqlite3VdbeAddOp2(v, OP_ResultRow, 1, 2);
 	}
 }
 
@@ -438,7 +418,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 0c9ac97..6f1d889 100644
--- a/test/sql/sql-debug.result
+++ b/test/sql/sql-debug.result
@@ -27,3 +27,27 @@ box.sql.execute('PRAGMA parser_trace')
 box.sql.execute('PRAGMA parser_trace = '.. result[1][1])
 ---
 ...
+--
+-- Make PRAGMA command return the result in YAML format.
+--
+box.sql.execute('PRAGMA')
+---
+- - ['case_sensitive_like', 0]
+  - ['count_changes', 0]
+  - ['defer_foreign_keys', 0]
+  - ['full_column_names', 0]
+  - ['parser_trace', 0]
+  - ['query_only', 0]
+  - ['read_uncommitted', 0]
+  - ['recursive_triggers', 1]
+  - ['reverse_unordered_selects', 0]
+  - ['select_trace', 0]
+  - ['short_column_names', 1]
+  - ['sql_trace', 0]
+  - ['vdbe_addoptrace', 0]
+  - ['vdbe_debug', 0]
+  - ['vdbe_eqp', 0]
+  - ['vdbe_listing', 0]
+  - ['vdbe_trace', 0]
+  - ['where_trace', 0]
+...
diff --git a/test/sql/sql-debug.test.lua b/test/sql/sql-debug.test.lua
index f946306..0cbc747 100644
--- a/test/sql/sql-debug.test.lua
+++ b/test/sql/sql-debug.test.lua
@@ -12,3 +12,8 @@ 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])
+
+--
+-- Make PRAGMA command return the result in YAML format.
+--
+box.sql.execute('PRAGMA')
-- 
2.7.4

  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 ` [tarantool-patches] [PATCH v4 2/6] sql: fix "PRAGMA parser_trace" result imeevma
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 ` imeevma [this message]
2019-01-20  0:19   ` [tarantool-patches] Re: [PATCH v4 5/6] sql: 'PRAGMA' result in YAML format 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=a3e6e457680e9aef5af78ca7a92f1572fcfbdef4.1547899933.git.imeevma@gmail.com \
    --to=imeevma@tarantool.org \
    --cc=korablev@tarantool.org \
    --cc=tarantool-patches@freelists.org \
    --subject='Re: [tarantool-patches] [PATCH v4 5/6] sql: '\''PRAGMA'\'' result in YAML format' \
    /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