-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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: improve error screening #56379
workload/schemachange: improve error screening #56379
Conversation
5c687e0
to
f8b23c8
Compare
f8b23c8
to
6da2c61
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just little things
Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 1 of 1 files at r4, 2 of 2 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, 3 of 3 files at r8.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @jayshrivastava)
pkg/workload/schemachange/error_screening.go, line 228 at r8 (raw file):
previousRows := map[string]bool{} for _, row := range rows {
this logic could be refactored to be more readable. Consider factoring out a helper per row, constraint tuple. I also wonder if you could rework this to be one query per constraint.
pkg/workload/schemachange/operation_generator.go, line 361 at r4 (raw file):
} ifNotExists := og.randIntn(2) == 0
nit: propagate some configuration of this rate lest we forget that we have control?
pkg/workload/schemachange/operation_generator.go, line 1283 at r4 (raw file):
} // randTable returns a sequence qualified by a schema
nit: randSequence
pkg/workload/schemachange/operation_generator.go, line 381 at r5 (raw file):
// Randomly decide if the sequence should be owned by a column. If so, it can // set set using the tree.SeqOptOwnedBy sequence option. if og.randIntn(2) == 0 {
same nit about control.
pkg/workload/schemachange/operation_generator.go, line 1030 at r6 (raw file):
} srcSequenceExists, err := sequenceExists(tx, srcSequenceName)
I wonder if we're going to want to eventually put a transaction scoped cache on this sort of information. Just food for thought.
Release note: None
Release note: None
The intention here is to return the table name which was fetched from the previous query. Previously, a new, unique name was being returned. Release note: None
6da2c61
to
153605e
Compare
153605e
to
bd086d8
Compare
Previously sequences would only be attached to the public schema by default. This commit enhances the workload by allowing sequences to belong to other schemas. Furthermore, this commit adds the possibility that a sequence can be owned by a column in a table which belongs to a different schema. Release note: None
Release note: None
Release note: None
Release note: None
Release note: None
Release note: None
bd086d8
to
868a286
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r17, 1 of 1 files at r18.
Reviewable status: complete! 1 of 0 LGTMs obtained
bors r=ajwerner |
Build succeeded: |
workload/schemachange: improve error screening
Several ops will be updated to screen for errors (#56119):
Sequences were also updated to have more interesting cases: non-existing sequences can randomly be returned and sequences can be owned by columns.
Furthermore, this change fixes a bug where non-existing tables were getting returned instead of existing ones.
Finally, error screening logic is updated to ignore transaction retry errors. This fixes one of the issues in #56230.