diff --git a/pkg/cmd/roachtest/acceptance.go b/pkg/cmd/roachtest/acceptance.go index 7965d827a557..1770bb3cab9c 100644 --- a/pkg/cmd/roachtest/acceptance.go +++ b/pkg/cmd/roachtest/acceptance.go @@ -46,8 +46,13 @@ func registerAcceptance(r *testRegistry) { {name: "status-server", fn: runStatusServer}, { name: "version-upgrade", - fn: runVersionUpgrade, - skip: "skipped due to flakiness", + fn: func(ctx context.Context, t *test, c *cluster) { + predV, err := PredecessorVersion(r.buildVersion) + if err != nil { + t.Fatal(err) + } + runVersionUpgrade(ctx, t, c, predV) + }, // This test doesn't like running on old versions because it upgrades to // the latest released version and then it tries to "head", where head is // the cockroach binary built from the branch on which the test is diff --git a/pkg/cmd/roachtest/versionupgrade.go b/pkg/cmd/roachtest/versionupgrade.go index ee651374d7c0..a3c5b604c84e 100644 --- a/pkg/cmd/roachtest/versionupgrade.go +++ b/pkg/cmd/roachtest/versionupgrade.go @@ -76,84 +76,66 @@ DROP TABLE test.t; `), } -const ( - headVersion = "HEAD" -) - -func runVersionUpgrade(ctx context.Context, t *test, c *cluster) { +func runVersionUpgrade(ctx context.Context, t *test, c *cluster, predecessorVersion string) { // This is ugly, but we can't pass `--encrypt=false` to old versions of // Cockroach. // // TODO(tbg): revisit as old versions are aged out of this test. c.encryptDefault = false - const baseVersion = "19.1.5" + // Set the bool within to true to create a new fixture for this test. This + // is necessary after every release. For example, when day `master` becomes + // the 20.2 release, this test will fail because it is missing a fixture for + // 20.1; run the test (on 20.1) with the bool flipped to create the fixture. + // Check it in (instructions are on the 'checkpointer' struct) and off we + // go. + cp := checkpointer{on: false} + u := newVersionUpgradeTest(c, versionUpgradeTestFeatures, - // Load baseVersion fixture. That fixture's cluster version may be - // at the predecessor version, so add a waitForUpgradeStep to make - // sure we're actually running at baseVersion before moving on. + // Start the cluster from a fixture. That fixture's cluster version may + // be at the predecessor version (though in practice it's fully up to + // date, if it was created via the checkpointer above), so add a + // waitForUpgradeStep to make sure we're upgraded all the way before + // moving on. // // See the comment on createCheckpoints for details on fixtures. - uploadAndStartFromCheckpointFixture(baseVersion), + uploadAndStartFromCheckpointFixture(predecessorVersion), waitForUpgradeStep(), - // NB: before the next step, cluster and binary version equals baseVersion, + // NB: at this point, cluster and binary version equal predecessorVersion, // and auto-upgrades are on. - binaryUpgradeStep("19.2.1"), - waitForUpgradeStep(), - - // Each new release has to be added here. When adding a new release, - // you'll probably need to use a release candidate binary. You may also - // want to bake in the last step above, i.e. run this test with - // createCheckpoints=true, update the fixtures (see comments there), and - // then change baseVersion to one release later. - - // HEAD gives us the main binary for this roachtest run. We upgrade into - // this version more capriciously to ensure better coverage by first - // rolling the cluster into the new version with auto-upgrade disabled, - // then rolling back, and then rolling forward and finalizing - // (automatically). + // We use an empty string for the version below, which means to use the + // main ./cockroach binary (i.e. the one being tested in this run). + // We upgrade into this version more capriciously to ensure better + // coverage by first rolling the cluster into the new version with + // auto-upgrade disabled, then rolling back, and then rolling forward + // and finalizing on the auto-upgrade path. preventAutoUpgradeStep(), // Roll nodes forward. - binaryUpgradeStep("HEAD"), + binaryUpgradeStep(""), // Roll back again. Note that bad things would happen if the cluster had // ignored our request to not auto-upgrade. The `autoupgrade` roachtest // exercises this in more detail, so here we just rely on things working // as they ought to. - binaryUpgradeStep("19.2.1"), - // Roll nodes forward, this time allowing them to upgrade. - binaryUpgradeStep("HEAD"), + binaryUpgradeStep(predecessorVersion), + // Roll nodes forward, this time allowing them to upgrade, and waiting + // for it to happen. + binaryUpgradeStep(""), allowAutoUpgradeStep(), waitForUpgradeStep(), + + // This is usually a noop, but if cp.on was set above, this is where we + // create fixtures. This is last so that we're creating the fixtures for + // the binary we're testing (i.e. if this run is on release-20.1, we'll + // create a 20.1 fixture). + cp.createCheckpointsAndFail, ) u.run(ctx, t) } -// createCheckpoints is used to "age out" old versions of CockroachDB. We want -// to test data that was created at v1.0, but we don't actually want to run a -// long chain of binaries starting all the way at v1.0. Instead, we periodically -// bake a set of store directories that originally started out on v1.0 and -// maintain it as a fixture for this test. -// -// The checkpoints will be created in the log directories. The test will fail -// on purpose when it's done. After, manually invoke the following to move the -// archives to the right place and commit the result: -// -// for i in 1 2 3 4; do -// mkdir -p pkg/cmd/roachtest/fixtures/${i} && \ -// mv artifacts/acceptance/version-upgrade/run_1/${i}.logs/checkpoint-*.tgz \ -// pkg/cmd/roachtest/fixtures/${i}/ -// done -const createCheckpoints = false - func (u *versionUpgradeTest) run(ctx context.Context, t *test) { - if createCheckpoints { - // We rely on cockroach-HEAD to create checkpoint, so upload it early. - _ = u.uploadVersion(ctx, t, headVersion) - } - defer func() { for _, db := range u.conns { _ = db.Close() @@ -174,10 +156,6 @@ func (u *versionUpgradeTest) run(ctx context.Context, t *test) { } } } - - if createCheckpoints { - t.Fatal("failing on purpose to have store snapshots collected in artifacts") - } } type versionUpgradeTest struct { @@ -217,7 +195,7 @@ func (u *versionUpgradeTest) conn(ctx context.Context, t *test, i int) *gosql.DB func (u *versionUpgradeTest) uploadVersion(ctx context.Context, t *test, newVersion string) option { var binary string - if newVersion == headVersion { + if newVersion == "" { binary = cockroach } else { var err error @@ -232,7 +210,10 @@ func (u *versionUpgradeTest) uploadVersion(ctx context.Context, t *test, newVers } } - target := "./cockroach-" + newVersion + target := "./cockroach" + if newVersion != "" { + target += "-" + newVersion + } u.c.Put(ctx, binary, target, u.c.All()) return startArgs("--binary=" + target) } @@ -333,31 +314,6 @@ func binaryUpgradeStep(newVersion string) versionStep { // test? We could run logictests. We could add custom logic here. Maybe // this should all be pushed to nightly migration tests instead. } - - if createCheckpoints && newVersion != headVersion { - // If we're taking checkpoints, momentarily stop the cluster (we - // need to do that to get the checkpoints to reflect a - // consistent cluster state). The binary at this point will be - // the new one, but the cluster version was not explicitly - // bumped, though auto-update may have taken place already. - // For example, if newVersion is 2.1, the cluster version in - // the store directories may be 2.0 on some stores and 2.1 on - // the others (though if any are on 2.1, then that's what's - // stored in system.settings). - // This means that when we restart from that version, we're - // going to want to use the binary mentioned in the checkpoint, - // or at least one compatible with the *predecessor* of the - // checkpoint version. For example, for checkpoint-2.1, the - // cluster version might be 2.0, so we can only use the 2.0 or - // 2.1 binary, but not the 19.1 binary (as 19.1 and 2.0 are not - // compatible). - name := checkpointName(newVersion) - c.Stop(ctx, nodes) - c.Run(ctx, c.All(), "./cockroach-HEAD", "debug", "rocksdb", "--db={store-dir}", - "checkpoint", "--checkpoint_dir={store-dir}/"+name) - c.Run(ctx, c.All(), "tar", "-C", "{store-dir}/"+name, "-czf", "{log-dir}/"+name+".tgz", ".") - c.Start(ctx, t, nodes, args, startArgsDontEncrypt, roachprodArgOption{"--sequential=false"}) - } } } @@ -398,15 +354,11 @@ func waitForUpgradeStep() versionStep { for i := 1; i <= c.spec.NodeCount; i++ { err := retry.ForDuration(30*time.Second, func() error { - db := u.conn(ctx, t, i) - - var currentVersion string - if err := db.QueryRow("SHOW CLUSTER SETTING version").Scan(¤tVersion); err != nil { - t.Fatalf("%d: %s", i, err) - } + currentVersion := u.clusterVersion(ctx, t, i).String() if currentVersion != newVersion { return fmt.Errorf("%d: expected version %s, got %s", i, newVersion, currentVersion) } + t.l.Printf("%s: acked by n%d", currentVersion, i) return nil }) if err != nil { @@ -438,3 +390,55 @@ func stmtFeatureTest( }, } } + +// checkpointer creates fixtures to "age out" old versions of CockroachDB. We +// want to test data that was created at v1.0, but we don't actually want to run +// a long chain of binaries starting all the way at v1.0. Instead, we +// periodically bake a set of store directories that originally started out on +// v1.0 and maintain it as a fixture for this test. +// +// The checkpoints will be created in the log directories downloaded as part of +// the artifacts. The test will fail on purpose when it's done. After, manually +// invoke the following to move the archives to the right place and commit the +// result: +// +// for i in 1 2 3 4; do +// mkdir -p pkg/cmd/roachtest/fixtures/${i} && \ +// mv artifacts/acceptance/version-upgrade/run_1/${i}.logs/checkpoint-*.tgz \ +// pkg/cmd/roachtest/fixtures/${i}/ +// done +type checkpointer struct { + on bool +} + +func (cp *checkpointer) createCheckpointsAndFail( + ctx context.Context, t *test, u *versionUpgradeTest, +) { + if !cp.on { + return + } + c := u.c + nodes := u.c.All() + // If we're taking checkpoints, momentarily stop the cluster (we + // need to do that to get the checkpoints to reflect a + // consistent cluster state). The binary at this point will be + // the new one, but the cluster version was not explicitly + // bumped, though auto-update may have taken place already. + // For example, if newVersion is 2.1, the cluster version in + // the store directories may be 2.0 on some stores and 2.1 on + // the others (though if any are on 2.1, then that's what's + // stored in system.settings). + // This means that when we restart from that version, we're + // going to want to use the binary mentioned in the checkpoint, + // or at least one compatible with the *predecessor* of the + // checkpoint version. For example, for checkpoint-2.1, the + // cluster version might be 2.0, so we can only use the 2.0 or + // 2.1 binary, but not the 19.1 binary (as 19.1 and 2.0 are not + // compatible). + name := checkpointName(u.binaryVersion(ctx, t, 1).String()) + c.Stop(ctx, nodes) + c.Run(ctx, c.All(), "./cockroach", "debug", "rocksdb", "--db={store-dir}", + "checkpoint", "--checkpoint_dir={store-dir}/"+name) + c.Run(ctx, c.All(), "tar", "-C", "{store-dir}/"+name, "-czf", "{log-dir}/"+name+".tgz", ".") + t.Fatalf("successfully created checkpoints; failing test on purpose") +} diff --git a/pkg/sql/opt/xform/custom_funcs.go b/pkg/sql/opt/xform/custom_funcs.go index a28d33e952b7..0d050a764095 100644 --- a/pkg/sql/opt/xform/custom_funcs.go +++ b/pkg/sql/opt/xform/custom_funcs.go @@ -969,13 +969,16 @@ func (c *CustomFuncs) allInvIndexConstraints( return constraints, true } -// canMaybeConstrainIndex performs two checks that can quickly rule out the -// possibility that the given index can be constrained by the specified filter: +// canMaybeConstrainIndex returns true if we should try to constrain a given +// index by the given filter. It returns false if it is impossible for the +// filter can constrain the scan. // -// 1. If the filter does not reference the first index column, then no -// constraint can be generated. -// 2. If none of the filter's constraints start with the first index column, -// then no constraint can be generated. +// If any of the three following statements are true, then it is +// possible that the index can be constrained: +// +// 1. The filter references the first index column. +// 2. The constraints are not tight (see props.Scalar.TightConstraints). +// 3. Any of the filter's constraints start with the first index column. // func (c *CustomFuncs) canMaybeConstrainIndex( filters memo.FiltersExpr, tabID opt.TableID, indexOrd int,