[tarantool-patches] [PATCH v3 5/6] sql: 'PRAGMA' result in the appropriate format

[imeevma@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 <imeevma@gmail.com>
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 <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. 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