Merge cockroachdb#131426 cockroachdb#131881

131426: tablemetadata: track more job metrics and update job progress while running r=kyle-a-wong a=kyle-a-wong

### tablemetadatacache: update progress of update tmj
updates the job progress value of the
update table metadata job as it processes
batches of tables. The progress will not be
updated on every successful batch update, but
will instead be updated every nth batch, where
n is defined by `batchesPerProgressUpdate`. This
is done because each batch executes relatively
quickly, and it is unnecessary to provide such
granular updates to the progress, each of which
results in a write to the database.

Part of: cockroachdb#130249
Epic: [CRDB-37558](https://cockroachlabs.atlassian.net/browse/CRDB-37558)
Release note: None

---
### tablemetadatacache: add metrics to tmj
Adds additional metrics to the update table metadata
job:
  * UpdatedTables - The total number of table rows
                    written to system.table_metadata
  * Errors        - The total number of errors emitted
                    from job runs
  * Duration      - The time spent executing the job

Part of: cockroachdb#130249
Epic: [CRDB-37558](https://cockroachlabs.atlassian.net/browse/CRDB-37558)
Release note: None

131881: cli: fix double-quote escaping for JSON values with format=sql r=rafiss a=rafiss

A previous commit (98f9c8a) made an attempt to fix how JSON values are escaped when they contain invalid UTF8 codes and are displayed in the CLI using the --format=sql flag (see cockroachdb#107518).

That commit ended up breaking how JSON values are escaped when they contain double quotes.

Luckily it turns out that both problems were actually caused by a long-lived mistake in the `clisqlexec.FormatVal` function. It shouldn't use `fmt.Sprintf("%+q", s)` to escape a string that has invalid characters, as that conflicts with how SQL strings are normally escaped. The proper way is to use `lexbase.EscapeSQLString(s)`.

fixes cockroachdb#131257
Release note (bug fix): Fixed a bug where the CLI would not correctly escape JSON values that had double-quotes inside of a string when using the --format=sql flag.

Co-authored-by: Kyle Wong <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>

docs/generated/metrics/metrics.html

@@ -1477,7 +1477,10 @@
 <tr><td>APPLICATION</td><td>logical_replication.replicated_time_seconds</td><td>The replicated time of the logical replication stream in seconds since the unix epoch.</td><td>Seconds</td><td>GAUGE</td><td>SECONDS</td><td>AVG</td><td>NONE</td></tr>
 <tr><td>APPLICATION</td><td>logical_replication.retry_queue_bytes</td><td>The replicated time of the logical replication stream in seconds since the unix epoch.</td><td>Bytes</td><td>GAUGE</td><td>BYTES</td><td>AVG</td><td>NONE</td></tr>
 <tr><td>APPLICATION</td><td>logical_replication.retry_queue_events</td><td>The replicated time of the logical replication stream in seconds since the unix epoch.</td><td>Events</td><td>GAUGE</td><td>COUNT</td><td>AVG</td><td>NONE</td></tr>
+<tr><td>APPLICATION</td><td>obs.tablemetadata.update_job.duration</td><td>Time spent running the update table metadata job.</td><td>Duration</td><td>HISTOGRAM</td><td>NANOSECONDS</td><td>AVG</td><td>NONE</td></tr>
+<tr><td>APPLICATION</td><td>obs.tablemetadata.update_job.errors</td><td>The total number of errors that have been emitted from the update table metadata job.</td><td>Errors</td><td>COUNTER</td><td>COUNT</td><td>AVG</td><td>NON_NEGATIVE_DERIVATIVE</td></tr>
 <tr><td>APPLICATION</td><td>obs.tablemetadata.update_job.runs</td><td>The total number of runs of the update table metadata job.</td><td>Executions</td><td>COUNTER</td><td>COUNT</td><td>AVG</td><td>NON_NEGATIVE_DERIVATIVE</td></tr>
+<tr><td>APPLICATION</td><td>obs.tablemetadata.update_job.table_updates</td><td>The total number of rows that have been updated in system.table_metadata</td><td>Rows Updated</td><td>COUNTER</td><td>COUNT</td><td>AVG</td><td>NON_NEGATIVE_DERIVATIVE</td></tr>
 <tr><td>APPLICATION</td><td>physical_replication.admit_latency</td><td>Event admission latency: a difference between event MVCC timestamp and the time it was admitted into ingestion processor</td><td>Nanoseconds</td><td>HISTOGRAM</td><td>NANOSECONDS</td><td>AVG</td><td>NONE</td></tr>
 <tr><td>APPLICATION</td><td>physical_replication.commit_latency</td><td>Event commit latency: a difference between event MVCC timestamp and the time it was flushed into disk. If we batch events, then the difference between the oldest event in the batch and flush is recorded</td><td>Nanoseconds</td><td>HISTOGRAM</td><td>NANOSECONDS</td><td>AVG</td><td>NONE</td></tr>
 <tr><td>APPLICATION</td><td>physical_replication.cutover_progress</td><td>The number of ranges left to revert in order to complete an inflight cutover</td><td>Ranges</td><td>GAUGE</td><td>COUNT</td><td>AVG</td><td>NONE</td></tr>

pkg/cli/clisqlexec/BUILD.bazel

@@ -39,6 +39,7 @@ go_test(
     name = "clisqlexec_test",
     srcs = [
         "format_html_test.go",
+        "format_sql_test.go",
         "format_table_test.go",
         "format_value_test.go",
         "main_test.go",

pkg/cli/clisqlexec/format_sql.go

@@ -43,7 +43,7 @@ func (p *sqlReporter) iter(w, _ io.Writer, _ int, row []string) error {
 	fmt.Fprint(w, "INSERT INTO results VALUES (")
 	for i, r := range row {
 		var buf bytes.Buffer
-		lexbase.EncodeSQLStringWithFlags(&buf, r, lexbase.EncNoDoubleEscapeQuotes)
+		lexbase.EncodeSQLStringWithFlags(&buf, r, lexbase.EncNoFlags)
 		fmt.Fprint(w, buf.String())
 		if i < len(row)-1 {
 			fmt.Fprint(w, ", ")

pkg/cli/clisqlexec/format_sql_test.go

@@ -0,0 +1,71 @@
+// Copyright 2024 The Cockroach Authors.
+//
+// Use of this software is governed by the CockroachDB Software License
+// included in the /LICENSE file.
+
+package clisqlexec_test
+
+import "github.com/cockroachdb/cockroach/pkg/cli"
+
+func Example_json_sql_format() {
+	c := cli.NewCLITest(cli.TestCLIParams{})
+	defer c.Cleanup()
+
+	testData := []string{
+		`e'{"a": "bc"}'`,
+		`e'{"a": "b\u0099c"}'`,
+		`e'{"a": "b\\"c"}'`,
+		`'"there are \"quotes\" in this json string"'`,
+		`'""'`,
+		`'{}'`,
+	}
+
+	for _, s := range testData {
+		query := `SELECT ` + s + `::json`
+		c.RunWithArgs([]string{"sql", "--format=sql", "-e", query})
+	}
+
+	// Output:
+	// sql --format=sql -e SELECT e'{"a": "bc"}'::json
+	// CREATE TABLE results (
+	//   jsonb STRING
+	// );
+	//
+	// INSERT INTO results VALUES ('{"a": "bc"}');
+	// -- 1 row
+	// sql --format=sql -e SELECT e'{"a": "b\u0099c"}'::json
+	// CREATE TABLE results (
+	//   jsonb STRING
+	// );
+	//
+	// INSERT INTO results VALUES (e'{"a": "b\\u0099c"}');
+	// -- 1 row
+	// sql --format=sql -e SELECT e'{"a": "b\\"c"}'::json
+	// CREATE TABLE results (
+	//   jsonb STRING
+	// );
+	//
+	// INSERT INTO results VALUES (e'{"a": "b\\"c"}');
+	// -- 1 row
+	// sql --format=sql -e SELECT '"there are \"quotes\" in this json string"'::json
+	// CREATE TABLE results (
+	//   jsonb STRING
+	// );
+	//
+	// INSERT INTO results VALUES (e'"there are \\"quotes\\" in this json string"');
+	// -- 1 row
+	// sql --format=sql -e SELECT '""'::json
+	// CREATE TABLE results (
+	//   jsonb STRING
+	// );
+	//
+	// INSERT INTO results VALUES ('""');
+	// -- 1 row
+	// sql --format=sql -e SELECT '{}'::json
+	// CREATE TABLE results (
+	//   jsonb STRING
+	// );
+	//
+	// INSERT INTO results VALUES ('{}');
+	// -- 1 row
+}

pkg/cli/clisqlexec/format_value.go

@@ -11,6 +11,8 @@ import (
 	"strings"
 	"unicode"
 	"unicode/utf8"
+
+	"github.com/cockroachdb/cockroach/pkg/sql/lexbase"
 )
 
 func isNotPrintableASCII(r rune) bool { return r < 0x20 || r > 0x7e || r == '"' || r == '\\' }
@@ -40,10 +42,11 @@ func FormatVal(val driver.Value, showPrintableUnicode bool, showNewLinesAndTabs
 				return t
 			}
 		}
-		s := fmt.Sprintf("%+q", t)
+		s := lexbase.EscapeSQLString(t)
+		// The result from EscapeSQLString is an escape-quoted string, like e'...'.
 		// Strip the start and final quotes. The surrounding display
 		// format (e.g. CSV/TSV) will add its own quotes.
-		return s[1 : len(s)-1]
+		return s[2 : len(s)-1]
 	}
 
 	// Fallback to printing the value as-is.

pkg/server/api_v2_databases_metadata_test.go

@@ -748,7 +748,6 @@ func TestTriggerMetadataUpdateJob(t *testing.T) {
 	defer close(jobReadyChan)
 	testCluster := serverutils.StartCluster(t, 3, base.TestClusterArgs{
 		ServerArgs: base.TestServerArgs{
-
 			Knobs: base.TestingKnobs{
 				TableMetadata: &tablemetadatacache_util.TestingKnobs{
 					OnJobReady: func() {

pkg/server/server_sql.go

@@ -1146,11 +1146,10 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*SQLServer, error) {
 	if tableStatsKnobs := cfg.TestingKnobs.TableStatsKnobs; tableStatsKnobs != nil {
 		tableStatsTestingKnobs = tableStatsKnobs.(*stats.TableStatsTestingKnobs)
 	}
-
 	if tableMetadataKnobs := cfg.TestingKnobs.TableMetadata; tableMetadataKnobs != nil {
 		execCfg.TableMetadataKnobs = tableMetadataKnobs.(*tablemetadatacacheutil.TestingKnobs)
-
 	}
+
 	// Set up internal memory metrics for use by internal SQL executors.
 	// Don't add them to the registry now because it will be added as part of pgServer metrics.
 	sqlMemMetrics := sql.MakeMemMetrics("sql", cfg.HistogramWindowInterval())

pkg/sql/lexbase/encode.go

@@ -42,10 +42,6 @@ const (
 	// without wrapping quotes.
 	EncBareIdentifiers
 
-	// EncNoDoubleEscapeQuotes indicates that backslashes will not be
-	// escaped when they are used as escape quotes.
-	EncNoDoubleEscapeQuotes
-
 	// EncFirstFreeFlagBit needs to remain unused; it is used as base
 	// bit offset for tree.FmtFlags.
 	EncFirstFreeFlagBit
@@ -144,7 +140,6 @@ func EncodeSQLStringWithFlags(buf *bytes.Buffer, in string, flags EncodeFlags) {
 	start := 0
 	escapedString := false
 	bareStrings := flags.HasFlags(EncBareStrings)
-	noDoubleEscapeQuotes := flags.HasFlags(EncNoDoubleEscapeQuotes)
 	// Loop through each unicode code point.
 	for i, r := range in {
 		if i < start {
@@ -165,9 +160,6 @@ func EncodeSQLStringWithFlags(buf *bytes.Buffer, in string, flags EncodeFlags) {
 			buf.WriteString("e'") // begin e'xxx' string
 			escapedString = true
 		}
-		if noDoubleEscapeQuotes && i+1 < len(in) && in[i:i+2] == "\\\"" {
-			continue
-		}
 		buf.WriteString(in[start:i])
 
 		ln := utf8.RuneLen(r)

pkg/sql/lexbase/encode_test.go

@@ -112,28 +112,6 @@ func testEncodeString(t *testing.T, input []byte, encode func(*bytes.Buffer, str
 	return stmt
 }
 
-func TestEncodeSQLStringWithNoDoubleEscapeQuotes(t *testing.T) {
-	testCases := []struct {
-		input  string
-		output string
-	}{
-		// (GH issue #107518)
-		{`\"`, `e'\"'`},
-		{`{"a": "b\u0099c"}`, `e'{"a": "b\\u0099c"}'`},
-		{`{\"a\": \"b\u0099c\"}`, `e'{\"a\": \"b\\u0099c\"}'`},
-	}
-
-	for _, tc := range testCases {
-		var buf bytes.Buffer
-		lexbase.EncodeSQLStringWithFlags(&buf, tc.input, lexbase.EncNoDoubleEscapeQuotes)
-		out := buf.String()
-
-		if out != tc.output {
-			t.Errorf("`%s`: expected `%s`, got `%s`", tc.input, tc.output, out)
-		}
-	}
-}
-
 func BenchmarkEncodeSQLString(b *testing.B) {
 	str := strings.Repeat("foo", 10000)
 	for i := 0; i < b.N; i++ {

pkg/sql/tablemetadatacache/BUILD.bazel

@@ -11,6 +11,7 @@ go_library(
     importpath = "github.com/cockroachdb/cockroach/pkg/sql/tablemetadatacache",
     visibility = ["//visibility:public"],
     deps = [
+        "//pkg/base",
         "//pkg/jobs",
         "//pkg/jobs/jobspb",
         "//pkg/roachpb",
@@ -20,6 +21,7 @@ go_library(
         "//pkg/sql/isql",
         "//pkg/sql/sem/tree",
         "//pkg/sql/sessiondata",
+        "//pkg/sql/tablemetadatacache/util",
         "//pkg/util/log",
         "//pkg/util/metric",
         "//pkg/util/timeutil",
@@ -48,9 +50,12 @@ go_test(
         "//pkg/kv/kvserver",
         "//pkg/security/securityassets",
         "//pkg/security/securitytest",
+        "//pkg/security/username",
         "//pkg/server",
         "//pkg/server/serverpb",
+        "//pkg/sql",
         "//pkg/sql/isql",
+        "//pkg/sql/tablemetadatacache/util",
         "//pkg/testutils",
         "//pkg/testutils/datapathutils",
         "//pkg/testutils/serverutils",
@@ -59,8 +64,6 @@ go_test(
         "//pkg/testutils/testcluster",
         "//pkg/util/leaktest",
         "//pkg/util/log",
-        "//pkg/util/syncutil",
-        "//pkg/util/timeutil",
         "@com_github_cockroachdb_datadriven//:datadriven",
         "@com_github_stretchr_testify//require",
     ],