Merge #86404 #86665 #86817 #86924 #87029

86404: sql, sqlstats: schedule  sql stats compaction job async r=xinhaoz a=xinhaoz

Closes #85582

Currently, during sql stats compaction job scheduling, the schedule is
retrieved by  querying from the `system.scheduled_jobs` table. The function
handling scheduling is called synchronously during node startup to ensure the
first schedule, and can thus block node startup if the query experiences
contention.

This commit changes the scheduling setup to be called asynchronously using a
channel notification to ensure the stats collector does not block node startup.
The callback for changing the cluster setting  `SQLStatsCleanupRecurrence` is
also modified to match this behaviour, now issuing a channel notification to
update the schedule to the new value.

Release justification: bug fix, low risk update to existing functionality

Release note: None

86665: upgrade/upgrades: downgrade precondition validation failure r=Xiang-Gu a=ajwerner

The assertion failure lead to sentry reports we did not want to see.

Backport will address #85952. 

Release justification: For backport.

Release note: None

86817: backupccl: add SHOW BACKUP option for validating descriptors r=fqazi a=fqazi

Fixes: #85268

Previously, the only way to know the validity of a backup
was to restore the entire backup. This was inadequate because
there was no way to know if a given backup would restore without
any descriptor validation errors. To address this, this patch
adds support for validating the descriptors inside a given
backup as a hidden option.

This validation like the existing debug doctor command will
not be perfect, and is subject to interpretation.

Release justification: low risk command for validating descriptors
inside a backup. Only for internal use and not a user facing
feature.

86924: sql: add secondary_region to SHOW REGIONS r=e-mbrown a=rafiss

fixes #86808

Release note (sql change): SHOW REGIONS now shows information about
secondary regions.

Release justification: low risk update to new functionality.

87029: ui: fix high contention time details label r=ericharmeling a=ericharmeling

Previously, the High Contention Time insight label was incorrectly High Wait Time. This commit fixes that typo.

Release justification: bug fix
Release note: None

Co-authored-by: Xin Hao Zhang <[email protected]>
Co-authored-by: Andrew Werner <[email protected]>
Co-authored-by: Faizan Qazi <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Eric Harmeling <[email protected]>

docs/generated/sql/bnf/show_backup.bnf

@@ -18,3 +18,6 @@ show_backup_stmt ::=
 	| 'SHOW' 'BACKUP' 'RANGES' string_or_placeholder 'WITH' kv_option_list
 	| 'SHOW' 'BACKUP' 'RANGES' string_or_placeholder 'WITH' 'OPTIONS' '(' kv_option_list ')'
 	| 'SHOW' 'BACKUP' 'RANGES' string_or_placeholder 
+	| 'SHOW' 'BACKUP' 'VALIDATE' string_or_placeholder 'WITH' kv_option_list
+	| 'SHOW' 'BACKUP' 'VALIDATE' string_or_placeholder 'WITH' 'OPTIONS' '(' kv_option_list ')'
+	| 'SHOW' 'BACKUP' 'VALIDATE' string_or_placeholder 

docs/generated/sql/bnf/stmt_block.bnf

@@ -761,6 +761,7 @@ show_backup_stmt ::=
 	| 'SHOW' 'BACKUP' 'SCHEMAS' string_or_placeholder opt_with_options
 	| 'SHOW' 'BACKUP' 'FILES' string_or_placeholder opt_with_options
 	| 'SHOW' 'BACKUP' 'RANGES' string_or_placeholder opt_with_options
+	| 'SHOW' 'BACKUP' 'VALIDATE' string_or_placeholder opt_with_options
 
 show_columns_stmt ::=
 	'SHOW' 'COLUMNS' 'FROM' table_name with_comment
@@ -1878,6 +1879,7 @@ show_backup_details ::=
 	'SCHEMAS'
 	| 'FILES'
 	| 'RANGES'
+	| 'VALIDATE'
 
 with_comment ::=
 	'WITH' 'COMMENT'

pkg/ccl/backupccl/BUILD.bazel

@@ -90,6 +90,7 @@ go_library(
         "//pkg/sql/catalog/systemschema",
         "//pkg/sql/catalog/tabledesc",
         "//pkg/sql/catalog/typedesc",
+        "//pkg/sql/doctor",
         "//pkg/sql/execinfra",
         "//pkg/sql/execinfrapb",
         "//pkg/sql/parser",

pkg/ccl/backupccl/datadriven_test.go

@@ -367,6 +367,11 @@ func (d *datadrivenTestState) getSQLDB(t *testing.T, server string, user string)
 //
 //   - "corrupt-backup" uri=<collectionUri>
 //     Finds the latest backup in the provided collection uri an flips a bit in one SST in the backup
+//
+//   - "link-backup" server=<server> src-path=<testDataPathRelative> dest-path=<fileIO path relative>
+//     Creates a symlink from the testdata path to the file IO path, so that we
+//     can restore precreated backup. src-path and dest-path are comma seperated
+//     paths that will be joined.
 func TestDataDriven(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
@@ -865,7 +870,22 @@ func TestDataDriven(t *testing.T) {
 					t.Fatal(err)
 				}
 				return ""
-
+			case "link-backup":
+				server := lastCreatedServer
+				sourceRelativePath := ""
+				destRelativePath := ""
+				ioDir := ds.getIODir(t, server)
+				d.ScanArgs(t, "server", &server)
+				d.ScanArgs(t, "src-path", &sourceRelativePath)
+				d.ScanArgs(t, "dest-path", &destRelativePath)
+				splitSrcPath := strings.Split(sourceRelativePath, ",")
+				sourcePath, err := filepath.Abs(testutils.TestDataPath(t, splitSrcPath...))
+				require.NoError(t, err)
+				splitDestPath := strings.Split(destRelativePath, ",")
+				destPath := filepath.Join(ioDir, filepath.Join(splitDestPath...))
+				require.NoError(t, err)
+				require.NoError(t, os.Symlink(sourcePath, destPath))
+				return ""
 			default:
 				return fmt.Sprintf("unknown command: %s", d.Cmd)
 			}

pkg/ccl/backupccl/restore_old_versions_test.go

@@ -588,10 +588,10 @@ func restoreOldVersionClusterTest(exportDir string) func(t *testing.T) {
 		sqlDB.Exec(t, `RESTORE FROM $1`, localFoo)
 
 		sqlDB.CheckQueryResults(t, "SHOW DATABASES", [][]string{
-			{"data", "root", "NULL", "{}", "NULL"},
-			{"defaultdb", "root", "NULL", "{}", "NULL"},
-			{"postgres", "root", "NULL", "{}", "NULL"},
-			{"system", "node", "NULL", "{}", "NULL"},
+			{"data", "root", "NULL", "NULL", "{}", "NULL"},
+			{"defaultdb", "root", "NULL", "NULL", "{}", "NULL"},
+			{"postgres", "root", "NULL", "NULL", "{}", "NULL"},
+			{"system", "node", "NULL", "NULL", "{}", "NULL"},
 		})
 
 		sqlDB.CheckQueryResults(t, "SHOW SCHEMAS", [][]string{

pkg/ccl/backupccl/show.go

@@ -23,6 +23,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/ccl/backupccl/backuputils"
 	"github.com/cockroachdb/cockroach/pkg/ccl/storageccl"
 	"github.com/cockroachdb/cockroach/pkg/cloud"
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
@@ -34,6 +35,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descbuilder"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
+	"github.com/cockroachdb/cockroach/pkg/sql/doctor"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgnotice"
@@ -46,6 +48,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/json"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/mon"
+	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/cockroach/pkg/util/tracing"
 	"github.com/cockroachdb/errors"
@@ -254,6 +257,9 @@ func showBackupPlanHook(
 			shower = backupShowerFileSetup(backup.InCollection)
 		case tree.BackupSchemaDetails:
 			shower = backupShowerDefault(p, true, opts)
+		case tree.BackupValidateDetails:
+			shower = backupShowerDoctor
+
 		default:
 			shower = backupShowerDefault(p, false, opts)
 		}
@@ -1018,6 +1024,69 @@ var backupShowerRanges = backupShower{
 	},
 }
 
+var backupShowerDoctor = backupShower{
+	header: colinfo.ResultColumns{
+		{Name: "validation_status", Typ: types.String},
+	},
+
+	fn: func(ctx context.Context, info backupInfo) (rows []tree.Datums, err error) {
+		var descTable doctor.DescriptorTable
+		var namespaceTable doctor.NamespaceTable
+		// Extract all the descriptors from the given manifest and generate the
+		// namespace and descriptor tables needed by doctor.
+		descriptors, _ := backupinfo.LoadSQLDescsFromBackupsAtTime(info.manifests, hlc.Timestamp{})
+		for _, desc := range descriptors {
+			builder := desc.NewBuilder()
+			mutDesc := builder.BuildCreatedMutable()
+			bytes, err := protoutil.Marshal(mutDesc.DescriptorProto())
+			if err != nil {
+				return nil, err
+			}
+			descTable = append(descTable,
+				doctor.DescriptorTableRow{
+					ID:        int64(desc.GetID()),
+					DescBytes: bytes,
+					ModTime:   desc.GetModificationTime(),
+				})
+			namespaceTable = append(namespaceTable,
+				doctor.NamespaceTableRow{
+					ID: int64(desc.GetID()),
+					NameInfo: descpb.NameInfo{
+						Name:           desc.GetName(),
+						ParentID:       desc.GetParentID(),
+						ParentSchemaID: desc.GetParentSchemaID(),
+					},
+				})
+		}
+		validationMessages := strings.Builder{}
+		// We will intentionally not validate any jobs inside the manifest, since
+		// these will be synthesized by the restore process.
+		cv := clusterversion.DoctorBinaryVersion
+		if len(info.manifests) > 0 {
+			cv = info.manifests[len(info.manifests)-1].ClusterVersion
+		}
+		ok, err := doctor.Examine(ctx,
+			clusterversion.ClusterVersion{Version: cv},
+			descTable, namespaceTable,
+			nil,
+			false, /*validateJobs*/
+			false,
+			&validationMessages)
+		if err != nil {
+			return nil, err
+		}
+		if !ok {
+			validationMessages.WriteString("ERROR: validation failed\n")
+		} else {
+			validationMessages.WriteString("No problems found!\n")
+		}
+		rows = append(rows, tree.Datums{
+			tree.NewDString(validationMessages.String()),
+		})
+		return rows, nil
+	},
+}
+
 func backupShowerFileSetup(inCol tree.StringOrPlaceholderOptList) backupShower {
 	return backupShower{header: colinfo.ResultColumns{
 		{Name: "path", Typ: types.String},

pkg/ccl/backupccl/show_test.go

@@ -545,8 +545,8 @@ func TestShowNonDefaultBackups(t *testing.T) {
 	sqlDB.Exec(t, `BACKUP DATABASE data INTO $1`, fullNonDefault)
 
 	// Get base number of files, schemas, and ranges in the backup
-	var oldCount [3]int
-	for i, typ := range []string{"FILES", "SCHEMAS", "RANGES"} {
+	var oldCount [4]int
+	for i, typ := range []string{"FILES", "SCHEMAS", "RANGES", "VALIDATE"} {
 		query := fmt.Sprintf(`SELECT count(*) FROM [SHOW BACKUP %s FROM LATEST IN '%s']`, typ,
 			fullNonDefault)
 		count, err := strconv.Atoi(sqlDB.QueryStr(t, query)[0][0])

pkg/ccl/backupccl/testdata/backup-restore/multiregion

@@ -41,11 +41,11 @@ RESTORE DATABASE d FROM LATEST IN 'nodelocal://0/database_backup/';
 query-sql
 SHOW DATABASES;
 ----
-d root us-east-1 {eu-central-1,us-east-1,us-west-1} zone
-data root <nil> {} <nil>
-defaultdb root <nil> {} <nil>
-postgres root <nil> {} <nil>
-system node <nil> {} <nil>
+d root us-east-1  {eu-central-1,us-east-1,us-west-1} zone
+data root <nil> <nil> {} <nil>
+defaultdb root <nil> <nil> {} <nil>
+postgres root <nil> <nil> {} <nil>
+system node <nil> <nil> {} <nil>
 
 # A new cluster with different localities settings.
 new-server name=s3 share-io-dir=s1 allow-implicit-access localities=eu-central-1,eu-north-1
@@ -117,10 +117,10 @@ HINT: to change the default primary region, use SET CLUSTER SETTING sql.defaults
 query-sql
 SHOW DATABASES;
 ----
-defaultdb root <nil> {} <nil>
-no_region_db root eu-central-1 {eu-central-1} zone
-postgres root <nil> {} <nil>
-system node <nil> {} <nil>
+defaultdb root <nil> <nil> {} <nil>
+no_region_db root eu-central-1  {eu-central-1} zone
+postgres root <nil> <nil> {} <nil>
+system node <nil> <nil> {} <nil>
 
 query-sql
 USE no_region_db;
@@ -162,11 +162,11 @@ HINT: to change the default primary region, use SET CLUSTER SETTING sql.defaults
 query-sql
 SHOW DATABASES;
 ----
-defaultdb root eu-north-1 {eu-north-1} zone
-no_region_db root eu-north-1 {eu-north-1} zone
-no_region_db_2 root eu-north-1 {eu-north-1} zone
-postgres root eu-north-1 {eu-north-1} zone
-system node <nil> {} <nil>
+defaultdb root eu-north-1  {eu-north-1} zone
+no_region_db root eu-north-1  {eu-north-1} zone
+no_region_db_2 root eu-north-1  {eu-north-1} zone
+postgres root eu-north-1  {eu-north-1} zone
+system node <nil> <nil> {} <nil>
 
 query-sql
 USE no_region_db;
@@ -182,9 +182,9 @@ RESTORE DATABASE eu_central_db FROM LATEST IN 'nodelocal://1/eu_central_database
 query-sql
 SHOW DATABASES;
 ----
-defaultdb root eu-north-1 {eu-north-1} zone
-eu_central_db root eu-central-1 {eu-central-1} zone
-no_region_db root eu-north-1 {eu-north-1} zone
-no_region_db_2 root eu-north-1 {eu-north-1} zone
-postgres root eu-north-1 {eu-north-1} zone
-system node <nil> {} <nil>
+defaultdb root eu-north-1  {eu-north-1} zone
+eu_central_db root eu-central-1  {eu-central-1} zone
+no_region_db root eu-north-1  {eu-north-1} zone
+no_region_db_2 root eu-north-1  {eu-north-1} zone
+postgres root eu-north-1  {eu-north-1} zone
+system node <nil> <nil> {} <nil>

pkg/ccl/backupccl/testdata/backup-restore/restore-schema-only-multiregion

@@ -44,11 +44,11 @@ RESTORE DATABASE d FROM LATEST IN 'nodelocal://0/database_backup/' with schema_o
 query-sql
 SHOW DATABASES;
 ----
-d root us-east-1 {eu-central-1,us-east-1,us-west-1} zone
-data root <nil> {} <nil>
-defaultdb root <nil> {} <nil>
-postgres root <nil> {} <nil>
-system node <nil> {} <nil>
+d root us-east-1  {eu-central-1,us-east-1,us-west-1} zone
+data root <nil> <nil> {} <nil>
+defaultdb root <nil> <nil> {} <nil>
+postgres root <nil> <nil> {} <nil>
+system node <nil> <nil> {} <nil>
 
 # A new cluster with different localities settings.
 new-server name=s3 share-io-dir=s1 allow-implicit-access localities=eu-central-1,eu-north-1
@@ -120,10 +120,10 @@ HINT: to change the default primary region, use SET CLUSTER SETTING sql.defaults
 query-sql
 SHOW DATABASES;
 ----
-defaultdb root <nil> {} <nil>
-no_region_db root eu-central-1 {eu-central-1} zone
-postgres root <nil> {} <nil>
-system node <nil> {} <nil>
+defaultdb root <nil> <nil> {} <nil>
+no_region_db root eu-central-1  {eu-central-1} zone
+postgres root <nil> <nil> {} <nil>
+system node <nil> <nil> {} <nil>
 
 query-sql
 USE no_region_db;
@@ -165,11 +165,11 @@ HINT: to change the default primary region, use SET CLUSTER SETTING sql.defaults
 query-sql
 SHOW DATABASES;
 ----
-defaultdb root eu-north-1 {eu-north-1} zone
-no_region_db root eu-north-1 {eu-north-1} zone
-no_region_db_2 root eu-north-1 {eu-north-1} zone
-postgres root eu-north-1 {eu-north-1} zone
-system node <nil> {} <nil>
+defaultdb root eu-north-1  {eu-north-1} zone
+no_region_db root eu-north-1  {eu-north-1} zone
+no_region_db_2 root eu-north-1  {eu-north-1} zone
+postgres root eu-north-1  {eu-north-1} zone
+system node <nil> <nil> {} <nil>
 
 query-sql
 USE no_region_db;
@@ -185,9 +185,9 @@ RESTORE DATABASE eu_central_db FROM LATEST IN 'nodelocal://1/eu_central_database
 query-sql
 SHOW DATABASES;
 ----
-defaultdb root eu-north-1 {eu-north-1} zone
-eu_central_db root eu-central-1 {eu-central-1} zone
-no_region_db root eu-north-1 {eu-north-1} zone
-no_region_db_2 root eu-north-1 {eu-north-1} zone
-postgres root eu-north-1 {eu-north-1} zone
-system node <nil> {} <nil>
+defaultdb root eu-north-1  {eu-north-1} zone
+eu_central_db root eu-central-1  {eu-central-1} zone
+no_region_db root eu-north-1  {eu-north-1} zone
+no_region_db_2 root eu-north-1  {eu-north-1} zone
+postgres root eu-north-1  {eu-north-1} zone
+system node <nil> <nil> {} <nil>

pkg/ccl/backupccl/testdata/backup-restore/show_backup

@@ -0,0 +1,39 @@
+# These tests validate the SHOW BACKUP command (old and new stynax) with
+# backup images that contain both invalid and valid sets of descriptors.
+
+new-server name=s1 allow-implicit-access
+----
+
+link-backup server=s1 src-path=show_backup_validate,invalidDependOnBy_21.1 dest-path=invalidDependOnBy_21.1
+----
+
+# This backup intentionally has a dangling invalid depend on by reference.
+query-sql regex=invalid\sdepended-on-by
+SELECT * FROM [SHOW BACKUP VALIDATE FROM 'invalidDependOnBy_21.1' IN 'nodelocal://0/'];
+----
+true
+
+link-backup server=s1 src-path=show_backup_validate,valid-22.2 dest-path=valid-22.2
+----
+
+# This backup is completely valid, but has no jobs.
+query-sql regex=No\sproblems\sfound!
+SELECT * FROM [SHOW BACKUP VALIDATE FROM 'valid-22.2' IN 'nodelocal://0/'];
+----
+true
+
+link-backup server=s1 src-path=show_backup_validate,valid-22.2-with-job dest-path=valid-22.2-with-job
+----
+
+# This back up is valid, and taken when a job was actively working on the
+# descriptor.
+query-sql regex=No\sproblems\sfound!
+SELECT * FROM [SHOW BACKUP VALIDATE FROM 'valid-22.2-with-job' IN 'nodelocal://0/'];
+----
+true
+
+# Validates the same backup with the old syntax.
+query-sql regex=No\sproblems\sfound!
+SELECT * FROM [SHOW BACKUP VALIDATE 'nodelocal://0/valid-22.2-with-job'];
+----
+true

pkg/ccl/backupccl/testdata/show_backup_validate/invalidDependOnBy_21.1/BACKUP_MANIFEST-CHECKSUM

@@ -0,0 +1 @@
+'���

pkg/ccl/backupccl/testdata/show_backup_validate/valid-22.2-with-job/BACKUP_MANIFEST-CHECKSUM

@@ -0,0 +1 @@
+����

pkg/ccl/backupccl/testdata/show_backup_validate/valid-22.2/BACKUP_MANIFEST-CHECKSUM

@@ -0,0 +1 @@
+�@>�