Merge #120861 #120941 #121119

120861: sql/pg_catalog: fix attname for dropped columns r=rafiss a=Jeremyyang920

Previously, the attname columns for dropped columns was padded used a %8d string format. This led to incorrect behavior as it meant a total of 8 '.' characters inclusive of the column ordinal as compared to the PG code base which always pads with 8 '.'. This affected migration tooling since it attempts to verify column names using the returned attname and led to mismatching column reports.

This commit fixes the issue by explicitly setting the padding to 8 '.' to match the PG implementation.

Fixes: #120855

Release note (bug fix): attname for dropped columns is now properly padded with 8 '.' to match PG.

120941: roachprod: add faster restarts, prettier multi-node sql printing r=dt a=dt

A couple minor quality-of-life improvements motivated by working on long-lived, multi-node DRT clusters.

121119: server: delete TestIntentResolution r=arulajmani a=arulajmani

This is an old test which is tightly coupled with how locks are tracked on the client. In particular, it expects all ranged locking requests to use `ResolveIntentRange` requests to resolve their locks; this is expected to change immminently (see #121086). It's also cumbersome to change given the randomization it uses to construct batches.

We've got coverage for things being tested here elsewhere. There's some integration tests in `intent_resolver_integration_test.go` that test things deterministically. There's also tests for the txn pipeliner and txn committer that test lock tracking on the client. I'm probably missing some other examples as well, but deleting this should be safe.

Epic: none

Release note: None

Co-authored-by: Jeremy Yang <[email protected]>
Co-authored-by: David Taylor <[email protected]>
Co-authored-by: Arul Ajmani <[email protected]>

pkg/cmd/roachprod/flags.go

@@ -199,6 +199,8 @@ func initFlags() {
 		"encrypt", startOpts.EncryptedStores, "start nodes with encryption at rest turned on")
 	startCmd.Flags().BoolVar(&startOpts.SkipInit,
 		"skip-init", startOpts.SkipInit, "skip initializing the cluster")
+	startCmd.Flags().BoolVar(&startOpts.IsRestart,
+		"restart", startOpts.IsRestart, "restart an existing cluster (skips serial start and init)")
 	startCmd.Flags().IntVar(&startOpts.InitTarget,
 		"init-target", startOpts.InitTarget, "node on which to run initialization")
 	startCmd.Flags().IntVar(&startOpts.StoreCount,

pkg/roachprod/install/cockroach.go

@@ -141,6 +141,10 @@ type StartOpts struct {
 	VirtualClusterLocation string // where separate process virtual clusters will be started
 	SQLInstance            int
 	StorageCluster         *SyncedCluster
+
+	// IsRestart allows skipping steps that are used during initial start like
+	// initialization and sequential node starts.
+	IsRestart bool
 }
 
 func (s *StartOpts) IsVirtualCluster() bool {
@@ -427,6 +431,13 @@ func (c *SyncedCluster) Start(ctx context.Context, l *logger.Logger, startOpts S
 
 	// Start cockroach processes and `init` cluster, if necessary.
 	if startOpts.Target != StartSharedProcessForVirtualCluster {
+		if startOpts.IsRestart {
+			l.Printf("%s (%s): starting cockroach processes", c.Name, startOpts.VirtualClusterName)
+			return c.Parallel(ctx, l, WithNodes(c.Nodes).WithDisplay("starting nodes"), func(ctx context.Context, node Node) (*RunResultDetails, error) {
+				return c.startNodeWithResult(ctx, l, node, startOpts)
+			})
+		}
+
 		l.Printf("%s (%s): starting cockroach processes", c.Name, startOpts.VirtualClusterName)
 		// For single node non-virtual clusters, `init` can be skipped
 		// because during the c.StartNode call above, the
@@ -684,12 +695,12 @@ func (c *SyncedCluster) ExecSQL(
 	return results, err
 }
 
-func (c *SyncedCluster) startNode(
+func (c *SyncedCluster) startNodeWithResult(
 	ctx context.Context, l *logger.Logger, node Node, startOpts StartOpts,
-) error {
+) (*RunResultDetails, error) {
 	startCmd, err := c.generateStartCmd(ctx, l, node, startOpts)
 	if err != nil {
-		return err
+		return newRunResultDetails(node, err), err
 	}
 	var uploadCmd string
 	if c.IsLocal() {
@@ -702,15 +713,21 @@ func (c *SyncedCluster) startNode(
 	uploadOpts.stdin = strings.NewReader(startCmd)
 	res, err = c.runCmdOnSingleNode(ctx, l, node, uploadCmd, uploadOpts)
 	if err != nil || res.Err != nil {
-		return errors.CombineErrors(err, res.Err)
+		return res, err
 	}
 
 	var runScriptCmd string
 	if c.IsLocal() {
 		runScriptCmd = fmt.Sprintf(`cd %s ; `, c.localVMDir(node))
 	}
 	runScriptCmd += "./cockroach.sh"
-	res, err = c.runCmdOnSingleNode(ctx, l, node, runScriptCmd, defaultCmdOpts("run-start-script"))
+	return c.runCmdOnSingleNode(ctx, l, node, runScriptCmd, defaultCmdOpts("run-start-script"))
+}
+
+func (c *SyncedCluster) startNode(
+	ctx context.Context, l *logger.Logger, node Node, startOpts StartOpts,
+) error {
+	res, err := c.startNodeWithResult(ctx, l, node, startOpts)
 	return errors.CombineErrors(err, res.Err)
 }
 

pkg/roachprod/roachprod.go

@@ -442,17 +442,63 @@ func SQL(
 	if len(c.Nodes) == 1 {
 		return c.ExecOrInteractiveSQL(ctx, l, tenantName, tenantInstance, cmdArray)
 	}
+
 	results, err := c.ExecSQL(ctx, l, c.Nodes, tenantName, tenantInstance, cmdArray)
 	if err != nil {
 		return err
 	}
 
-	for _, r := range results {
-		l.Printf("node %d:\n%s", r.Node, r.CombinedOut)
+	for i, r := range results {
+		printSQLResult(l, i, r, cmdArray)
 	}
 	return nil
 }
 
+// printSQLResult does a best-effort attempt to print single-result-row-per-node
+// result-sets gathered from many nodes as one-line-per-node instead of header
+// separated n-line blocks, to improve the overall readability, falling back to
+// normal header-plus-response-block per node otherwise.
+func printSQLResult(l *logger.Logger, i int, r *install.RunResultDetails, args []string) {
+	tableFormatted := false
+	for i, c := range args {
+		if c == "--format=table" || c == "--format" && len(args) > i+1 && args[i+1] == "table" {
+			tableFormatted = true
+			break
+		}
+	}
+
+	singleResultLen, resultLine := 3, 1 // 3 is header, result, empty-trailing.
+	if tableFormatted {
+		// table output adds separator above the result, and a trailing row count.
+		singleResultLen, resultLine = 5, 2
+	}
+	// If we got a header line and zero or one result lines, we can print the
+	// result line as one-line-per-node, rather than a header per node and then
+	// its n result lines, to make the aggregate output more readable. We can
+	// detect this by splitting on newline into only as many lines as we expect,
+	// and seeing if the final piece is empty or has the rest of >1 results in it.
+	lines := strings.SplitN(r.CombinedOut, "\n", singleResultLen)
+	if len(lines) > 0 && lines[len(lines)-1] == "" {
+		if i == 0 { // Print the header line of the results once.
+			fmt.Printf("    %s\n", lines[0])
+			if tableFormatted {
+				fmt.Printf("    %s\n", lines[1])
+			}
+		}
+		// Print the result line if there is one.
+		if len(lines) > resultLine {
+			fmt.Printf("%2d: %s\n", r.Node, lines[resultLine])
+			return
+		}
+		// No result from this node, so print a blank for its ID.
+		fmt.Printf("%2d:\n", r.Node)
+		return
+	}
+	// Just print the roachprod header identifying the node, then the node's whole
+	// response, including its internal header row.
+	l.Printf("node %d:\n%s", r.Node, r.CombinedOut)
+}
+
 // IP gets the ip addresses of the nodes in a cluster.
 func IP(l *logger.Logger, clusterName string, external bool) ([]string, error) {
 	if err := LoadClusters(); err != nil {

pkg/server/BUILD.bazel

@@ -429,7 +429,6 @@ go_test(
         "grpc_gateway_test.go",
         "helpers_test.go",
         "index_usage_stats_test.go",
-        "intent_test.go",
         "job_profiler_test.go",
         "load_endpoint_test.go",
         "main_test.go",
@@ -488,7 +487,6 @@ go_test(
         "//pkg/kv/kvpb",
         "//pkg/kv/kvserver",
         "//pkg/kv/kvserver/closedts",
-        "//pkg/kv/kvserver/kvserverbase",
         "//pkg/kv/kvserver/kvserverpb",
         "//pkg/kv/kvserver/kvstorage",
         "//pkg/kv/kvserver/liveness/livenesspb",
@@ -550,7 +548,6 @@ go_test(
         "//pkg/util/protoutil",
         "//pkg/util/randutil",
         "//pkg/util/stop",
-        "//pkg/util/syncutil",
         "//pkg/util/timeutil",
         "//pkg/util/tracing",
         "//pkg/util/tracing/tracingpb",