-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
release-19.1: changefeed schema change nemeses and fixes #43037
release-19.1: changefeed schema change nemeses and fixes #43037
Conversation
Here's the last one! Tests fail without the other PRs so those have to merge first. This didn't flake in an hour of Nemeses/cloudstorage testing on roachprod-stress. |
Currently, the changefeed nemeses system does not support schema changes. This has previously allowed some fairly critical bugs to creep past our testing. This PR adds support for schema change events to the changefeed nemeses. Release note: None.
Currently, the changefeed poller detects a scan boundary when it detects that the last version of a table descriptor has a pending mutation but the current version doesn't. In case of an `ALTER TABLE DROP COLUMN` statement, the point at which this happens is the point at which the schema change backfill completes. However, this is incorrect since the dropped column gets logically dropped before that. This PR corrects this problem by instead using the set of column descriptors within the current and previous table descriptors to detect a scan boundary. Fixes cockroachdb#41961 Release note (bug fix): Changefeeds now emit backfill row updates for dropped column when the table descriptor drops that column.
90e9a67
to
b8c34bf
Compare
b8c34bf
to
265a2b7
Compare
Hey @ajwerner looks like the 19.1 release is out. Mind taking a look at this so I can page it out? |
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.
Seems like you added some eg.go
files by accident.
Reviewed 4 of 5 files at r1, 6 of 7 files at r2, 2 of 35 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15, @ajwerner, and @danhhz)
pkg/ccl/changefeedccl/cdctest/nemeses.go, line 531 at r1 (raw file):
} txn, err := ns.db.Begin()
Would it make you sad to also backport #43215? If so we can do that separately
pkg/col/coldata/vec.eg.go, line 2 at r3 (raw file):
// Code generated by execgen; DO NOT EDIT. // Copyright 2018 The Cockroach Authors.
This shouldn't be here right?
The cloud storage sink uses the original filename format, so unlike release-19.2, we also need to pad schemaID. Release note: None
265a2b7
to
da56d30
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.
Fixed the eg.go files.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aayushshah15 and @danhhz)
pkg/ccl/changefeedccl/cdctest/nemeses.go, line 531 at r1 (raw file):
Previously, ajwerner wrote…
Would it make you sad to also backport #43215? If so we can do that separately
Definitely separately
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 2 of 35 files at r3, 31 of 32 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @aayushshah15 and @danhhz)
TFTR! |
Backport:
Please see individual PRs for details.
/cc @cockroachdb/release