Merge pull request #104048 from knz/20230529-precond-checks

cockroachdb · Jun 1, 2023 · 84a410b · 84a410b
2 parents 6dd9210 + f9b7783
commit 84a410b
Show file tree

Hide file tree

Showing 3 changed files with 18 additions and 13 deletions.
diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go
@@ -84,6 +84,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/tracing/collector"
 	"github.com/cockroachdb/cockroach/pkg/util/tracing/tracingpb"
 	"github.com/cockroachdb/errors"
+	"github.com/cockroachdb/redact"
 )
 
 // CrdbInternalName is the name of the crdb_internal schema.
@@ -5011,7 +5012,8 @@ CREATE TABLE crdb_internal.invalid_objects (
   database_name STRING,
   schema_name   STRING,
   obj_name      STRING,
-  error         STRING
+  error         STRING,
+  error_redactable STRING NOT VISIBLE
 )`,
 	populate: func(
 		ctx context.Context, p *planner, dbContext catalog.DatabaseDescriptor, addRow func(...tree.Datum) error,
@@ -5061,6 +5063,7 @@ CREATE TABLE crdb_internal.invalid_objects (
 				tree.NewDString(scName),
 				tree.NewDString(objName),
 				tree.NewDString(validationError.Error()),
+				tree.NewDString(string(redact.Sprint(validationError))),
 			)
 		}
 

diff --git a/pkg/upgrade/upgrades/precondition_before_starting_an_upgrade.go b/pkg/upgrade/upgrades/precondition_before_starting_an_upgrade.go
@@ -12,8 +12,6 @@ package upgrades
 
 import (
 	"context"
-	"fmt"
-	"strings"
 
 	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/kv"
@@ -25,6 +23,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlutil"
 	"github.com/cockroachdb/cockroach/pkg/upgrade"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/redact"
 )
 
 func preconditionBeforeStartingAnUpgrade(
@@ -48,10 +47,10 @@ func preconditionBeforeStartingAnUpgrade(
 func preconditionNoInvalidDescriptorsBeforeUpgrading(
 	ctx context.Context, cs clusterversion.ClusterVersion, d upgrade.TenantDeps,
 ) error {
-	var errMsg strings.Builder
+	var errMsg redact.StringBuilder
 	err := d.InternalExecutorFactory.DescsTxnWithExecutor(ctx, d.DB, d.SessionData,
 		func(ctx context.Context, txn *kv.Txn, descriptors *descs.Collection, ie sqlutil.InternalExecutor) error {
-			query := `SELECT * FROM crdb_internal.invalid_objects`
+			query := `SELECT id, database_name, schema_name, obj_name, error_redactable FROM crdb_internal.invalid_objects`
 			rows, err := ie.QueryIterator(
 				ctx, "check-if-there-are-any-invalid-descriptors", txn /* txn */, query)
 			if err != nil {
@@ -68,13 +67,16 @@ func preconditionNoInvalidDescriptorsBeforeUpgrading(
 					tree.Name(tree.MustBeDString(row[3])),
 				)
 				tableID := descpb.ID(tree.MustBeDInt(row[0]))
-				errString := string(tree.MustBeDString(row[4]))
+				// Note: the cast from the `error` column to RedactableString here is only valid because
+				// the `error` column is populated via a RedactableString too. Conversion from string
+				// to RedactableString is not safe in general.
+				errString := redact.RedactableString(tree.MustBeDString(row[4]))
 				// TODO(ssd): Remove in 23.1 once we are sure that the migration which fixes this corruption has run.
-				if veryLikelyKnownUserfileBreakage(ctx, txn, descriptors, tableID, errString) {
-					log.Infof(ctx, "ignoring invalid descriptor %v (%v) with error %q because it looks like known userfile-related corruption",
-						descName.String(), tableID, errString)
+				if veryLikelyKnownUserfileBreakage(ctx, txn, descriptors, tableID, errString.StripMarkers()) {
+					log.Infof(ctx, "ignoring invalid descriptor %v (%v) with error because it looks like known userfile-related corruption: %v",
+						&descName, tableID, errString)
 				} else {
-					errMsg.WriteString(fmt.Sprintf("invalid descriptor: %v (%v) because %v\n", descName.String(), row[0], row[4]))
+					errMsg.Printf("invalid descriptor: %v (%d) because %v\n", &descName, tableID, errString)
 				}
 			}
 			return err
@@ -87,5 +89,5 @@ func preconditionNoInvalidDescriptorsBeforeUpgrading(
 	}
 	return pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
 		"there exists invalid descriptors as listed below; fix these descriptors "+
-			"before attempting to upgrade again:\n%v", errMsg.String())
+			"before attempting to upgrade again:\n%v", errMsg)
 }
diff --git a/pkg/upgrade/upgrades/precondition_before_starting_an_upgrade_external_test.go b/pkg/upgrade/upgrades/precondition_before_starting_an_upgrade_external_test.go
@@ -119,8 +119,8 @@ func TestPreconditionBeforeStartingAnUpgrade(t *testing.T) {
 		require.Error(t, err, "upgrade should be refused because precondition is violated.")
 		require.Equal(t, "pq: verifying precondition for version 22.1-2: "+
 			"there exists invalid descriptors as listed below; fix these descriptors before attempting to upgrade again:\n"+
-			"invalid descriptor: defaultdb.public.t (104) because 'relation \"t\" (104): invalid depended-on-by relation back reference: referenced descriptor ID 53: referenced descriptor not found'\n"+
-			"invalid descriptor: defaultdb.public.temp_tbl (104) because 'no matching name info found in non-dropped relation \"t\"'",
+			"invalid descriptor: defaultdb.public.t (104) because relation \"t\" (104): invalid depended-on-by relation back reference: referenced descriptor ID 53: referenced descriptor not found\n"+
+			"invalid descriptor: defaultdb.public.temp_tbl (104) because no matching name info found in non-dropped relation \"t\"",
 			strings.ReplaceAll(err.Error(), "1000022", "22"))
 		// The cluster version should remain at `v0`.
 		ts.tdb.CheckQueryResults(t, "SHOW CLUSTER SETTING version", [][]string{{v0.String()}})