clusterversion,sql: remove old cluster versions corresponding to 20.2 and 21.1 #67822

postamar · 2021-07-20T19:21:29Z

This pull request removes all old cluster versions corresponding to 20.2 and 21.1 which fall under the SQL group's responsibility.

Fixes #66546.

See #66546 and #65200 for an exhaustive (and possibly exhausting) list.

cockroach-teamcity · 2021-07-20T19:21:50Z

This change is

postamar · 2021-07-20T21:36:38Z

This PR is best reviewed commit-by-commit.

Note: you'll notice a ccl: remove local-mixed-20.2-21.1 logic tests commit. This was necessary for removing both ImplicitColumnPartitioning and MultiRegionFeatures. I could squash these three commits into one if anyone cares. I'm operating under the assumption that we don't care about mixed 20.2/21.1 tests at this point in master.

postamar · 2021-07-21T21:08:01Z

The bazel CI failure seems like it can be ignored. It's complaining about a timeout. I ran the bazel test locally and it succeeded after 507 seconds so I'm guessing that the threshold in the CI is a bit low or something.

ajwerner

LGTM with a few comments

ajwerner · 2021-07-27T03:08:06Z

pkg/startupmigrations/migrations.go

- includedInBootstrap: clusterversion.ByKey(clusterversion.AlterColumnTypeGeneral),
+ // This migration does not have a dedicated cluster version key but was


I think you get to just remove the workFn here; this migration is "baked in" so to speak.

I originally left this in because if I didn't some backup tests would fail, complaining that system.tenants doesn't exist. Among other tests, TestPaginatedBackupTenant.

I've looked into this some more and now think I surfaced an unrelated bug. In the bootstrap package, system.tenants gets created only if this is the system tenant. However the migration doesn't seem to care about this. Case in point if I change the current diff to add if !r.codec.ForSystemTenant() { return nil } to createTenantsTable the test fails.

The other test with problems was TestBackupRestoreInsideTenant/tenant-backup.

I came up with what I believe to be an acceptable fix in that in backup_planning.go I wrap the query to system.tenants in if p.ExecCfg().Codec.ForSystemTenant() { ... } which should be a no-op, AFAIK the table is empty for non-system tenants. Like I said it probably shouldn't even exist in those cases, yet it does in these backup tests. Is this an artifact of how we set up those test clusters or a real bug in the startupmigrations? I still haven't been able to tell.

ajwerner · 2021-07-27T03:11:54Z

pkg/sql/schemachanger/scpb/scpb.proto

@@ -97,7 +97,7 @@ message SequenceDependency {
 // For example, nextval('foo.public.seq') vs. nextval(12345::REGCLASS),
 // where 12345 is the ID of foo.public.seq.
 // Sequences referenced only by its ID have the ability to be renamed.
- bool by_id = 5 [(gogoproto.customname) = "ByID"];
+ reserved 5; // This field was used until 21.2.


We've never actually stored these things, so part of me thinks we can just nix this.

ajwerner · 2021-07-27T03:14:52Z

pkg/clusterversion/cockroach_versions.go

 // All versions which have a registered long-running migration must have a
 // version higher than this version.
- LongRunningMigrations
+ deletedLongRunningMigrations


Why not just delete these?

Can't remember why, now I don't see why not, so: will do.

Partially addresses cockroachdb#66546 by removing a cluster version and its associated dependencies which, for any cluster whose version is at least 21.1, is certain to be active. Release note: None

The code covered by these tests is exercised in mixed-version clusters with 20.2 and 21.1 nodes, as the name of the test suite implies. These are no longer relevant at this stage of the 21.2 release cycle. Release note: None

Partially addresses cockroachdb#66546 by removing a cluster version and its associated dependencies which, for any cluster whose version is at least 21.1, is certain to be active. Release note: None

ajwerner

Reviewed 2 of 3 files at r28, 1 of 9 files at r32, 2 of 6 files at r33.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru, @ajwerner, and @postamar)

postamar · 2021-07-28T13:01:17Z

bors r+

craig · 2021-07-28T14:32:51Z

Build succeeded:

GitHub CI (Cockroach)

postamar force-pushed the 66546 branch 2 times, most recently from 75b8389 to e3bbd7a Compare July 20, 2021 21:33

postamar marked this pull request as ready for review July 20, 2021 21:33

postamar requested review from a team and adityamaru and removed request for a team July 20, 2021 21:34

postamar force-pushed the 66546 branch from e3bbd7a to 0581e21 Compare July 21, 2021 15:54

ajwerner reviewed Jul 27, 2021

View reviewed changes

Marius Posta added 17 commits July 27, 2021 11:49

clusterversion: remove GeospatialType, Box2DType

f3d95c5

Partially addresses cockroachdb#66546 by removing a cluster version and its associated dependencies which, for any cluster whose version is at least 21.1, is certain to be active. Release note: None

clusterversion: remove AlterColumnTypeGeneral

597abbf

Partially addresses cockroachdb#66546 by removing a cluster version and its associated dependencies which, for any cluster whose version is at least 21.1, is certain to be active. Release note: None

clusterversion: remove UserDefinedSchemas

84e07ce

Partially addresses cockroachdb#66546 by removing a cluster version and its associated dependencies which, for any cluster whose version is at least 21.1, is certain to be active. Release note: None

clusterversion: remove NoOriginFKIndexes

cdb0bd9

Partially addresses cockroachdb#66546 by removing a cluster version and its associated dependencies which, for any cluster whose version is at least 21.1, is certain to be active. Release note: None

clusterversion: remove MaterializedViews

dbb1144

Partially addresses cockroachdb#66546 by removing a cluster version and its associated dependencies which, for any cluster whose version is at least 21.1, is certain to be active. Release note: None

clusterversion: remove EmptyArraysInInvertedIndexes

a5414f2

Partially addresses cockroachdb#66546 by removing a cluster version and its associated dependencies which, for any cluster whose version is at least 21.1, is certain to be active. Release note: None

clusterversion: remove UniqueWithoutIndexConstraints

7e05086

Partially addresses cockroachdb#66546 by removing a cluster version and its associated dependencies which, for any cluster whose version is at least 21.1, is certain to be active. Release note: None

clusterversion: remove VirtualComputedColumns

8a4bf03

Partially addresses cockroachdb#66546 by removing a cluster version and its associated dependencies which, for any cluster whose version is at least 21.1, is certain to be active. Release note: None

clusterversion: remove NewSchemaChanger

7efdcb0

Partially addresses cockroachdb#66546 by removing a cluster version and its associated dependencies which, for any cluster whose version is at least 21.1, is certain to be active. Release note: None

clusterversion: remove SequencesRegclass

d669af3

Partially addresses cockroachdb#66546 by removing a cluster version and its associated dependencies which, for any cluster whose version is at least 21.1, is certain to be active. Release note: None

ccl: remove local-mixed-20.2-21.1 logic tests

feefbde

The code covered by these tests is exercised in mixed-version clusters with 20.2 and 21.1 nodes, as the name of the test suite implies. These are no longer relevant at this stage of the 21.2 release cycle. Release note: None

clusterversion: remove ImplicitColumnPartitioning

2c067a2

Partially addresses cockroachdb#66546 by removing a cluster version and its associated dependencies which, for any cluster whose version is at least 21.1, is certain to be active. Release note: None

clusterversion: remove ForeignKeyRepresentationMigration

4df066d

Partially addresses cockroachdb#66546 by removing a cluster version and its associated dependencies which, for any cluster whose version is at least 21.1, is certain to be active. Release note: None

clusterversion: remove ProtectedTsMetaPrivilegesMigration

4b0e1b9

Partially addresses cockroachdb#66546 by removing a cluster version and its associated dependencies which, for any cluster whose version is at least 21.1, is certain to be active. Release note: None

clusterversion: remove MultiRegionFeatures

998f810

Partially addresses cockroachdb#66546 by removing a cluster version and its associated dependencies which, for any cluster whose version is at least 21.1, is certain to be active. Release note: None

clusterversion: remove ChangefeedsSupportPrimaryIndexChanges

e1c3673

Partially addresses cockroachdb#66546 by removing a cluster version and its associated dependencies which, for any cluster whose version is at least 21.1, is certain to be active. Release note: None

clusterversion: rename LongRunningMigrations

c5d9b13

Partially addresses cockroachdb#66546 by removing a cluster version and its associated dependencies which, for any cluster whose version is at least 21.1, is certain to be active. Release note: None

postamar force-pushed the 66546 branch from 0581e21 to c5d9b13 Compare July 27, 2021 20:48

postamar requested a review from a team as a code owner July 27, 2021 20:48

ajwerner approved these changes Jul 28, 2021

View reviewed changes

tbg removed the request for review from a team July 28, 2021 06:53

craig bot merged commit f5a630c into cockroachdb:master Jul 28, 2021

postamar deleted the 66546 branch July 28, 2021 14:41

ajwerner mentioned this pull request Jul 29, 2021

clusterversion: set binaryMinSupportedVersion to v21_1 #67281

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clusterversion,sql: remove old cluster versions corresponding to 20.2 and 21.1 #67822

clusterversion,sql: remove old cluster versions corresponding to 20.2 and 21.1 #67822

postamar commented Jul 20, 2021 •

edited

Loading

cockroach-teamcity commented Jul 20, 2021

postamar commented Jul 20, 2021

postamar commented Jul 21, 2021

ajwerner left a comment

ajwerner Jul 27, 2021

postamar Jul 27, 2021

postamar Jul 27, 2021 •

edited

Loading

postamar Jul 27, 2021

ajwerner Jul 27, 2021

postamar Jul 27, 2021

ajwerner Jul 27, 2021

postamar Jul 27, 2021

ajwerner left a comment

postamar commented Jul 28, 2021

craig bot commented Jul 28, 2021

		includedInBootstrap: clusterversion.ByKey(clusterversion.AlterColumnTypeGeneral),
		// This migration does not have a dedicated cluster version key but was

clusterversion,sql: remove old cluster versions corresponding to 20.2 and 21.1 #67822

clusterversion,sql: remove old cluster versions corresponding to 20.2 and 21.1 #67822

Conversation

postamar commented Jul 20, 2021 • edited Loading

cockroach-teamcity commented Jul 20, 2021

postamar commented Jul 20, 2021

postamar commented Jul 21, 2021

ajwerner left a comment

Choose a reason for hiding this comment

ajwerner Jul 27, 2021

Choose a reason for hiding this comment

postamar Jul 27, 2021

Choose a reason for hiding this comment

postamar Jul 27, 2021 • edited Loading

Choose a reason for hiding this comment

postamar Jul 27, 2021

Choose a reason for hiding this comment

ajwerner Jul 27, 2021

Choose a reason for hiding this comment

postamar Jul 27, 2021

Choose a reason for hiding this comment

ajwerner Jul 27, 2021

Choose a reason for hiding this comment

postamar Jul 27, 2021

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

postamar commented Jul 28, 2021

craig bot commented Jul 28, 2021

postamar commented Jul 20, 2021 •

edited

Loading

postamar Jul 27, 2021 •

edited

Loading