Skip to content

Conversation

@morgo
Copy link
Collaborator

@morgo morgo commented Oct 6, 2025

A Pull Request should be associated with an Issue.

We wish to have discussions in Issues. A single issue may be targeted by multiple PRs.
If you're offering a new feature or fixing anything, we'd like to know beforehand in Issues,
and potentially we'll be able to point development in a particular direction.
Further notes in https://github.com/block/spirit/blob/main/.github/CONTRIBUTING.md

Fixes #471
Fixes #464
Fixes #461 (also fixed in #462 but I couldn't wait for this to merge).

@morgo morgo force-pushed the mtocker-boundary-checking branch from ffec118 to d37ad7f Compare October 6, 2025 18:21
@morgo morgo requested a review from kolbe October 9, 2025 20:31
// Test VARBINARY types
binaryData := []byte{0x01, 0x02, 0x03}
result := EscapeMySQLType("VARBINARY(255)", binaryData)
assert.Contains(t, result, "_binary")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this redundant with the following assertion?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, what this does is checks that the collation hint is specified ahead of the string. So it doesn't use the connection's properties.


for {
select {
case notification := <-ddlNotifications:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess if there's some other test watching for notifications, this will consume the notification and make the other test fail?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not the case. The replication client sends all DDL changes to the notification channel. Because the binary logs are shared, there is a race where it may occasionally get the DDL of another test. But the channels are not shared.

An alternative fix is that there could be filtering in the replication client to only send notifications for tables for which there are subscriptions. I didn't do this originally, but I could switch to it in future.

@morgo morgo enabled auto-merge October 10, 2025 00:54
@morgo morgo mentioned this pull request Oct 10, 2025
@morgo morgo force-pushed the mtocker-boundary-checking branch from 91be8c7 to 407403f Compare October 10, 2025 01:09
@morgo morgo merged commit be82214 into block:main Oct 10, 2025
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants