sql: separate the execution flags from the key in crdb_internal.node_…

…statement_statistics (#15456)

Prior to this patch, the flags and keys were printed out in the same
column of "node_statement_statistics" (the `key` column). This made it
(slightly) more difficult to report statistics: since reporting uses
the anonymized column, one would have needed to extract the flags from
the key, and prepend that to the anonymized column. Instead, this
patch separates the flags from a separate column, which can be
selected together with the anonymized column to produce a reporting
entry.

Prior to this patch, the flags (e.g. error, distsql) were embedded in
the collection key, causing the scrubbing (anonymization) function to
fail entirely for keys with non-empty flags. This patch ensures that
the scrubbing occurs on the key with the flags omitted.

The statistics logic tests are sensitive to flags and were thus
failing depending on whether distsql was on auto mode. This patch
fixes the distsql mode for the part of the test that checks key
generation.

pkg/sql/app_stats.go

@@ -263,12 +263,24 @@ func (s *sqlStats) startResetWorker(stopper *stop.Stopper) {
 	})
 }
 
+func splitStmtStatKey(key string) (stmt, flags string) {
+	// Return the prefix indicating an error or DistSQL usage separately.
+	i := 0
+	for ; i < len(key) && (key[i] == '!' || key[i] == '+'); i++ {
+	}
+	flags = key[:i]
+	stmt = key[i:]
+	return stmt, flags
+}
+
 func (p *planner) scrubStmtStatKey(key string) (string, bool) {
+	// Re-parse the statement to obtain its AST.
 	stmt, err := parser.ParseOne(key)
 	if err != nil {
 		return "", false
 	}
 
+	// Re-format to remove most names.
 	formatter := parser.FmtReformatTableNames(parser.FmtAnonymize,
 		func(t *parser.NormalizableTableName, buf *bytes.Buffer, f parser.FmtFlags) {
 			tn, err := t.Normalize()
@@ -284,6 +296,5 @@ func (p *planner) scrubStmtStatKey(key string) (string, bool) {
 			// Virtual table: we want to keep the name.
 			tn.Format(buf, parser.FmtParsable)
 		})
-
 	return parser.AsStringWithFlags(stmt, formatter), true
 }

pkg/sql/crdb_internal.go

@@ -337,6 +337,7 @@ var crdbInternalStmtStatsTable = virtualSchemaTable{
 CREATE TABLE crdb_internal.node_statement_statistics (
   node_id             INT NOT NULL,
   application_name    STRING NOT NULL,
+  flags               STRING NOT NULL,
   key                 STRING NOT NULL,
   anonymized          STRING,
   count               INT NOT NULL,
@@ -396,8 +397,10 @@ CREATE TABLE crdb_internal.node_statement_statistics (
 
 			// Now retrieve the per-stmt stats proper.
 			for _, stmtKey := range stmtKeys {
+				realKey, flags := splitStmtStatKey(stmtKey)
+
 				anonymized := parser.DNull
-				anonStr, ok := p.scrubStmtStatKey(stmtKey)
+				anonStr, ok := p.scrubStmtStatKey(realKey)
 				if ok {
 					anonymized = parser.NewDString(anonStr)
 				}
@@ -412,7 +415,8 @@ CREATE TABLE crdb_internal.node_statement_statistics (
 				err := addRow(
 					nodeID,
 					parser.NewDString(appName),
-					parser.NewDString(stmtKey),
+					parser.NewDString(flags),
+					parser.NewDString(realKey),
 					anonymized,
 					parser.NewDInt(parser.DInt(s.data.Count)),
 					parser.NewDInt(parser.DInt(s.data.FirstAttemptCount)),

pkg/sql/distsqlrun/expr.go

@@ -70,7 +70,7 @@ func processExpression(exprSpec Expression, h *parser.IndexedVarHelper) (parser.
 	// Convert to a fully typed expression.
 	typedExpr, err := parser.TypeCheck(expr, nil, parser.TypeAny)
 	if err != nil {
-		return nil, err
+		return nil, errors.Wrap(err, expr.String())
 	}
 
 	return typedExpr, nil

pkg/sql/testdata/logic_test/statement_statistics

@@ -30,7 +30,11 @@ SELECT _    true
 SELECT _, _ true
 
 statement ok
-CREATE TABLE test(x INT, y INT, z INT)
+CREATE TABLE test(x INT, y INT, z INT); INSERT INTO test(x, y, z) VALUES (0,0,0);
+
+# Disable DistSQL for most statements, so that they don't get the "+" flag.
+statement ok
+SET distsql = off
 
 statement ok
 SET application_name = 'valuetest'
@@ -40,6 +44,9 @@ SET application_name = 'valuetest'
 statement ok
 SELECT sin(1.23)
 
+# Check stats for query errors.
+statement error cannot take square root
+SELECT sqrt(-1.0)
 
 # Check that shortened queries can use virtual tables.
 
@@ -72,17 +79,33 @@ SELECT x FROM test WHERE y IN (4, 5, 6+x, 7, 8)
 statement ok
 SELECT ROW(1,2,3,4,5) FROM test WHERE FALSE
 
+# Make one query run in distsql mode to test the flag
+# and flag combinations
+
 statement ok
-SET application_name = ''
+set distsql = on
 
-query T
-SELECT key FROM crdb_internal.node_statement_statistics WHERE application_name = 'valuetest' ORDER BY key
+statement ok
+SELECT x FROM test WHERE y IN (4, 5, 6, 7, 8)
+
+statement error division by zero
+SELECT x FROM test WHERE y = 1/z
+
+statement ok
+SET application_name = ''; RESET distsql
+
+query TT colnames
+SELECT key,flags FROM crdb_internal.node_statement_statistics WHERE application_name = 'valuetest' ORDER BY key
 ----
+key                                                      flags
 INSERT INTO test VALUES (_, _, _)
 SELECT ROW(_, _, _, _, _) FROM test WHERE _
 SELECT key FROM crdb_internal.node_statement_statistics
 SELECT sin(_)
+SELECT sqrt(- _)                                         !
 SELECT x FROM (VALUES (_, _, _)) AS t (x)
+SELECT x FROM test WHERE y = (_ / z)                     !+
+SELECT x FROM test WHERE y IN (_, _)                     +
 SELECT x FROM test WHERE y IN (_, _)
 SELECT x FROM test WHERE y IN (_, _, _ + x, _, _)
 SELECT x FROM test WHERE y NOT IN (_, _)
@@ -97,7 +120,10 @@ INSERT INTO _ VALUES (_, _, _)
 SELECT ROW(_, _, _, _, _) FROM _ WHERE _
 SELECT _ FROM crdb_internal.node_statement_statistics
 SELECT sin(_)
+SELECT sqrt(- _)
 SELECT _ FROM (VALUES (_, _, _)) AS _ (_)
+SELECT _ FROM _ WHERE _ = (_ / _)
+SELECT _ FROM _ WHERE _ IN (_, _)
 SELECT _ FROM _ WHERE _ IN (_, _)
 SELECT _ FROM _ WHERE _ IN (_, _, _ + _, _, _)
 SELECT _ FROM _ WHERE _ NOT IN (_, _)