Offer user to send feedback from profile error dialog

sql/connection.cc

Index: sql/connection.cc
diff --git a/sql/connection.cc b/sql/connection.cc
index ab84266673330e57bb24e2496743bd9f73f3948d..a566843f8489211bd60b34ab54d3943e62248120 100644
--- a/sql/connection.cc
+++ b/sql/connection.cc
@@ -1922,6 +1922,40 @@ bool Connection::QuickIntegrityCheck() {
   return messages.size() == 1 && messages[0] == "ok";
 }
 
+sql::DatabaseDiagnosticMap Connection::GetDiagnosticMap() {
+  sql::DatabaseDiagnosticMap result;
+  const bool has_valid_header =
+      (ExecuteAndReturnErrorCode("PRAGMA auto_vacuum") == SQLITE_OK);
+  result["Has valid header"] = has_valid_header ? "Yes" : "No";
+  const bool select_sqlite_master_result =
+      (ExecuteAndReturnErrorCode("SELECT COUNT(*) FROM sqlite_master") ==
+       SQLITE_OK);
+  result["\"SELECT COUNT(*) FROM sqlite_master\" succeeds"] =
+      select_sqlite_master_result ? "Yes" : "No";

[Scott Hess - ex-Googler, 2016/07/10 05:16:52]

Gross.  You're running the SELECT to determine something.  Name the line after
what you're determining, not how you're determining it.

[afakhry, 2016/07/11 16:47:46]

That's because I didn't know what that statement does. How about "Master table
exists"?

[Scott Hess - ex-Googler, 2016/07/13 01:18:46]

Do you expect the master table to not exist?

Earlier I think I suggested a select like this as "The minimal SQLite statement
which actually needs to touch the database schema".  If that's what you think
this is doing, then maybe something like "valid schema" would make sense.

[afakhry, 2016/07/13 19:57:58]

Well, I'm not the sql expert here, so I don't know what to expect. I'm just
running this statement because you suggested so earlier. All I know from my
experiments is that when I manually corrupt the DB file, this statement will
fail with NOTADB error. If you tell me it means "Valid Schema", then it means
"Valid Schema". :)
I renamed the entry accordingly.

+
+  // If any of the above failed, that's an indication that the file is really
+  // corrupted. Doing the integrity check or getting the schema will crash on
+  // DCHECKs on debug builds.
+  std::string integrity_check_result("Failed");
+  std::string schema("Failed");
+  if (has_valid_header && select_sqlite_master_result) {

[michaeln, 2016/07/12 20:05:32]

instead of dropping errors on the floor with a NullErrorHandler(), would it make
sense to add any errors generated to the diag log?

[afakhry, 2016/07/13 19:57:58]

Done. I'm actually using the same code in Connection::ReportDiagnosticInfo()
which takes a sql error and a statement and uses those to generate the
diagnostic errors.

+    std::vector<std::string> integrity_check_output;
+    if (FullIntegrityCheck(&integrity_check_output)) {
+      integrity_check_result = "";
+      for (const auto& line : integrity_check_output)

[michaeln, 2016/07/09 03:00:38]

The output is uninteresting in the success case, it'll always contains one
value, "OK".  I think you want to generate the result string in all case.

[afakhry, 2016/07/11 16:47:45]

Done.

+        integrity_check_result += line + "\n";
+
+      // we can only get the schema if the integrity check passes.
+      schema = GetSchema();
+    }
+  }
+
+  result["Integrity check"] = integrity_check_result;
+  result["Schema"] = schema;

[michaeln, 2016/07/09 03:00:38]

nit: for readability, might be nice to set all four result[] entries here at the
end so reads can more easily see what values are returned in the map

[Scott Hess - ex-Googler, 2016/07/10 05:16:52]

Note that |schema| and the integrity check results can be multi-line values. 
I'm not sure your output flattener thing accounts for that.  I think that's
another argument for just generating a flat string rather than a map, wherever
this ends up.

[afakhry, 2016/07/11 16:47:45]

Done.

[afakhry, 2016/07/11 16:47:46]

For the integrity check, multilines are handled above. For the schema, if the
string returned has multilines, then it should be fine and they will show up in
the feedback description properly.

[michaeln, 2016/07/12 20:05:32]

i think i'd vote for a basic string too, i don't think we want callers depending
on particular keys being provided in the map, if thats true lookup based on key
value is not important --> we don't need a map

[Scott Hess - ex-Googler, 2016/07/13 01:18:46]

IMHO having callers depend on keys would be really bad, because it would
encourage using this as a magic parameter-passing bucket.

[Scott Hess - ex-Googler, 2016/07/13 01:18:46]

Multilines are handled above by injecting newlines, which is also how you
flatten the map.  There's no reason the integrity check and schema couldn't
contain things which look like "key: value\n", which could be confusing.  So
mostly I was wondering if you wanted to do something to callout the distinction,
like an indent or other prefix, so that you can tell when you're seeing a
multi-line value.

[Actually, I think it will end up like:

Schema: CREATE TABLE A ...;
CREATE TABLE B ...;
Nextkey: nextvalue

which is visually not great.  Better might be:
Schema:
 CREATE TABLE A...;
 CREATE TABLE B...;
Nextkey: nextvalue

Though I suppose one could also argue that formatting in diagnostic data is
hardly the first concern we should have.]

[afakhry, 2016/07/13 19:57:58]

Done. Using a string now.

[afakhry, 2016/07/13 19:57:58]

Now I'm using whatever CollectCorruptionInfo() or CollectErrorInfo() return.

[afakhry, 2016/07/13 19:57:58]

Done.

[afakhry, 2016/07/13 19:57:58]

Agreed, formatting is not really the primary concern, however, let me know what
diagnostic info you'd like to see in those feedback reports and how you'd like
them to be formatted, so that it's as easy as possible for you to figure out
what's going on out there in the wild.

I'm currently using the what CollectCorruptionInfo() and CollectErrorInfo()
return. If that's not enough, let's add more.

+
+  return result;
+}
+
 // TODO(shess): Allow specifying maximum results (default 100 lines).
 bool Connection::IntegrityCheckHelper(
     const char* pragma_sql,