Merge #72607

72607: server: report the SQL instance ID as log tag `sqli` r=rimadeodhar a=knz

Informs #58938.

Prior to this patch, the SQL instance ID was not reported in log
messages, which made it hard to distinguish log messages coming from
different in-memory SQL instance servers (e.g. in tests).

This patch fixes it.

The patch also changes the log tag for SQL instances from just `sql`
to `sqli`, which is easier to search for and recognize.

Release note: None

Co-authored-by: Raphael 'kena' Poss <[email protected]>

docs/generated/redact_safe.md

@@ -3,6 +3,7 @@ The following types are considered always safe for reporting:
 File | Type
 --|--
 pkg/base/node_id.go | `*NodeIDContainer`
+pkg/base/node_id.go | `*SQLIDContainer`
 pkg/base/node_id.go | `*StoreIDContainer`
 pkg/cli/exit/exit.go | `Code`
 pkg/jobs/jobspb/wrap.go | `JobID`

pkg/base/node_id.go

@@ -162,13 +162,21 @@ func (s *StoreIDContainer) Set(ctx context.Context, val int32) {
 type SQLInstanceID int32
 
 func (s SQLInstanceID) String() string {
-	return strconv.FormatInt(int64(s), 10)
+	if s == 0 {
+		return "?"
+	}
+	return strconv.Itoa(int(s))
 }
 
 // SQLIDContainer wraps a SQLInstanceID and optionally a NodeID.
 type SQLIDContainer struct {
 	w             errorutil.TenantSQLDeprecatedWrapper // NodeID
 	sqlInstanceID SQLInstanceID
+
+	// If the value has been set, str represents the instance ID
+	// converted to string. We precompute this value to speed up
+	// String() and keep it from allocating memory dynamically.
+	str atomic.Value
 }
 
 // NewSQLIDContainer sets up an SQLIDContainer. It is handed either a positive SQLInstanceID
@@ -196,6 +204,7 @@ func (c *SQLIDContainer) SetSQLInstanceID(sqlInstanceID SQLInstanceID) error {
 		return errors.New("attempting to initialize instance ID when node ID is set")
 	}
 	c.sqlInstanceID = sqlInstanceID
+	c.str.Store(strconv.Itoa(int(sqlInstanceID)))
 	return nil
 }
 
@@ -227,6 +236,44 @@ func (c *SQLIDContainer) SQLInstanceID() SQLInstanceID {
 	return c.sqlInstanceID
 }
 
+// SafeValue implements the redact.SafeValue interface.
+func (c *SQLIDContainer) SafeValue() {}
+
+func (c *SQLIDContainer) String() string {
+	st := c.str.Load()
+	if st == nil {
+		// This can mean either that:
+		// - neither the instance ID nor the node ID has been set.
+		// - only the node ID has been set.
+		//
+		// In the latter case, we don't want to return "?" here, as in the
+		// NodeIDContainer case above: we want to return the node ID
+		// representation instead. Alas, there is no way to know
+		// but to open the node ID container box.
+		//
+		// TODO(knz): This could be greatly simplified if we accepted to
+		// use the same data type for both SQL instance ID and node ID
+		// containers.
+		v, ok := c.w.Optional()
+		if !ok {
+			// This is definitely not a node ID, and since we're in this
+			// branch of the conditional above, we don't have SQL instance
+			// ID either (yet).
+			return "?"
+		}
+		nc := v.(*NodeIDContainer)
+		st = nc.str.Load()
+		if st == nil {
+			// We're designating a Node ID, but it was not set yet.
+			return "?"
+		}
+		// Node ID was set. Keep its representation for the instance ID as
+		// well.
+		c.str.Store(st)
+	}
+	return st.(string)
+}
+
 // TestingIDContainer is an SQLIDContainer with hard-coded SQLInstanceID of 10 and
 // NodeID of 1.
 var TestingIDContainer = func() *SQLIDContainer {

pkg/server/tenant.go

@@ -358,7 +358,15 @@ func makeTenantSQLServerArgs(
 	stopper *stop.Stopper, kvClusterName string, baseCfg BaseConfig, sqlCfg SQLConfig,
 ) (sqlServerArgs, error) {
 	st := baseCfg.Settings
-	baseCfg.AmbientCtx.AddLogTag("sql", nil)
+
+	// We want all log messages issued on behalf of this SQL instance to report
+	// the instance ID (once known) as a tag.
+	instanceIDContainer := base.NewSQLIDContainer(0, nil)
+	// We use the tag "sqli" instead of just "sql" because the latter is
+	// too generic and would be hard to search if someone was looking at
+	// a log message and wondering what it stands for.
+	baseCfg.AmbientCtx.AddLogTag("sqli", instanceIDContainer)
+
 	// TODO(tbg): this is needed so that the RPC heartbeats between the testcluster
 	// and this tenant work.
 	//
@@ -511,7 +519,7 @@ func makeTenantSQLServerArgs(
 			externalStorageFromURI: externalStorageFromURI,
 			// Set instance ID to 0 and node ID to nil to indicate
 			// that the instance ID will be bound later during preStart.
-			nodeIDContainer: base.NewSQLIDContainer(0, nil),
+			nodeIDContainer: instanceIDContainer,
 		},
 		sqlServerOptionalTenantArgs: sqlServerOptionalTenantArgs{
 			tenantConnect: tenantConnect,