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

workload/schemachange: "fix" workload #59635

Merged
merged 20 commits into from
Mar 16, 2021

Conversation

ajwerner
Copy link
Contributor

@ajwerner ajwerner commented Feb 1, 2021

This is a variety of commits which fixes bugs in the schemachange workload. It also disables a number of features.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ajwerner ajwerner force-pushed the ajwerner/fix-schemachange-workload branch from ceeb249 to 0566f92 Compare March 1, 2021 17:31
@ajwerner ajwerner force-pushed the ajwerner/fix-schemachange-workload branch 2 times, most recently from 93f3b50 to f2a2fb6 Compare March 10, 2021 07:42
@ajwerner ajwerner changed the title [DNM][WIP] workload/schemachange: "fix" workload workload/schemachange: "fix" workload Mar 10, 2021
@ajwerner ajwerner marked this pull request as ready for review March 10, 2021 07:43
@ajwerner
Copy link
Contributor Author

I spun this up under stress on 256 cores: 7290 runs so far, 0 failures, over 20m0s. I feel reasonably good about it given some of the things it turned up extremely quickly when they existed. There's more features to turn back on but this is a good start.

@ajwerner ajwerner force-pushed the ajwerner/fix-schemachange-workload branch from f2a2fb6 to 44fb031 Compare March 10, 2021 15:49
@ajwerner
Copy link
Contributor Author

@fqazi I have a feeling this may interest you. Might be worth stepping back and reading the code for this thing. It's powerful and finds bugs but could surely be more usable and extensible.

@ajwerner ajwerner force-pushed the ajwerner/fix-schemachange-workload branch from 44fb031 to 3a5ec65 Compare March 11, 2021 05:34
Copy link
Collaborator

@fqazi fqazi left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 9 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @postamar)

Copy link
Collaborator

@arulajmani arulajmani left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 9 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @postamar)


pkg/workload/schemachange/operation_generator.go, line 207 at r1 (raw file):

	setColumnNotNull:        1,
	setColumnType:           1,
	insertRow:               0,

Disallowing inserts will inhibit our ability to test ALTER TYPE ... DROP VALUE right? Ideally we'd want to seed tables that use user defined types with some data.

Is the complication here mostly around computed columns overflowing (like you mention in a comment somewhere below)? If so, would it make sense to block inserts if the table contains computed columns further down instead? I'm not sure if this is even feasible, as I couldn't find with what probability we generate computed columns, and maybe thats crippling in itself.


pkg/workload/schemachange/operation_generator.go, line 796 at r1 (raw file):

			// Filter out the generated virtual columns.
			// TODO(ajwerner): Remove this after #61768 is fixed.

Looks like the issue linked is closed, so you could get rid of this thing.


pkg/workload/schemachange/operation_generator.go, line 1733 at r1 (raw file):

	rows := [][]string{}
	for _, col := range cols {
		if col.generated {

nit: you could get rid of this as this is always false here.

ajwerner and others added 15 commits March 16, 2021 00:49
Before this we would end up swallowing errors needed by the higher layers
like transaction restarts.

Release note: None
We used to expect `DROP TABLE ... RESTRICT` to fail when the table was
involved in an FK relation. It's fine.

Release note: None
We don't allow dropping it in this case.

Release note: None
Before this commit we'd only look at the previous operation, not all
previous operations. This became a problem when validation was added.

Release note: None
Transaction restart errors must be rolled all the way back. Not doing that
means the workload totally fails. Also, we want to treat uncategorized errors
in generation as a real problem.

Release note: None
Could cause an error which was previously being swallowed.

Release note: None
Fixed by cockroachdb#59565.

Release justification: Testing only.

Release note: None
ajwerner and others added 5 commits March 16, 2021 00:49
We currently return an error indicating that this is unsupported.

Release justification: non-production code changes

Release note: None
These all should be re-enabled but they current have problems.

Release note: None
@ajwerner ajwerner force-pushed the ajwerner/fix-schemachange-workload branch from 3a5ec65 to 4f62dba Compare March 16, 2021 04:54
Copy link
Contributor Author

@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.

TFTR!

bors r=arulajmani

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @fqazi, and @postamar)


pkg/workload/schemachange/operation_generator.go, line 207 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Disallowing inserts will inhibit our ability to test ALTER TYPE ... DROP VALUE right? Ideally we'd want to seed tables that use user defined types with some data.

Is the complication here mostly around computed columns overflowing (like you mention in a comment somewhere below)? If so, would it make sense to block inserts if the table contains computed columns further down instead? I'm not sure if this is even feasible, as I couldn't find with what probability we generate computed columns, and maybe thats crippling in itself.

We'll need to fix this. IIRC there were a few different issues. I wanted to stabilize this test before going back to features as this PR wasn't tiny as it is. The computed column overflow was the first one I was hitting.

Insert row also has the problem that there are combinations of schema changes which can fail permanently, generally involving drop column. I think we'll need to add a rule that says that we don't do drop column in a transaction that has schema changes which might fail. D


pkg/workload/schemachange/operation_generator.go, line 796 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Looks like the issue linked is closed, so you could get rid of this thing.

Done.


pkg/workload/schemachange/operation_generator.go, line 1733 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: you could get rid of this as this is always false here.

Done.

@craig
Copy link
Contributor

craig bot commented Mar 16, 2021

Build succeeded:

@craig craig bot merged commit db4f939 into cockroachdb:master Mar 16, 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

Successfully merging this pull request may close these issues.

4 participants