clusterversion,sql,backupccl: remove PublicSchemasWithDescriptors by celiala · Pull Request #85825 · cockroachdb/cockroach

celiala · 2022-08-09T16:58:50Z

This commit removes the 22.1 version gate PublicSchemasWithDescriptors, which was referenced in:

pkg/ccl/backupccl/
pkg/ccl/changefeedccl/
pkg/config/zonepb/zone.go
pkg/sql/catalog/descs/
pkg/sql/descriptor.go
pkg/sql/importer/
pkg/sql/rename_table.go
pkg/sql/reparent_database.go

Cleanup was done following guidance from 21.2 cleanup:

For the most part, if the gates were just simple if !version.IsActive { return x } or something, I just removed the block, and even if it was a little more complicated, like args = [x]; if version { args = append(args, y) }; foo(args) I still tried to mostly inline it such that it looked natural (i.e. remove that append and make it args = [x, y]).

However for just a couple more complicated cases that were referring to <21.2 versions that needed to be replaced when those were deleted, I added a placeholder clusterversion.TODOPre21_2 alias for 21.2. Replacing those calls with this alias shouldn't change their behavior -- it was already always true, since the code today should never run in a <21.2 cluster -- but means we can delete those older versions in the meantime and then the owners of these bits can decide how to update them.

Partially addresses #80663

Release note: none
Release justification: remove old version gates

cockroach-teamcity · 2022-08-09T16:58:58Z

This change is

postamar

LGTM, thanks for doing this. @RichardJCai can you take a look also if you have time, please ? In particular, should anything be done to the backup tests or is it safe to outright remove these such as has been done? I think it's OK, but still.

pkg/sql/rename_table.go

pkg/sql/importer/import_job.go

RichardJCai

Only thing from me is I'd like to keep the cleanup on fail backup test. Other than that LGTM

Reviewed 17 of 19 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @celiala, @postamar, and @RichardJCai)

pkg/ccl/backupccl/restore_old_versions_test.go line 1104 at r1 (raw file):

						Server: &server.TestingKnobs{
							DisableAutomaticVersionUpgrade: make(chan struct{}),
							BinaryVersionOverride:          clusterversion.ByKey(clusterversion.PublicSchemasWithDescriptors - 1),

I'd like to keep this test, I think other ones can go, I think this test will still pass if we remove the BinaryVersionOverride

celiala

TFTRs!

Only thing from me is I'd like to keep the cleanup on fail backup test.

Ack:

left tests in restore_old_versions_test.go
updated comments

I'll go ahead and merge once gets pass.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @postamar and @RichardJCai)

pkg/ccl/backupccl/restore_old_versions_test.go line 1104 at r1 (raw file):
Ack.

if we remove the BinaryVersionOverride

I wasn't clear what to do for this?

But for now I've replaced PublicSchemasWithDescriptors with TODOPreV22_1 so that we can keep the test while still removing the gate.

Code quote:

sqlDB.CheckQueryResults(t, `SELECT x FROM test.public.t`, [][]string{{"3"}

pkg/sql/rename_table.go line 174 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

This comment needs to be rewritten also.

Done.

pkg/sql/importer/import_job.go line 131 at r1 (raw file):

Previously, postamar (Marius Posta) wrote…

This comment could be rewritten as "The public schema is expected to always be present in the database".

Done.

celiala · 2022-08-15T14:29:49Z

These tests are failing - @postamar @RichardJCai - could you weigh-in on whether these are flakes or legit? TestRowLevelTTLJobRandomEntries/one_column_pk_multiple_nodes seems to require a test adjustment, but I'm unsure of how? (also, feel free to take-over/add to this PR as needed, if easier :) )

Bazel Essential CI:

pkg/ccl/serverccl/serverccl_test_/serverccl_test
- exit code 142

GitHub CI:

TestRowLevelTTLJobRandomEntries/one_column_pk_multiple_nodes
- see details below

Bazel Extended CI:

No pass/skip/fail event found for test
- TestMetadataSST

------- Stdout: -------
=== RUN   TestRowLevelTTLJobRandomEntries/one_column_pk_multiple_nodes
    ttljob_test.go:546: test case: ttljob_test.testCase{desc:"one column pk multiple nodes", createTable:"CREATE TABLE tbl (\n\tid UUID PRIMARY KEY DEFAULT gen_random_uuid(),\n\ttext TEXT\n) WITH (ttl_expire_after = '30 days')", preSetup:[]string(nil), postSetup:[]string(nil), numExpiredRows:1001, numNonExpiredRows:5, numSplits:10, forceNonMultiTenant:false, numNodes:5, expirationExpression:"", addRow:(func(*ttljob_test.rowLevelTTLTestJobTestHelper, *tree.CreateTable, time.Time))(nil)}
*
* INFO: Running test with the default test tenant. If you are only seeing a test case failure when this message appears, there may be a problem with your test case running within tenants.
*
    ttljob_test.go:679:
          Error Trace:  ttljob_test.go:679
          Error:        Not equal:
                        expected: 1001
                        actual  : 1002
          Test:         TestRowLevelTTLJobRandomEntries/one_column_pk_multiple_nodes
    --- FAIL: TestRowLevelTTLJobRandomEntries/one_column_pk_multiple_nodes (13.87s)

celiala · 2022-08-17T22:51:43Z

Closing in favor of #86225

Release note: None Release justification: remove old version gates

celiala · 2022-08-23T07:49:18Z

TFTRs!

Incorporated additions from #86225 to get PR to pass - thank you! 🙏🏼

bors r+

craig · 2022-08-23T09:17:43Z

Build succeeded:

Bazel Essential CI (Cockroach)

celiala mentioned this pull request Aug 9, 2022

clusterversion: bump min-supported version to 22.1 #80663

Closed

2 tasks

celiala force-pushed the remove-gate.PublicSchemasWithDescriptors branch from c3050df to 8d1bdb4 Compare August 9, 2022 17:43

celiala marked this pull request as ready for review August 9, 2022 17:53

celiala requested review from a team as code owners August 9, 2022 17:53

celiala requested a review from a team August 9, 2022 17:53

celiala requested review from a team as code owners August 9, 2022 17:53

celiala requested review from a team, RichardJCai, ajwerner, gh-casper and msbutler and removed request for a team August 9, 2022 17:53

postamar removed request for a team, ajwerner, gh-casper and msbutler August 10, 2022 14:24

postamar approved these changes Aug 10, 2022

View reviewed changes

pkg/sql/rename_table.go Outdated Show resolved Hide resolved

pkg/sql/importer/import_job.go Outdated Show resolved Hide resolved

RichardJCai approved these changes Aug 10, 2022

View reviewed changes

celiala force-pushed the remove-gate.PublicSchemasWithDescriptors branch from 8d1bdb4 to 2521803 Compare August 11, 2022 22:21

celiala commented Aug 11, 2022

View reviewed changes

celiala force-pushed the remove-gate.PublicSchemasWithDescriptors branch from 2521803 to 5e26a84 Compare August 11, 2022 23:04

RichardJCai mentioned this pull request Aug 16, 2022

clusterversion,sql,backupccl: remove PublicSchemasWithDescriptors #86225

Closed

celiala closed this Aug 18, 2022

celiala reopened this Aug 22, 2022

celiala force-pushed the remove-gate.PublicSchemasWithDescriptors branch from 5e26a84 to 0fe61f4 Compare August 22, 2022 21:35

clusterversion,sql,backupccl: remove PublicSchemasWithDescriptors

4d6e876

Release note: None Release justification: remove old version gates

celiala force-pushed the remove-gate.PublicSchemasWithDescriptors branch from 0fe61f4 to 4d6e876 Compare August 23, 2022 01:46

craig bot merged commit 281e642 into cockroachdb:master Aug 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clusterversion,sql,backupccl: remove PublicSchemasWithDescriptors#85825

clusterversion,sql,backupccl: remove PublicSchemasWithDescriptors#85825
craig[bot] merged 1 commit intocockroachdb:masterfrom
celiala:remove-gate.PublicSchemasWithDescriptors

celiala commented Aug 9, 2022 •

edited

Loading

Uh oh!

cockroach-teamcity commented Aug 9, 2022

Uh oh!

postamar left a comment

Uh oh!

Uh oh!

Uh oh!

RichardJCai left a comment

Uh oh!

celiala left a comment

Uh oh!

celiala commented Aug 15, 2022

Uh oh!

celiala commented Aug 17, 2022

Uh oh!

celiala commented Aug 23, 2022

Uh oh!

craig bot commented Aug 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

celiala commented Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Aug 9, 2022

Uh oh!

postamar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

RichardJCai left a comment

Choose a reason for hiding this comment

Uh oh!

celiala left a comment

Choose a reason for hiding this comment

Uh oh!

celiala commented Aug 15, 2022

Uh oh!

celiala commented Aug 17, 2022

Uh oh!

celiala commented Aug 23, 2022

Uh oh!

craig bot commented Aug 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

celiala commented Aug 9, 2022 •

edited

Loading