Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

*: remove 19.x cluster versions and version gates #47447

Closed
dt opened this issue Apr 13, 2020 · 2 comments
Closed

*: remove 19.x cluster versions and version gates #47447

dt opened this issue Apr 13, 2020 · 2 comments
Assignees

Comments

@dt
Copy link
Member

dt commented Apr 13, 2020

[edit from @lucy-zhang] We're lagging behind on cleaning up cluster versions. We would like to have this test pass with only the last 2 major cluster versions permitted, as originally intended:

// Iterate through all versions known to be active. For example, if
//
// byRelease = ["2.0", "2.1", "19.1"]
//
// then we know that the current release cycle is 19.2, so mixed version
// clusters are running at least 19.1, so anything slotted under 2.1 (like
// 2.1-12) and 2.0 is always-on. To avoid interfering with backports, we're
// a bit more lenient and allow one more release cycle until validation fails.
// In the above example, we would tolerate 2.1-x but not 2.0-x.
// Currently we're actually a few versions behind in enforcing a ban on old
// versions/migrations. See #47447.
if n := len(byRelease) - 5; n >= 0 {
var buf strings.Builder
for i, mami := range byRelease[:n+1] {
s := "next release"
if i+1 < len(byRelease)-1 {
nextMM := byRelease[i+1]
s = fmt.Sprintf("%d.%d", nextMM.major, nextMM.minor)
}
for _, nv := range mami.vs {
fmt.Fprintf(&buf, "introduced in %s: %s\n", s, nv.Key)
}
}
mostRecentRelease := byRelease[len(byRelease)-1]
return errors.Errorf(
"found versions that are always active because %d.%d is already "+
"released; these should be removed:\n%s",
mostRecentRelease.minor, mostRecentRelease.major,
buf.String(),
)
}
return nil

@dt dt self-assigned this Apr 13, 2020
@thoszhang thoszhang changed the title *: remove 19.1 cluster versions and version gates *: remove 19.x cluster versions and version gates Oct 12, 2020
@thoszhang
Copy link
Contributor

thoszhang commented Oct 12, 2020

Here are all the obsolete version gates we need to clean up in 21.1:

I've tagged people who may be able to help (with the help of git blame on where the version gates still exist), but feel free to reassign as necessary. Hopefully these are trivial changes, but it'd be good to have someone familiar with the surrounding logic actually do the cleanup.

@knz
Copy link
Contributor

knz commented Oct 14, 2020

I'm ok doing those assigned to me but this would work somewhat better if my "package of work" was a separate issue

craig bot pushed a commit that referenced this issue Oct 15, 2020
55453: clusterversion: clean up unused 19.2/20.1 cluster versions r=lucy-zhang a=lucy-zhang

clusterversion: remove commented-out old cluster versions

From now on, we're going to delete the entries for obsolete cluster
versions entirely instead of commenting them out.

Release note: None

----

clusterversion: clean up unused 19.2/20.1 cluster versions

Related to #47447.

Release note: None

Co-authored-by: Lucy Zhang <[email protected]>
craig bot pushed a commit that referenced this issue Oct 15, 2020
55558: opt: prove partial index implication in scanIndexIter r=mgartner a=mgartner

This commit updates `scanIndexIter` in order to make it safer to reduce
filters during partial index implication. The goal is to help avoid the
common mistake of using filters for an enumerated index that were
reduced in a previous iteration. Another benefit of these changes is
that code for checking filter/predicate implication has been
de-duplicated and moved into `scanIndexIter`.

The `Next` and for-loop iteration pattern has been replaced with a
`ForEach` and callback pattern. Partial indexes are only enumerated if
their partial indexes are implied by the provided filters. Filters
reduced during partial index implication are passed to the callback
function.

`HasInvertedIndexes` and `canMaybeConstrainIndexWithCols` previously
used the `scanIndexIter` for simple iteration. They no longer do to
avoid over complicating the `scanIndexIter` with features to halt
iteration and ignore partial index implication.

Release note: None

55562: sql: remove TimeTZ and Time Precision 19.2->20.1 gates r=arulajmani a=otan

Refs #47447 

Release note: None

55615: util: fix metamorphic build tag r=jordanlewis a=jordanlewis

Prevously, the build tag for when metamorphic was set to on was
misconfigured.

Release note: None

Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Oliver Tan <[email protected]>
Co-authored-by: Jordan Lewis <[email protected]>
craig bot pushed a commit that referenced this issue Oct 19, 2020
55607: sql: fix reg* cast escaping r=rafiss a=otan

Resolves #53686

----

tree: add more casting fixes for text to reg*

Release note (sql change): Added ability to cast a string containing all
integers to be cast into a given regtype, e.g. '1234'::regproc is now
valid.

Release note (bug fix): Fixed a bug where casts from string to regproc,
regtype or regprocedure would not work if they contained " characters at
the beginning or at the end.

sql: fix unescaped casts to regclass types

Release note (bug fix): Fix a bug where casts to
regclass were not escaped (i.e. when the
type or table name had " characters).



55618: server: improve error message on drain timeout r=andreimatei a=andreimatei

When draining times out, we print out a warning. Commonly that warning
tells you that the leases could not be drained in time. In this case,
the patch improves the message:
- don't print a useless stack trace any more
- refer to the cluster setting controlling the timeout
- refer to leases, not "Raft leadership". Draining deals with both, but
  leases are the concept that's somewhat exposed to users.

Release note: None

55638: tpcc: remove unused and unpartitionable stock_item_fk_idx r=nvanbenschoten a=nvanbenschoten

This was discussed in #50911. We ended up not making the change because
it didn't appear to improve performance. Given the fact that the index
is only used in a single place and can easily be replaced by the primary
key of the stock table, it doesn't seem possible that this would
actually hurt performance. However, it does seem possible for the index
to hurt performance in multi-region clusters, where the fk validation
was not guaranteed to be local because it was not using the partitioned
primary key of the stock table.

This allows us to remove a scary comment around partitioning code,
because the index was not partitionable by warehouse id.

55651: *: include bazel in .gitignore r=irfansharif a=otan

Release note: None

55696: clusterversion: remove unused timestamp-related cluster versions r=lucy-zhang a=lucy-zhang

Remove `VersionTimeTZType` and `VersionTimePrecision`, which are unused.

Part of #47447.

Release note: None

Co-authored-by: Oliver Tan <[email protected]>
Co-authored-by: Andrei Matei <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: Lucy Zhang <[email protected]>
craig bot pushed a commit that referenced this issue Nov 16, 2020
56397: clusterversion,*: remove most 19.2 and 20.1 cluster versions r=lucy-zhang a=lucy-zhang

This PR removes most 19.2 and 20.1 cluster versions and their version gates or associated baked-in migrations. I left a few of them alone where it wasn't obvious what to do, and will open separate issues for those.

Related to #47447.

Co-authored-by: Lucy Zhang <[email protected]>
@irfansharif irfansharif self-assigned this Nov 26, 2020
irfansharif added a commit to irfansharif/cockroach that referenced this issue Nov 26, 2020
It's an old cluster version, introduced in the 19.2 release cycle. It's
now safe to remove. Part of cockroachdb#47447. Fixes cockroachdb#56398.

Release note: None
irfansharif added a commit to irfansharif/cockroach that referenced this issue Nov 26, 2020
This, and all surrounding migration code and tests, are now safe to
remove. It mostly served as documentation, which we've moved to the
field itself. Part of cockroachdb#47447. Fixes cockroachdb#56401.

(While here, Let's also tell git that `versionkey_string.go` is a
generated file.)

Release note: None
irfansharif added a commit to irfansharif/cockroach that referenced this issue Nov 27, 2020
This, and all surrounding migration code and tests, are now safe to
remove. It mostly served as documentation, which we've moved to the
field itself. Part of cockroachdb#47447. Fixes cockroachdb#56401.

(While here, Let's also tell git that `versionkey_string.go` is a
generated file.)

Release note: None
irfansharif added a commit to irfansharif/cockroach that referenced this issue Nov 27, 2020
It's an old cluster version, introduced in the 19.2 release cycle. It's
now safe to remove. Part of cockroachdb#47447. Fixes cockroachdb#56398.

Release note: None
craig bot pushed a commit that referenced this issue Nov 27, 2020
57118: migration,server: plumb in initial version to the migration manager r=irfansharif a=irfansharif

It'll let us return early, and makes the manager "more functional" in its
behavior. 

We also promote the clusterversion type to a first-class citizen in the
external facing API for pkg/migration. This package concerns itself with
migrations between cluster versions and we should have our API reflect as much.

The proto changes are safe, we haven't had a major release with the previous
proto definitions.

Release note: None

57155: clusterversion,*: remove VersionContainsEstimatesCounter r=irfansharif a=irfansharif

This, and all surrounding migration code and tests, are now safe to
remove. It mostly served as documentation, which we've moved to the
field itself. Part of #47447. Fixes #56401.

(While here, Let's also tell git that `versionkey_string.go` is a
generated file.)

Release note: None

---

First commit is from #57154.

57156: sql,clusterversion: remove VersionAuthLocalAndTrustRejectMethods r=irfansharif a=irfansharif

It's an old cluster version, introduced in the 19.2 release cycle. It's
now safe to remove. Part of #47447. Fixes #56398.


Release note: None

---

See only last commit. First two are from #57154 and #57155 respectively.

Co-authored-by: irfan sharif <[email protected]>
irfansharif added a commit to irfansharif/cockroach that referenced this issue Nov 28, 2020
It's an old cluster version, introduced in the 19.2 release cycle. It's
now safe to remove. Part of cockroachdb#47447. Fixes cockroachdb#56398.

Release note: None
craig bot pushed a commit that referenced this issue Nov 28, 2020
57155: clusterversion,*: remove VersionContainsEstimatesCounter r=irfansharif a=irfansharif

This, and all surrounding migration code and tests, are now safe to
remove. It mostly served as documentation, which we've moved to the
field itself. Part of #47447. Fixes #56401.

(While here, Let's also tell git that `versionkey_string.go` is a
generated file.)

Release note: None

---

First commit is from #57154.

57156: sql,clusterversion: remove VersionAuthLocalAndTrustRejectMethods r=irfansharif a=irfansharif

It's an old cluster version, introduced in the 19.2 release cycle. It's
now safe to remove. Part of #47447. Fixes #56398.


Release note: None

---

See only last commit. First two are from #57154 and #57155 respectively.

Co-authored-by: irfan sharif <[email protected]>
@irfansharif irfansharif removed their assignment Dec 31, 2020
@dt dt closed this as completed Feb 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants