Merge #71985 #72350 #72379

71985: sql: fix comment on constraint for tables in a schema r=e-mbrown a=RichardJCai

Release note (sql change): Previously if you did comment on constraint
on a table in a schema the command would succeed but the comment
would not actually created. Now the comment is successfully created.

72350: sql/*: fix improperly wrapped errors r=ajwerner a=rafiss

refs #42510

I'm working on a linter that detects errors that are not wrapped
correctly, and it discovered these.

Release note: None

72379: build: fix bazel invocation of stress in github-pull-request-make  r=rickystewart a=stevendanna

Every run of the stress and stressrace bazel CI jobs were failing
with:

    [17:16:00][Run stress tests] /bin/bash: stress: command not found

I haven't dug into the Git history enough to know why this was working
before. Rather, I've just copied what the `dev` tool does for me and
checked that the constructed `bazci` command does in fact work.

Fixes #72321

Co-authored-by: richardjcai <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Steven Danna <[email protected]>

pkg/cmd/github-pull-request-make/main.go

@@ -31,6 +31,7 @@ import (
 	"os/exec"
 	"path/filepath"
 	"regexp"
+	"strconv"
 	"strings"
 	"time"
 
@@ -40,23 +41,50 @@ import (
 	"golang.org/x/oauth2"
 )
 
-const githubAPITokenEnv = "GITHUB_API_TOKEN"
-const teamcityVCSNumberEnv = "BUILD_VCS_NUMBER"
-const targetEnv = "TARGET"
+const (
+	githubAPITokenEnv    = "GITHUB_API_TOKEN"
+	teamcityVCSNumberEnv = "BUILD_VCS_NUMBER"
+	targetEnv            = "TARGET"
+	// The following environment variables are for testing and are
+	// prefixed with GHM_ to help prevent accidentally triggering
+	// test code inside the CI pipeline.
+	packageEnv    = "GHM_PACKAGES"
+	forceBazelEnv = "GHM_FORCE_BAZEL"
+)
 
 // https://github.com/golang/go/blob/go1.7.3/src/cmd/go/test.go#L1260:L1262
 //
 // It is a Test (say) if there is a character after Test that is not a lower-case letter.
 // We don't want TesticularCancer.
 const goTestStr = `func (Test[^a-z]\w*)\(.*\*testing\.TB?\) {$`
 
+const bazelStressTarget = "@com_github_cockroachdb_stress//:stress"
+
 var currentGoTestRE = regexp.MustCompile(`.*` + goTestStr)
 var newGoTestRE = regexp.MustCompile(`^\+\s*` + goTestStr)
 
 type pkg struct {
 	tests []string
 }
 
+func pkgsFromGithubPRForSHA(
+	ctx context.Context, org string, repo string, sha string,
+) (map[string]pkg, error) {
+	client := ghClient(ctx)
+	currentPull := findPullRequest(ctx, client, org, repo, sha)
+	if currentPull == nil {
+		log.Printf("SHA %s not found in open pull requests, skipping stress", sha)
+		return nil, nil
+	}
+
+	diff, err := getDiff(ctx, client, org, repo, *currentPull.Number)
+	if err != nil {
+		return nil, err
+	}
+
+	return pkgsFromDiff(strings.NewReader(diff))
+}
+
 // pkgsFromDiff parses a git-style diff and returns a mapping from directories
 // to tests added in those directories in the given diff.
 func pkgsFromDiff(r io.Reader) (map[string]pkg, error) {
@@ -158,6 +186,25 @@ func getDiff(
 	return diff, err
 }
 
+func parsePackagesFromEnvironment(input string) (map[string]pkg, error) {
+	const expectedFormat = "PACKAGE_NAME=TEST_NAME[,TEST_NAME...][;PACKAGE_NAME=...]"
+	pkgTestStrs := strings.Split(input, ";")
+	pkgs := make(map[string]pkg, len(pkgTestStrs))
+	for _, pts := range pkgTestStrs {
+		ptsParts := strings.Split(pts, "=")
+		if len(ptsParts) < 2 {
+			return nil, fmt.Errorf("invalid format for package environment variable: %q (expected format: %s)",
+				input, expectedFormat)
+		}
+		pkgName := ptsParts[0]
+		tests := ptsParts[1]
+		pkgs[pkgName] = pkg{
+			tests: strings.Split(tests, ","),
+		}
+	}
+	return pkgs, nil
+}
+
 func main() {
 	sha, ok := os.LookupEnv(teamcityVCSNumberEnv)
 	if !ok {
@@ -172,32 +219,34 @@ func main() {
 		log.Fatalf("environment variable %s is %s; expected 'stress' or 'stressrace'", targetEnv, target)
 	}
 
-	const org = "cockroachdb"
-	const repo = "cockroach"
+	forceBazel := false
+	if forceBazelStr, ok := os.LookupEnv(forceBazelEnv); ok {
+		forceBazel, _ = strconv.ParseBool(forceBazelStr)
+	}
 
 	crdb, err := os.Getwd()
 	if err != nil {
 		log.Fatal(err)
 	}
 
-	ctx := context.Background()
-	client := ghClient(ctx)
-
-	currentPull := findPullRequest(ctx, client, org, repo, sha)
-	if currentPull == nil {
-		log.Printf("SHA %s not found in open pull requests, skipping stress", sha)
-		return
-	}
+	var pkgs map[string]pkg
+	if pkgStr, ok := os.LookupEnv(packageEnv); ok {
+		log.Printf("Using packages from environment variable %s", packageEnv)
+		pkgs, err = parsePackagesFromEnvironment(pkgStr)
+		if err != nil {
+			log.Fatal(err)
+		}
 
-	diff, err := getDiff(ctx, client, org, repo, *currentPull.Number)
-	if err != nil {
-		log.Fatal(err)
+	} else {
+		ctx := context.Background()
+		const org = "cockroachdb"
+		const repo = "cockroach"
+		pkgs, err = pkgsFromGithubPRForSHA(ctx, org, repo, sha)
+		if err != nil {
+			log.Fatal(err)
+		}
 	}
 
-	pkgs, err := pkgsFromDiff(strings.NewReader(diff))
-	if err != nil {
-		log.Fatal(err)
-	}
 	if len(pkgs) > 0 {
 		for name, pkg := range pkgs {
 			// 20 minutes total seems OK, but at least 2 minutes per test.
@@ -222,7 +271,7 @@ func main() {
 			}
 
 			var args []string
-			if bazel.BuiltWithBazel() {
+			if bazel.BuiltWithBazel() || forceBazel {
 				args = append(args, "test")
 				// NB: We use a pretty dumb technique to list the bazel test
 				// targets: we ask bazel query to enumerate all the tests in this
@@ -254,13 +303,13 @@ func main() {
 				args = append(args, "--test_arg=-test.timeout", fmt.Sprintf("--test_arg=%s", timeout))
 				// Give the entire test 1 more minute than the duration to wrap up.
 				args = append(args, fmt.Sprintf("--test_timeout=%d", int((duration+1*time.Minute).Seconds())))
-				// NB: stress and bazci are expected to be put in `PATH` by the caller.
-				args = append(args, "--run_under", fmt.Sprintf("stress -stderr -maxfails 1 -maxtime %s -p %d", duration, parallelism))
+				args = append(args, "--run_under", fmt.Sprintf("%s -stderr -maxfails 1 -maxtime %s -p %d", bazelStressTarget, duration, parallelism))
 				if target == "stressrace" {
 					args = append(args, "--config=race")
 				} else {
 					args = append(args, "--test_sharding_strategy=disabled")
 				}
+				// NB: bazci is expected to be put in `PATH` by the caller.
 				cmd := exec.Command("bazci", args...)
 				cmd.Stdout = os.Stdout
 				cmd.Stderr = os.Stderr

pkg/geo/geos/geos.go

@@ -194,10 +194,10 @@ func initGEOS(dirs []string) (*C.CR_GEOS, string, error) {
 		}
 		err = errors.CombineErrors(
 			err,
-			errors.Newf(
-				"geos: cannot load GEOS from dir %q: %s",
-				dir,
+			errors.Wrapf(
 				newErr,
+				"geos: cannot load GEOS from dir %q",
+				dir,
 			),
 		)
 	}

pkg/sql/colflow/colrpc/inbox.go

@@ -12,7 +12,6 @@ package colrpc
 
 import (
 	"context"
-	"fmt"
 	"io"
 	"math"
 	"sync/atomic"
@@ -33,6 +32,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/log/logcrash"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
+	"github.com/cockroachdb/errors"
 	"github.com/cockroachdb/logtags"
 )
 
@@ -208,7 +208,7 @@ func (i *Inbox) RunWithStream(
 	case readerCtx = <-i.contextCh:
 		log.VEvent(streamCtx, 2, "Inbox reader arrived")
 	case <-streamCtx.Done():
-		return fmt.Errorf("%s: streamCtx while waiting for reader (remote client canceled)", streamCtx.Err())
+		return errors.Wrap(streamCtx.Err(), "streamCtx error while waiting for reader (remote client canceled)")
 	case <-flowCtxDone:
 		// The flow context of the inbox host has been canceled. This can occur
 		// e.g. when the query is canceled, or when another stream encountered
@@ -233,7 +233,7 @@ func (i *Inbox) RunWithStream(
 		return nil
 	case <-streamCtx.Done():
 		// The client canceled the stream.
-		return fmt.Errorf("%s: streamCtx in Inbox stream handler (remote client canceled)", streamCtx.Err())
+		return errors.Wrap(streamCtx.Err(), "streamCtx error in Inbox stream handler (remote client canceled)")
 	}
 }
 
@@ -258,7 +258,7 @@ func (i *Inbox) Init(ctx context.Context) {
 		select {
 		case i.stream = <-i.streamCh:
 		case err := <-i.timeoutCh:
-			i.errCh <- fmt.Errorf("%s: remote stream arrived too late", err)
+			i.errCh <- errors.Wrap(err, "remote stream arrived too late")
 			return err
 		case <-i.Ctx.Done():
 			// Our reader canceled the context meaning that it no longer needs
@@ -325,7 +325,7 @@ func (i *Inbox) Next() coldata.Batch {
 			// Regardless of the cause we want to propagate such an error as
 			// expected one in all cases so that the caller could decide on how
 			// to handle it.
-			err = pgerror.Newf(pgcode.InternalConnectionFailure, "inbox communication error: %s", err)
+			err = pgerror.Wrap(err, pgcode.InternalConnectionFailure, "inbox communication error")
 			i.errCh <- err
 			colexecerror.ExpectedError(err)
 		}

pkg/sql/comment_on_constraint.go

@@ -17,7 +17,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
-	"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemadesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
@@ -62,8 +61,10 @@ func (n *commentOnConstraintNode) startExec(params runParams) error {
 	if err != nil {
 		return err
 	}
-	cSchema, ok := schemadesc.GetVirtualSchemaByID(n.tableDesc.GetParentSchemaID())
-	if !ok {
+	schema, err := params.p.Descriptors().GetImmutableSchemaByID(
+		params.ctx, params.extendedEvalCtx.Txn, n.tableDesc.GetParentSchemaID(), tree.SchemaLookupFlags{},
+	)
+	if err != nil {
 		return err
 	}
 
@@ -78,16 +79,16 @@ func (n *commentOnConstraintNode) startExec(params runParams) error {
 	switch kind := constraint.Kind; kind {
 	case descpb.ConstraintTypePK:
 		constraintDesc := constraint.Index
-		n.oid = hasher.PrimaryKeyConstraintOid(n.tableDesc.GetParentID(), cSchema.GetName(), n.tableDesc.GetID(), constraintDesc)
+		n.oid = hasher.PrimaryKeyConstraintOid(n.tableDesc.GetParentID(), schema.GetName(), n.tableDesc.GetID(), constraintDesc)
 	case descpb.ConstraintTypeFK:
 		constraintDesc := constraint.FK
-		n.oid = hasher.ForeignKeyConstraintOid(n.tableDesc.GetParentID(), cSchema.GetName(), n.tableDesc.GetID(), constraintDesc)
+		n.oid = hasher.ForeignKeyConstraintOid(n.tableDesc.GetParentID(), schema.GetName(), n.tableDesc.GetID(), constraintDesc)
 	case descpb.ConstraintTypeUnique:
 		constraintDesc := constraint.Index.ID
-		n.oid = hasher.UniqueConstraintOid(n.tableDesc.GetParentID(), cSchema.GetName(), n.tableDesc.GetID(), constraintDesc)
+		n.oid = hasher.UniqueConstraintOid(n.tableDesc.GetParentID(), schema.GetName(), n.tableDesc.GetID(), constraintDesc)
 	case descpb.ConstraintTypeCheck:
 		constraintDesc := constraint.CheckConstraint
-		n.oid = hasher.CheckConstraintOid(n.tableDesc.GetParentID(), cSchema.GetName(), n.tableDesc.GetID(), constraintDesc)
+		n.oid = hasher.CheckConstraintOid(n.tableDesc.GetParentID(), schema.GetName(), n.tableDesc.GetID(), constraintDesc)
 
 	}
 	// Setting the comment to NULL is the

pkg/sql/comment_on_constraint_test.go

@@ -33,7 +33,9 @@ func TestCommentOnConstraint(t *testing.T) {
 		CREATE DATABASE d;
 		SET DATABASE = d;
 		CREATE TABLE t ( a int UNIQUE, b numeric CONSTRAINT positive_price CHECK (b > 0), c int CHECK (b > c), CONSTRAINT pkey PRIMARY KEY (a,c));
-		CREATE TABLE t2 (a UUID PRIMARY KEY, b int NOT NULL REFERENCES  t (a))
+		CREATE TABLE t2 (a UUID PRIMARY KEY, b int NOT NULL REFERENCES  t (a));
+		CREATE SCHEMA s;
+		CREATE TABLE s.t ( a int UNIQUE, b numeric CONSTRAINT positive_price CHECK (b > 0), c int CHECK (b > c), CONSTRAINT pkey PRIMARY KEY (a,c));
 `); err != nil {
 		t.Fatal(err)
 	}
@@ -48,6 +50,11 @@ func TestCommentOnConstraint(t *testing.T) {
 			`SELECT obj_description(oid, 'pg_constraint') FROM pg_constraint WHERE conname='t_a_key'`,
 			gosql.NullString{String: `unique_comment`, Valid: true},
 		},
+		{
+			`COMMENT ON CONSTRAINT t_a_key ON s.t IS 'unique_comment'`,
+			`SELECT obj_description(oid, 'pg_constraint') FROM pg_constraint WHERE conname='t_a_key'`,
+			gosql.NullString{String: `unique_comment`, Valid: true},
+		},
 		{
 			`COMMENT ON CONSTRAINT positive_price ON t IS 'check_comment'`,
 			`SELECT obj_description(oid, 'pg_constraint') FROM pg_constraint WHERE conname='positive_price'`,

pkg/sql/exec_util.go

@@ -1799,7 +1799,7 @@ type registrySession interface {
 func (r *SessionRegistry) CancelQuery(queryIDStr string) (bool, error) {
 	queryID, err := StringToClusterWideID(queryIDStr)
 	if err != nil {
-		return false, fmt.Errorf("query ID %s malformed: %s", queryID, err)
+		return false, errors.Wrapf(err, "query ID %s malformed", queryID)
 	}
 
 	r.Lock()

pkg/sql/flowinfra/inbound.go

@@ -137,7 +137,7 @@ func processInboundStreamHelper(
 			if err != nil {
 				if err != io.EOF {
 					// Communication error.
-					err = pgerror.Newf(pgcode.InternalConnectionFailure, "inbox communication error: %s", err)
+					err = pgerror.Wrap(err, pgcode.InternalConnectionFailure, "inbox communication error")
 					sendErrToConsumer(err)
 					errChan <- err
 					return

pkg/sql/logictest/testdata/logic_test/drop_index

@@ -327,7 +327,7 @@ DROP INDEX t_secondary CASCADE;
 ALTER TABLE t DROP COLUMN b;
 INSERT INTO t SELECT a + 1 FROM t;
 
-statement error pgcode 23505 duplicate key value: decoding err=column-id "2" does not exist
+statement error pgcode 23505 duplicate key value got decoding error: column-id "2" does not exist
 UPSERT INTO t SELECT a + 1 FROM t;
 
 statement ok

pkg/sql/opt/optgen/cmd/langgen/main.go

@@ -130,7 +130,7 @@ func generate(compiled *lang.CompiledExpr, out string, genFunc genFunc) error {
 		if err != nil {
 			// Write out incorrect source for easier debugging.
 			b = buf.Bytes()
-			err = fmt.Errorf("code formatting failed with Go parse error\n%s:%s", out, err)
+			err = errors.Wrapf(err, "code formatting failed with Go parse error\n%s", out)
 		}
 	} else {
 		b = buf.Bytes()

pkg/sql/opt/optgen/cmd/optgen/main.go

@@ -218,7 +218,7 @@ func (g *optgen) generate(compiled *lang.CompiledExpr, genFunc genFunc) error {
 			// Write out incorrect source for easier debugging.
 			b = buf.Bytes()
 			out := g.cmdLine.Lookup("out").Value.String()
-			err = fmt.Errorf("code formatting failed with Go parse error\n%s:%s", out, err)
+			err = errors.Wrapf(err, "code formatting failed with Go parse error\n%s", out)
 		}
 	} else {
 		b = buf.Bytes()

pkg/sql/opt/optgen/lang/compiler.go

@@ -200,7 +200,7 @@ func (c *Compiler) compileRules(rules RuleSetExpr) bool {
 
 func (c *Compiler) addErr(src *SourceLoc, err error) {
 	if src != nil {
-		err = fmt.Errorf("%s: %s", src, err.Error())
+		err = errors.Wrapf(err, "%s", src)
 	}
 	c.errors = append(c.errors, err)
 }

pkg/sql/opt/props/func_dep_rand_test.go

@@ -432,7 +432,7 @@ func (tc *testConfig) checkAPIs(fd *FuncDepSet, tr testRelation) error {
 			to:     closure,
 			strict: true,
 		}); err != nil {
-			return fmt.Errorf("ComputeClosure%s incorrectly returns %s: %s", cols, closure, err)
+			return errors.Wrapf(err, "ComputeClosure%s incorrectly returns %s", cols, closure)
 		}
 
 		reduced := fd.ReduceCols(cols)
@@ -441,15 +441,15 @@ func (tc *testConfig) checkAPIs(fd *FuncDepSet, tr testRelation) error {
 			to:     cols,
 			strict: true,
 		}); err != nil {
-			return fmt.Errorf("ReduceCols%s incorrectly returns %s: %s", cols, reduced, err)
+			return errors.Wrapf(err, "ReduceCols%s incorrectly returns %s", cols, reduced)
 		}
 
 		var proj FuncDepSet
 		proj.CopyFrom(fd)
 		proj.ProjectCols(cols)
 		// The FDs after projection should still hold on the table.
 		if err := tr.checkFDs(&proj); err != nil {
-			return fmt.Errorf("ProjectCols%s incorrectly returns %s: %s", cols, proj.String(), err)
+			return errors.Wrapf(err, "ProjectCols%s incorrectly returns %s", cols, proj.String())
 		}
 	}