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

migrations: add migration to wait on in-flight schema changes #76154

Merged

Conversation

stevendanna
Copy link
Collaborator

@stevendanna stevendanna commented Feb 7, 2022

Eventually, we want to stop accepting non-MVCC AddSSTables. To do
this, we need all CREATE INDEX schema changes to use the new
mvcc-compatible process. To make this more tenable, we want to drain
in-flight schema changes during a migration.

Release note: Finalizing an upgrade to 22.2 requires that all
in-flight schema changes enter a terminal state. This may mean that
finalization takes as long as the longest-running schema change.

Release justification: Critical migration to ensure that 22.2's assumptions
about non-MVCC operations are true.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@stevendanna stevendanna force-pushed the wait-for-schema-changes-migration branch 2 times, most recently from ea28cff to 56b00b4 Compare February 9, 2022 12:21
@stevendanna stevendanna requested review from dt and ajwerner February 10, 2022 10:54
@stevendanna
Copy link
Collaborator Author

We'd likely want to wait on this until after #73878 and until we are ready to pull the trigger on only using the new AddSStable and new backfiller.

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: though I agree about waiting

Reviewed 10 of 12 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @dt and @stevendanna)


pkg/migration/migrations/wait_for_schema_changes.go, line 73 at r1 (raw file):

		id, ok := d.(*tree.DInt)
		if !ok {
			return errors.Newf("unexpected type for id column: %T (expected DInt)", d)

make these errors into assertion failures?

Code quote:

			return errors.Newf("unexpected number of columns: %d (expected 1)", len(datums))
		}
		d := datums[0]
		id, ok := d.(*tree.DInt)
		if !ok {
			return errors.Newf("unexpected type for id column: %T (expected DInt)", d)

pkg/migration/migrations/wait_for_schema_changes_test.go, line 258 at r1 (raw file):

				// TODO(ssd): Some tests may succeed just
				// because we haven't actually started the
				// migrations yet. The sleep is a poor way to

plumb a channel into a testing knob?

Code quote:

				// TODO(ssd): Some tests may succeed just
				// because we haven't actually started the
				// migrations yet. The sleep is a poor way to
				// help ensure the migration starts before the
				// schema change finishes.
				time.Sleep(10 * time.Millisecond)

@stevendanna stevendanna force-pushed the wait-for-schema-changes-migration branch 2 times, most recently from a84ac05 to db5b216 Compare February 11, 2022 12:11
Copy link
Collaborator Author

@stevendanna stevendanna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner and @dt)


pkg/migration/migrations/wait_for_schema_changes.go, line 73 at r1 (raw file):

Previously, ajwerner wrote…

make these errors into assertion failures?

Done.


pkg/migration/migrations/wait_for_schema_changes_test.go, line 258 at r1 (raw file):

Previously, ajwerner wrote…

plumb a channel into a testing knob?

Done. Along with some new interval knobs to speed up the test a bit.

@stevendanna stevendanna marked this pull request as ready for review February 14, 2022 00:17
@stevendanna stevendanna requested a review from a team February 14, 2022 00:17
@stevendanna stevendanna requested a review from a team as a code owner February 14, 2022 00:17
@shermanCRL shermanCRL requested review from samiskin and removed request for a team February 14, 2022 13:52
Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we going to do this? What would we get out of it?

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner, @dt, and @samiskin)

@stevendanna
Copy link
Collaborator Author

@ajwerner That's a good question. I wonder what @dt and @erikgrinaker think. IIRC, we wanted to do this to lower the impact of #76934. If we aren't going to do that this release, then it might be nice to not do this migration in this release and preserve the ability to flip back to the old index backfill code if we really need to.

@stevendanna stevendanna force-pushed the wait-for-schema-changes-migration branch from db5b216 to 2321ca3 Compare August 4, 2022 12:32
@stevendanna stevendanna requested review from a team as code owners August 4, 2022 12:32
@stevendanna stevendanna force-pushed the wait-for-schema-changes-migration branch 2 times, most recently from 5da6850 to c63993f Compare August 4, 2022 13:41
@stevendanna
Copy link
Collaborator Author

@dt @ajwerner I rebased this against master and also added a commit that removes the option for old-style backfills.

@stevendanna stevendanna force-pushed the wait-for-schema-changes-migration branch 2 times, most recently from 0dba059 to b2a2ea4 Compare August 15, 2022 10:29
@stevendanna stevendanna requested a review from a team August 15, 2022 10:29
@stevendanna stevendanna added the release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. label Aug 15, 2022
@stevendanna stevendanna force-pushed the wait-for-schema-changes-migration branch 8 times, most recently from 678e570 to cb60960 Compare August 19, 2022 23:55
@stevendanna
Copy link
Collaborator Author

bors r=dt

@stevendanna
Copy link
Collaborator Author

bors cancel

@craig
Copy link
Contributor

craig bot commented Aug 20, 2022

Canceled.

@stevendanna stevendanna force-pushed the wait-for-schema-changes-migration branch 2 times, most recently from 854875d to db847b1 Compare August 20, 2022 15:31
@stevendanna
Copy link
Collaborator Author

bors r=dt

@craig
Copy link
Contributor

craig bot commented Aug 20, 2022

Build failed:

Eventually, we want to stop accepting non-MVCC AddSSTables. To do
this, we need all CREATE INDEX schema changes to use the new
mvcc-compatible process. To make this more tenable, we want to drain
in-flight schema changes during a migration.

Release note (ops change): Finalizing an upgrade to 22.2 requires that
all in-flight schema changes enter a terminal state. This may mean
that finalization takes as long as the longest-running schema change.
This removes the ability to turn off MVCC-compliant index backfilling.

One downside here is that we lose the roachtest that uses the old
index backfiller method as a way to verify the new index
backfiller.

Release note (ops change): The option
`sql.mvcc_compliant_index_creation.enabled` has been removed.
@stevendanna stevendanna force-pushed the wait-for-schema-changes-migration branch from db847b1 to 2099121 Compare August 22, 2022 09:50
@stevendanna
Copy link
Collaborator Author

bors r=dt

@craig
Copy link
Contributor

craig bot commented Aug 22, 2022

Build failed:

@stevendanna
Copy link
Collaborator Author

bors retry

@craig
Copy link
Contributor

craig bot commented Aug 22, 2022

Build failed:

@stevendanna
Copy link
Collaborator Author

@craig
Copy link
Contributor

craig bot commented Aug 22, 2022

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-release-22.2 Used to mark GA and release blockers, technical advisories, and bugs for 22.2 release-blocker Indicates a release-blocker. Use with branch-release-2x.x label to denote which branch is blocked. T-disaster-recovery
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants