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

Fix bug in handling sub events of replication.TransactionPayloadEvent #875

Merged

Conversation

froot
Copy link
Contributor

@froot froot commented May 8, 2024

When using the canal package with a MySQL instance with binlog-transaction-compression=ON, the following error occurs when you execute an INSERT statement on a table:

[2024/05/07 17:30:01] [info] sync.go:237 table structure changed, clear table cache: test_database.employees
panic: interface conversion: replication.Event is *replication.QueryEvent, not *replication.RowsEvent

goroutine 1 [running]:
github.com/go-mysql-org/go-mysql/canal.(*Canal).handleRowsEvent(0x140002ee0e0?, 0x1400049a180?)
	/Users/lulusheng/go/pkg/mod/github.com/go-mysql-org/go-mysql@v1.7.1-0.20240507101235-8d4de0fa6687/canal/sync.go:253 +0x2a4
github.com/go-mysql-org/go-mysql/canal.(*Canal).runSyncBinlog(0x140002ee0e0)
	/Users/lulusheng/go/pkg/mod/github.com/go-mysql-org/go-mysql@v1.7.1-0.20240507101235-8d4de0fa6687/canal/sync.go:107 +0x9fc
github.com/go-mysql-org/go-mysql/canal.(*Canal).run(0x140002ee0e0)
	/Users/lulusheng/go/pkg/mod/github.com/go-mysql-org/go-mysql@v1.7.1-0.20240507101235-8d4de0fa6687/canal/canal.go:239 +0xdc
github.com/go-mysql-org/go-mysql/canal.(*Canal).Run(...)
	/Users/lulusheng/go/pkg/mod/github.com/go-mysql-org/go-mysql@v1.7.1-0.20240507101235-8d4de0fa6687/canal/canal.go:194
main.main()
	/Users/lulusheng/Desktop/mysql-binlog-hacking/main.go:42 +0x194

This is because the runSyncBinlog function assumes that the sub events of a replication.TransactionPayloadEvent event are all row events. This isn't true (you can see an example of this in this PR comment #773 (comment)). This PR addresses this issue by making the event handling logic its own method and calling it recursively from the TransactionPayloadEvent case.

This PR is easier to review if you hide the whitespace.

@froot froot marked this pull request as ready for review May 8, 2024 19:57
@lance6716
Copy link
Collaborator

lgtm.

I noticed in #773 (comment) the header of subevents are not the same as a normal binlog events, and EventHandler interface will pass that header to the caller. Does your application want more change? like filling the log position for subevents.

@froot
Copy link
Contributor Author

froot commented May 9, 2024

Yeah I noticed the Log position: 0. For my application, we don't use that field so it doesn't affect us.

It seems like mysqlbinlog will output the end position of the Transaction_payload_event for all the sub events:

# at 886
#240508 14:41:28 server id 2  end_log_pos 1094 CRC32 0x12e4f78b         Transaction_Payload             payload_size=175       compression_type=ZSTD   uncompressed_size=235
# Start of compressed events.
# at 1094
#240508 14:41:28 server id 2  end_log_pos 1094 CRC32 0x66d1b0d9         Query   thread_id=8     exec_time=0     error_code=0
SET TIMESTAMP=1715193688/*!*/;
BEGIN
/*!*/;
# at 1094
#240508 14:41:28 server id 2  end_log_pos 1094 CRC32 0x14acda3c         Table_map: `test_database`.`employees` mapped to number 92
# has_generated_invisible_primary_key=0
# at 1094
#240508 14:41:28 server id 2  end_log_pos 1094 CRC32 0xae348986         Write_rows: table id 92 flags: STMT_END_F
[...]

We could probably mimic the same approach when creating the subevents of a transaction payload event. Anyways, I don't think it's necessary for this PR in particular. Are you good to approve @lance6716 ?

Also, I noticed that there hasn't been a new release in a while. How difficult would it be to cut a new release soon?

@lance6716
Copy link
Collaborator

@atercattus Hi, do you have time to release a new version? In fact I'm not familiar with the procedure.

@lance6716 lance6716 merged commit e35272c into go-mysql-org:master May 10, 2024
13 checks passed
@atercattus
Copy link
Member

Of course. Should have done this a long time ago :)

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.

None yet

3 participants