replication: Add mysql::serialization based Gtid Log Event #990

dveeden · 2025-02-14T15:07:47Z

Closes #845

With mysqlbinlog:

# at 158
#250213 14:55:59 server id 1  end_log_pos 241 CRC32 0x52293f34 
# Position  Timestamp   Type   Source ID        Size      Source Pos    Flags 
# 0000009e ef f9 ad 67   2a   01 00 00 00   53 00 00 00   f1 00 00 00   00 00
# 000000b1 02 78 00 00 02 02 25 02  dc f0 09 02 30 f9 03 22 |.x..........0...|
# 000000c1 bd 03 ad 02 21 02 44 44  5a 68 51 03 22 04 d9 03 |......DDZhQ.....|
# 000000d1 06 0c 61 61 62 62 63 63  08 00 0a 04 0c 7f cf de |..aabbcc........|
# 000000e1 ef 6a 06 2e 06 10 45 03  12 c3 02 0b 34 3f 29 52 |.j....E.....4..R|
#       GTID    last_committed=0        sequence_number=1       rbr_only=no     original_committed_timestamp=1739454959050447   immediate_commit_timestamp=1739454959050447 transaction_length=209
# original_commit_timestamp=1739454959050447 (2025-02-13 14:55:59.050447 CET)
# immediate_commit_timestamp=1739454959050447 (2025-02-13 14:55:59.050447 CET)
/*!80001 SET @@session.original_commit_timestamp=1739454959050447*//*!*/;
/*!80014 SET @@session.original_server_version=90200*//*!*/;
/*!80014 SET @@session.immediate_server_version=90200*//*!*/;
SET @@SESSION.GTID_NEXT= '896e7882-18fe-11ef-ab88-22222d34d411:aabbcc:123'/*!*/;

With go-mysqlbinlog (decoding a similar, but not identical event):

=== Gtid_tagged_log_event ===
Date: 2025-02-17 21:14:49
Log position: 240
Event size: 82
Commit flag: 1
GTID_NEXT: 896e7882-18fe-11ef-ab88-22222d34d411:foobaz:1
LAST_COMMITTED: 0
SEQUENCE_NUMBER: 1
Immediate commmit timestamp: 1739823289369365 (2025-02-17T21:14:49.369365+01:00)
Orignal commmit timestamp: 0 (<n/a>)
Transaction length: 210
Immediate server version: 90200
Orignal server version: 0

…vent_serialized

serialization/serialization.go

…vent_serialized

dveeden · 2025-02-21T06:24:05Z

@serprex if you're interested, maybe you could review this PR?

serprex · 2025-02-21T14:04:30Z

replication/event.go

+		return errors.New("failed to get transaction_length field")
+	}
+
+	// TODO: add and test commit_group_ticket


I haven't yet been able to generate any events with an actual commit_group_ticket.

I first tried with binlog_group_commit_sync_delay=... but that didn't make any difference.

Then looking at the code I noticed that this has to do with Group Replication (part of InnoDB Cluster), So I setup a cluster with 3 sandbox instances... but that also didn't give me any events for testing.

Looks like this might also be known as BGC (Binlog Group Commit) tickets.

serialization/serialization.go

Co-authored-by: Philip Dubé <[email protected]>

lance6716

still reviewing, I hope I can find some spare time soon

serialization/serialization.go

Copilot

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

replication/event.go:516

Consider handling the error returned by uuid.FromBytes(e.SID) instead of discarding it to ensure that invalid SID values are caught.

u, _ := uuid.FromBytes(e.SID)

replication/event.go:554

[nitpick] For consistency with GTIDEvent, consider renaming GtidTaggedLogEvent to GTIDTaggedLogEvent.

type GtidTaggedLogEvent struct {

serialization/serialization.go

lance6716 · 2025-02-22T14:33:14Z

serialization/serialization.go

+	return f.Value
+}
+
+func Unmarshal(data []byte, v interface{}) error {


it's better to split this function into two dedicated functions for Message and Format? I don't understand why they are merged. seems not shared many logic

If you compare it to json.Unmarshal() that also doesn't have different functions depending on the type (array, string, object, etc).
I think I like to keep it like this.

serialization/serialization.go

lance6716

rest lgtm

serialization/serialization.go

lance6716 · 2025-02-23T06:49:17Z

serialization/serialization.go

+			pos++
+			var n uint64
+			var err error
+			switch f := m.Fields[i].Type.(type) {


It's more like a golang-style to let every type (FieldIntFixed, FieldUintVar, ...) implement an interface with decode method. So this type-switch can be replaced by

n, err := m.Fields[i].Type.decode(data, pos)

This almost works now:

diff --git a/serialization/serialization.go b/serialization/serialization.go index 2b71d76..29ff5ed 100644 --- a/serialization/serialization.go +++ b/serialization/serialization.go @@ -69,6 +69,7 @@ type Field struct { // FieldType represents a `type_field` type FieldType interface { fmt.Stringer + decode(data []byte, pos uint64) (uint64, error) } // FieldIntFixed is for values with a fixed length. @@ -221,35 +222,9 @@ func Unmarshal(data []byte, v interface{}) error { } m.Fields[i].ID = int(data[pos] >> 1) pos++ - var n uint64 - var err error - switch f := m.Fields[i].Type.(type) { - case FieldIntFixed: - n, err = f.decode(data, pos) - if err != nil { - return err - } - m.Fields[i].Type = f - case FieldUintVar: - n, err = f.decode(data, pos) - if err != nil { - return err - } - m.Fields[i].Type = f - case FieldIntVar: - n, err = f.decode(data, pos) - if err != nil { - return err - } - m.Fields[i].Type = f - case FieldString: - n, err = f.decode(data, pos) - if err != nil { - return err - } - m.Fields[i].Type = f - default: - return fmt.Errorf("unsupported field type: %T", m.Fields[i].Type) + n, err := m.Fields[i].Type.decode(data, pos) + if err != nil { + return err } pos = n }

Gives me this:

serialization_test.go:238:12: cannot use FieldIntFixed{…} (value of type FieldIntFixed) as FieldType value in struct literal: FieldIntFixed does not implement FieldType (method decode has pointer receiver)

Oh I see that's because decode will change itself so it should has a pointer receiver. In other words *FieldIntFixed implements FieldType interface. So we should assign *FieldIntFixed like

diff --git i/replication/event.go w/replication/event.go index f2056e7..500587f 100644 --- i/replication/event.go +++ w/replication/event.go @@ -561,7 +561,7 @@ func (e *GtidTaggedLogEvent) Decode(data []byte) error { Fields: []serialization.Field{ { Name: "gtid_flags", - Type: serialization.FieldIntFixed{ + Type: &serialization.FieldIntFixed{ Length: 1, }, },

Do you it's acceptable? In this way we also don't need to assign back to Type field like line 237

This doesn't seem to work. I thried this: dveeden@7a58236

It flags f.Value = ... as unused write. Should we return it instead?

The switch also checks for valid field types, without the switch that's not done anymore.

I'll try to provide a valid diff today

replication/event.go

Co-authored-by: Copilot <[email protected]>

lance6716 · 2025-02-27T10:23:48Z

Hi @dveeden would you like me to directly modify your PR based on my comments?

dveeden · 2025-02-27T10:40:38Z

Hi @dveeden would you like me to directly modify your PR based on my comments?

I plan to apply those changes today. If more changes is needed after today you can either modify this PR directly or send a PR for my branch.

lance6716

rest lgtm

serialization/serialization.go

lance6716 · 2025-03-01T09:11:23Z

serialization/serialization.go

+			pos++
+			var n uint64
+			var err error
+			switch f := m.Fields[i].Type.(type) {


Oh I see that's because decode will change itself so it should has a pointer receiver. In other words *FieldIntFixed implements FieldType interface. So we should assign *FieldIntFixed like

diff --git i/replication/event.go w/replication/event.go index f2056e7..500587f 100644 --- i/replication/event.go +++ w/replication/event.go @@ -561,7 +561,7 @@ func (e *GtidTaggedLogEvent) Decode(data []byte) error { Fields: []serialization.Field{ { Name: "gtid_flags", - Type: serialization.FieldIntFixed{ + Type: &serialization.FieldIntFixed{ Length: 1, }, },

Do you it's acceptable? In this way we also don't need to assign back to Type field like line 237

Co-authored-by: lance6716 <[email protected]>

Signed-off-by: lance6716 <[email protected]>

replication: Add mysql::serialization based Gtid Log Event

3ea3d67

dveeden requested review from atercattus and lance6716 February 14, 2025 15:07

dveeden added 8 commits February 14, 2025 16:13

fixup

a3d5dee

fixup

10158f4

Merge remote-tracking branch 'upstream/master' into gtid_tagged_log_e…

6888c31

…vent_serialized

fixup

09838f5

Add test and get field by name

b6ceaf3

more testing

3bd47aa

More testing

0b445a1

fixup

b55f86e

lance6716 reviewed Feb 16, 2025

View reviewed changes

serialization/serialization.go Show resolved Hide resolved

serialization/serialization.go Show resolved Hide resolved

serialization/serialization.go Outdated Show resolved Hide resolved

serialization/serialization.go Outdated Show resolved Hide resolved

dveeden added 9 commits February 16, 2025 11:42

prepare for negative values

3028527

Update based on review

76b98bb

Merge remote-tracking branch 'upstream/master' into gtid_tagged_log_e…

990de1d

…vent_serialized

fixup

0237595

fixup

209cfad

fixup

ad8acac

fixup

f412b2b

updates

a7e4280

updates

b7b8f38

dveeden marked this pull request as ready for review February 18, 2025 13:24

dveeden added 2 commits February 18, 2025 14:28

fixup

28d3be8

More testing

4a8e497

serprex reviewed Feb 21, 2025

View reviewed changes

serialization/serialization.go Outdated Show resolved Hide resolved

serprex reviewed Feb 21, 2025

View reviewed changes

serialization/serialization.go Outdated Show resolved Hide resolved

serprex reviewed Feb 21, 2025

View reviewed changes

serialization/serialization.go Outdated Show resolved Hide resolved

serprex approved these changes Feb 21, 2025

View reviewed changes

dveeden and others added 5 commits February 21, 2025 16:10

Update serialization/serialization.go

c74b18e

Co-authored-by: Philip Dubé <[email protected]>

Merge branch 'master' into gtid_tagged_log_event_serialized

aa11671

fixup

86bf3fc

formatting

8d78a72

Update based on review

ba1dcc9

lance6716 reviewed Feb 22, 2025

View reviewed changes

serialization/serialization.go Outdated Show resolved Hide resolved

serialization/serialization.go Outdated Show resolved Hide resolved

lance6716 requested a review from Copilot February 22, 2025 14:26

Copilot AI reviewed Feb 22, 2025

View reviewed changes

serialization/serialization.go Outdated Show resolved Hide resolved

lance6716 reviewed Feb 22, 2025

View reviewed changes

lance6716 reviewed Feb 23, 2025

View reviewed changes

Update serialization/serialization.go

1558b91

Co-authored-by: Copilot <[email protected]>

dveeden added 9 commits February 27, 2025 11:40

Merge branch 'master' into gtid_tagged_log_event_serialized

da20b43

Don't store the message in the event struct

ab00980

use map to speedup lookup of fields

80d1d62

Use decode method

89b293f

Change Size and LastNonIgnoreableField to uint8

a45d879

Change field ID to uint8

085c631

Use stringParts as suggested in review

ac2d26f

fixup

3a1789b

Set original values to immediate values if skipped

14de33c

lance6716 reviewed Mar 1, 2025

View reviewed changes

dveeden and others added 6 commits March 1, 2025 10:54

Update serialization/serialization.go

173111f

Co-authored-by: lance6716 <[email protected]>

Update serialization/serialization.go

f129364

Co-authored-by: lance6716 <[email protected]>

test my suggestion

b7e35fe

Signed-off-by: lance6716 <[email protected]>

fix unit test

78157e3

Signed-off-by: lance6716 <[email protected]>

try to fix unit test again

9b61d61

Signed-off-by: lance6716 <[email protected]>

Merge branch 'master' into gtid_tagged_log_event_serialized

dfc7475

lance6716 merged commit f53dd22 into go-mysql-org:master Mar 3, 2025
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replication: Add mysql::serialization based Gtid Log Event #990

replication: Add mysql::serialization based Gtid Log Event #990

dveeden commented Feb 14, 2025 •

edited

Loading

dveeden commented Feb 21, 2025

serprex Feb 21, 2025

dveeden Feb 28, 2025

lance6716 left a comment

Copilot AI left a comment

lance6716 Feb 22, 2025

dveeden Feb 28, 2025

lance6716 left a comment

lance6716 Feb 23, 2025

dveeden Feb 27, 2025 •

edited

Loading

lance6716 Mar 1, 2025

dveeden Mar 2, 2025

lance6716 Mar 3, 2025

lance6716 commented Feb 27, 2025

dveeden commented Feb 27, 2025

lance6716 left a comment

lance6716 Mar 1, 2025

replication: Add mysql::serialization based Gtid Log Event #990

replication: Add mysql::serialization based Gtid Log Event #990

Conversation

dveeden commented Feb 14, 2025 • edited Loading

dveeden commented Feb 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance6716 left a comment

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance6716 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dveeden Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lance6716 commented Feb 27, 2025

dveeden commented Feb 27, 2025

lance6716 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dveeden commented Feb 14, 2025 •

edited

Loading

dveeden Feb 27, 2025 •

edited

Loading