Merge #121148 #121151

121148: server: clean up formatting of store props logged on start r=sumeerbhola a=jbowens

On start Cockroach logs some information about the stores it initialized. Previously, the store properties were included through logging the roachpb.StoreProperties struct with a +v format specifier. Since the WAL failover path is a pointer to a string, the address of the string was printed rather than its contents. This commit implements redact.SafeFormatter for the roachpb.StoreProperties type and uses it to fix this bug.

Epic: none
Release note: none

121151: changefeedccl: add logging for bulk oracle/rebalancing during planning r=rharding6373 a=andyyang890

This patch adds a log line to note when the new bulk oracle is used and
makes an existing log line for rebalancing active even when verbose
logging is not on to aid with future debugging.

Epic: None

Release note: None

Co-authored-by: Jackson Owens <[email protected]>
Co-authored-by: Andy Yang <[email protected]>

pkg/ccl/changefeedccl/changefeed_dist.go

@@ -396,6 +396,7 @@ func makePlan(
 		evalCtx := execCtx.ExtendedEvalContext()
 		oracle := replicaoracle.NewOracle(replicaOracleChoice, dsp.ReplicaOracleConfig(locFilter))
 		if useBulkOracle.Get(&evalCtx.Settings.SV) {
+			log.Infof(ctx, "using bulk oracle for DistSQL planning")
 			oracle = kvfollowerreadsccl.NewBulkOracle(dsp.ReplicaOracleConfig(evalCtx.Locality), locFilter, kvfollowerreadsccl.StreakConfig{})
 		}
 		planCtx := dsp.NewPlanningCtxWithOracle(ctx, execCtx.ExtendedEvalContext(), nil, /* planner */
@@ -410,9 +411,7 @@ func makePlan(
 		switch {
 		case distMode == sql.LocalDistribution || rangeDistribution == int64(defaultDistribution):
 		case rangeDistribution == int64(balancedSimpleDistribution):
-			if log.ExpensiveLogEnabled(ctx, 2) {
-				log.Infof(ctx, "rebalancing ranges using balanced simple distribution")
-			}
+			log.Infof(ctx, "rebalancing ranges using balanced simple distribution")
 			sender := execCtx.ExecCfg().DB.NonTransactionalSender()
 			distSender := sender.(*kv.CrossRangeTxnWrapperSender).Wrapped().(*kvcoord.DistSender)
 			ri := kvcoord.MakeRangeIterator(distSender)

pkg/roachpb/data.go

@@ -902,6 +902,29 @@ func (v Value) PrettyPrint() (ret string) {
 	return buf.String()
 }
 
+// SafeFormat implements the redact.SafeFormatter interface.
+func (sp StoreProperties) SafeFormat(w redact.SafePrinter, _ rune) {
+	w.SafeString(redact.SafeString(sp.Dir))
+	w.SafeString(":")
+	if sp.ReadOnly {
+		w.SafeString(" ro")
+	} else {
+		w.SafeString(" rw")
+	}
+	w.Printf(" encrypted=%t", sp.Encrypted)
+	if sp.WalFailoverPath != nil {
+		w.Printf(" wal_failover_path=%s", redact.SafeString(*sp.WalFailoverPath))
+	}
+	if sp.FileStoreProperties != nil {
+		w.SafeString(" fs:{")
+		w.Printf("bdev=%s", redact.SafeString(sp.FileStoreProperties.BlockDevice))
+		w.Printf(" fstype=%s", redact.SafeString(sp.FileStoreProperties.FsType))
+		w.Printf(" mountpoint=%s", redact.SafeString(sp.FileStoreProperties.MountPoint))
+		w.Printf(" mountopts=%s", redact.SafeString(sp.FileStoreProperties.MountOptions))
+		w.SafeString("}")
+	}
+}
+
 // Kind returns the kind of commit trigger as a string.
 func (ct InternalCommitTrigger) Kind() redact.SafeString {
 	switch {

pkg/roachpb/data_test.go

@@ -367,6 +367,50 @@ func TestValueGetErrorsRedacted(t *testing.T) {
 	require.Equal(t, string(redact.Sprintf("%s %s", err, "sensitive").Redact()), "value type is not INT: BYTES ‹×›")
 }
 
+func TestStorePropertiesSafeFormat(t *testing.T) {
+	type testCase struct {
+		props     *StoreProperties
+		formatted redact.RedactableString
+	}
+
+	walFailoverPath := "/mnt/data2/cockroach/auxiliary/among-stores"
+
+	testCases := []testCase{
+		{
+			props: &StoreProperties{
+				Dir:             "/mnt/data1/cockroach",
+				Encrypted:       true,
+				WalFailoverPath: &walFailoverPath,
+				FileStoreProperties: &FileStoreProperties{
+					BlockDevice:  "nvme1n1",
+					FsType:       "ext4",
+					MountPoint:   "/mnt/data1",
+					MountOptions: "rw,relatime",
+				},
+			},
+			formatted: "/mnt/data1/cockroach: rw encrypted=true wal_failover_path=/mnt/data2/cockroach/auxiliary/among-stores fs:{bdev=nvme1n1 fstype=ext4 mountpoint=/mnt/data1 mountopts=rw,relatime}",
+		},
+		{
+			props: &StoreProperties{
+				Dir:       "/mnt/data3/cockroach",
+				ReadOnly:  true,
+				Encrypted: false,
+				FileStoreProperties: &FileStoreProperties{
+					BlockDevice:  "nvme1n1",
+					FsType:       "zfs",
+					MountPoint:   "/mnt/data3",
+					MountOptions: "ro,relatime",
+				},
+			},
+			formatted: "/mnt/data3/cockroach: ro encrypted=false fs:{bdev=nvme1n1 fstype=zfs mountpoint=/mnt/data3 mountopts=ro,relatime}",
+		},
+	}
+	for _, tc := range testCases {
+		got := redact.Sprintf("%s", tc.props)
+		require.Equal(t, tc.formatted, got)
+	}
+}
+
 func TestSetGetChecked(t *testing.T) {
 	v := Value{}
 

pkg/server/config.go

@@ -861,7 +861,7 @@ func (cfg *Config) CreateEngines(ctx context.Context) (Engines, error) {
 		// TODO(jackson): Refactor to either reference count references to the env,
 		// or leave ownership with the caller of Open.
 		storeEnvs[i] = nil
-		detail(redact.Sprintf("store %d: %+v", i, eng.Properties()))
+		detail(redact.Sprintf("store %d: %s", i, eng.Properties()))
 		engines = append(engines, eng)
 	}