From c6423e9fdf0d1f85fba35059052d11a713efffd3 Mon Sep 17 00:00:00 2001 From: Marcus Gartner Date: Wed, 8 Apr 2020 14:47:13 -0700 Subject: [PATCH 1/4] opt: corrects function doc for canMaybeConstrainIndex This commit corrects the function documentation for canMaybeConstrainIndex. Prior to this change, it did not mention a third check, for tight filter constraints, that is performed to determine the possibility that an index could be constrained by a filer. Also, the documentation now correctly maps to the logic of the function. Previously it falsely claimed that if any of the checks were false then the function would return false. Now it correctly states that if any of the checks are true, then the function returns true. Release note: None --- pkg/sql/opt/xform/custom_funcs.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/pkg/sql/opt/xform/custom_funcs.go b/pkg/sql/opt/xform/custom_funcs.go index dcc6cc76be25..8641fd35009b 100644 --- a/pkg/sql/opt/xform/custom_funcs.go +++ b/pkg/sql/opt/xform/custom_funcs.go @@ -983,13 +983,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, From bab6e648bd49e716a487ac35f964bd75e44bf081 Mon Sep 17 00:00:00 2001 From: Tobias Schottdorf Date: Thu, 9 Apr 2020 13:28:55 +0200 Subject: [PATCH 2/4] roachtest: simplify version-upgrade The code to get the cluster version was already written, so reuse it. Release note: None --- pkg/cmd/roachtest/versionupgrade.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/roachtest/versionupgrade.go b/pkg/cmd/roachtest/versionupgrade.go index ee651374d7c0..a743e71d3a9e 100644 --- a/pkg/cmd/roachtest/versionupgrade.go +++ b/pkg/cmd/roachtest/versionupgrade.go @@ -398,15 +398,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 { From 8f1ebe40bc1585259887b1734d27b9fd5f43ac4f Mon Sep 17 00:00:00 2001 From: Tobias Schottdorf Date: Thu, 9 Apr 2020 13:43:33 +0200 Subject: [PATCH 3/4] roachtest: automate and de-flake version-upgrade The recent changes to this test seem to have uncovered a latent [bug], as a result of which the test was skipped. This commit lets the test start from v19.2 fixtures (instead of v19.1 before) which experimentally were verified to not exhibit the bug, so the test is unskipped. While I was there, I automated the base version discovery of this test. On a branch at version X, the test will now use the fixtures for the last major release preceding X. We'll occasionally have to bake new fixtures and update the mapping in PredecessorVersion() but this is better than having the test rot on older branches whenever we update it for a new dev cycle. The root cause of the bug will be fixed separately; this is tracked in issue #44732. [bug]: https://github.com/cockroachdb/cockroach/pull/47235#issuecomment-611419920 Release note: None --- pkg/cmd/roachtest/acceptance.go | 9 +++++++-- pkg/cmd/roachtest/versionupgrade.go | 5 ++--- 2 files changed, 9 insertions(+), 5 deletions(-) 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 a743e71d3a9e..f1f3c265f1bc 100644 --- a/pkg/cmd/roachtest/versionupgrade.go +++ b/pkg/cmd/roachtest/versionupgrade.go @@ -80,21 +80,20 @@ 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" 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. // // 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, From 6e0b72f50827214de9c43fbe05e12eb66a2b22f7 Mon Sep 17 00:00:00 2001 From: Tobias Schottdorf Date: Thu, 9 Apr 2020 15:27:14 +0200 Subject: [PATCH 4/4] roachtest: improve UX for version-upgrades fixtures Now that version-upgrade automatically starts up on the predecessor version, it is more important to be able to create fixtures easily. Streamline and document that process so that "anyone can do it". Release note: None --- pkg/cmd/roachtest/versionupgrade.go | 167 +++++++++++++++------------- 1 file changed, 88 insertions(+), 79 deletions(-) diff --git a/pkg/cmd/roachtest/versionupgrade.go b/pkg/cmd/roachtest/versionupgrade.go index f1f3c265f1bc..a3c5b604c84e 100644 --- a/pkg/cmd/roachtest/versionupgrade.go +++ b/pkg/cmd/roachtest/versionupgrade.go @@ -76,10 +76,6 @@ DROP TABLE test.t; `), } -const ( - headVersion = "HEAD" -) - 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. @@ -87,72 +83,59 @@ func runVersionUpgrade(ctx context.Context, t *test, c *cluster, predecessorVers // TODO(tbg): revisit as old versions are aged out of this test. c.encryptDefault = false + // 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(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() @@ -173,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 { @@ -216,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 @@ -231,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) } @@ -332,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"}) - } } } @@ -433,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") +}