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

Give RowsEvent a method to inspect the underlying event type. #1016

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

proton-lisandro-pin
Copy link

@proton-lisandro-pin proton-lisandro-pin commented Mar 20, 2025

It's currently impossible to figure out what kind of operation led to a RowsEvent 😞
Multiple results indicate an UPDATE, but there's no way to differentiate an INSERT from a DELETE.

This PR implements a new method to determine the event type, regardless of version and/or engine.

It's currently impossible to figure out what kind of operation led to a
`RowsEvent`; multiple results indicate an `UPDATE`, but there's no way to
differentiate an `INSERT` from a `DELETE`.
Copy link
Collaborator

@lance6716 lance6716 left a comment

Choose a reason for hiding this comment

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

nit: It's better to add a separate type RowsEventType? Because I think the new methods of this PR can be used like

if e.IsInsert() {
  ...
} else if e.IsUpdate {
 ...
}
...

e.eventType will be checked for many times.

@@ -1120,6 +1120,18 @@ func (e *RowsEvent) Decode(data []byte) error {
return e.DecodeData(pos, data)
}

func (e *RowsEvent) IsInsert() bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes currently RowsEvent has no public type information. You can know the type from EventHeader

Copy link
Author

@proton-lisandro-pin proton-lisandro-pin Mar 21, 2025

Choose a reason for hiding this comment

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

Indeed. The problem is mostly that once casted to RowsEvent, it is impossible to inspect an event type - this unnecessarily complicates code needing to handle rows events based on operation type.
It also means consumers need to deal with raw event types (f.ex. UPDATE_ROWS_EVENTv01 vs UPDATE_ROWS_EVENTv2) which should be kept an implementation detail.

The type data is available thou, we just need to cleanly make it public.

@dveeden
Copy link
Collaborator

dveeden commented Mar 21, 2025

Does this work correctly with compressed binlog events in MySQL?

@proton-lisandro-pin
Copy link
Author

nit: It's better to add a separate type RowsEventType? Because I think the new methods of this PR can be used like

Sure 😄 I was mostly following the existing style for Is*Column() methods for TableMapEvents, but i do like introducing a dedicated type better. Fixed.

@proton-lisandro-pin
Copy link
Author

Does this work correctly with compressed binlog events in MySQL?

Not sure tbh. The change covers all binlog versions currently handled by the module, so if compressed binlogs are already supported, then yes.

@proton-lisandro-pin proton-lisandro-pin changed the title Give RowsEvent methods to inspect the underlying event type. Give RowsEvent a method to inspect the underlying event type. Mar 21, 2025
@dveeden
Copy link
Collaborator

dveeden commented Mar 21, 2025

Does this work correctly with compressed binlog events in MySQL?

Not sure tbh. The change covers all binlog versions currently handled by the module, so if compressed binlogs are already supported, then yes.

These are in TransactionPayloadEvent, Not sure if and how that's handled. Might be good to test.

@proton-lisandro-pin
Copy link
Author

These are in TransactionPayloadEvent, Not sure if and how that's handled. Might be good to test.

Ah, those a dedicated event type and won't be affected by this change then.

@lance6716
Copy link
Collaborator

For TransactionPayloadEvent, I see that it still holds sub-events like normal BinlogEvent

type TransactionPayloadEvent struct {
format FormatDescriptionEvent
Size uint64
UncompressedSize uint64
CompressionType uint64
Payload []byte
Events []*BinlogEvent

And the events are decoded like it's in a normal code path
pe, err := parser.Parse(data)
if err != nil {
return err
}
e.Events = append(e.Events, pe)

The problem is we didn't have a unit test to verify it, like capturing the wire data and decoding the TransactionPayloadEvent. I'll consider add such unit test later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants