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

schemachanger: check explain diagrams during rollback test #85406

Merged
merged 1 commit into from
Aug 1, 2022

Conversation

postamar
Copy link
Contributor

@postamar postamar commented Aug 1, 2022

This commit enriches the declarative schema changer integration tests by
making data-driven EXPLAIN output assertions easier to add as
a complement to otherwise unrelated tests. In particular, this commit
improves the rollback test to check the explained rollback plan for each
post-commit revertible stage. This should make it easier to debug bad
rule definitions which otherwise would manifest themselves as causing
the schema change to hang during the rollback.

Release note: None

This commit enriches the declarative schema changer integration tests by
making data-driven EXPLAIN output assertions easier to add as
a complement to otherwise unrelated tests. In particular, this commit
improves the rollback test to check the explained rollback plan for each
post-commit revertible stage. This should make it easier to debug bad
rule definitions which otherwise would manifest themselves as causing
the schema change to hang during the rollback.

Release note: None
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@postamar
Copy link
Contributor Author

postamar commented Aug 1, 2022

I was trying to rewrite some rules and struggled to debug them without any clear idea of what the expected plan was at rollback time. We also need more testing hooks inside the rollback test to catch planning errors like circular graphs etc but that's out of scope for this already considerable change (in terms of test artifacts at least).

@postamar
Copy link
Contributor Author

postamar commented Aug 1, 2022

This is ready for review. CI failures seem to be unrelated flakes.

@postamar postamar marked this pull request as ready for review August 1, 2022 15:51
@postamar postamar requested a review from a team August 1, 2022 15:51
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: I had hoped for this as an outcome after we made it one directive per test. Thanks for making it happen.

Reviewed 4 of 183 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @postamar)


pkg/sql/schemachanger/sctest/end_to_end.go line 206 at r1 (raw file):

		_, err := file.Write(prefixBuf.Bytes())
		require.NoError(t, err)
		_, err = fmt.Fprintf(file, "EXPLAIN (%s) %s;\n----\n", tag, explainedStmt)

Something rubs me the wrong way about using SQL syntax in EXPLAIN (ddl, verbose) rollback at post-commit stage 1 of 7; but not enough to block this strictly positive PR. I think I'd prefer if this was somehow a SQL-level comment, but I don't care that much.

@postamar
Copy link
Contributor Author

postamar commented Aug 1, 2022

Thanks for the review!

Something rubs me the wrong way about using SQL syntax in EXPLAIN (ddl, verbose) rollback at post-commit stage 1 of 7; but not enough to block this strictly positive PR. I think I'd prefer if this was somehow a SQL-level comment, but I don't care that much.

I know, right? I don't like it either. I'm left with the feeling we're lacking some jobs abstractions here, something to trigger a job failure when a particular condition is met. A bit like pausepoints; "failpoints" perhaps?

@postamar
Copy link
Contributor Author

postamar commented Aug 1, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 1, 2022

Build succeeded:

@craig craig bot merged commit 314baa5 into cockroachdb:master Aug 1, 2022
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

Successfully merging this pull request may close these issues.

3 participants