-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
crosscluster/logical: check UDT equivalency during LDR creation #133194
Conversation
0619858
to
7b9c55a
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.
Amazing! Also, could you modify the commit message/release note to explain that this enables UDT support on the mode=immediate? I assume you're following up with another pr to fix #132164
dbA.Exec(t, "ALTER TABLE tab ADD COLUMN enum_col mytype NOT NULL") | ||
dbB.Exec(t, "ALTER TABLE b.tab ADD COLUMN enum_col b.mytype NOT NULL") | ||
dbA.ExpectErr(t, | ||
`cannot create logical replication stream: destination table tab column enum_col has user-defined type USER DEFINED ENUM: public.mytype`, | ||
`cannot create logical replication stream: .* destination type USER DEFINED ENUM: public.mytype has logical representations \[a b c\], but the source type USER DEFINED ENUM: mytype has \[a b\]`, |
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.
could you also add a test case for composite type mismatch?
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.
done
for i, desc := range spec.TypeDescriptors { | ||
sourceTypes[i] = &desc | ||
} | ||
// TODO(rafi): do we need a different type resolver? |
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.
if the user is using the sql path, we still need to fast fail here until #132164 is resolved.
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.
done
This check requires that the logical and physical representations of each type are identical. In the future, we may investigate ways to only require logical equivalency. Release note (ops change): When creating a logical replication stream, any user-defined types in the source and destination are now checked for equivalency. This allows for creating a stream that handles user-defined types without needing to use the `WITH SKIP SCHEMA CHECK` option as long as the replication stream uses `mode = immediate`.
7b9c55a
to
ea57988
Compare
tftr! bors r+ |
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error setting reviewers, but backport branch blathers/backport-release-24.3-133194 is ready: POST https://api.github.com/repos/cockroachdb/cockroach/pulls/133274/requested_reviewers: 422 Reviews may only be requested from collaborators. One or more of the teams you specified is not a collaborator of the cockroachdb/cockroach repository. [] Backport to branch 24.3.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
This check requires that the logical and physical representations of each type are identical. In the future, we may investigate ways to only require logical equivalency.
fixes #132167
Release note (ops change): When creating a logical replication stream,
any user-defined types in the source and destination are now checked for
equivalency. This allows for creating a stream that handles user-defined
types without needing to use the
WITH SKIP SCHEMA CHECK
option as longas the replication stream uses
mode = immediate
.