Allow to synchronise GTIDs 'OnPosSynced'

[bejelith]

We want to keep track of the GTID when a transaction has ended or a table has been updated

[taylorchu]

btw, need to run gofmt.

@siddontang @bejelith After running this in production for over a year, I think the concept of "forced" saving in OnXXXSynced is often not useful. Because we often have different requirements when to persist position metadata.

#379

We use SyncedPosition and SyncedGTIDSet to get the gtid or binlog pos that is "processed", and save it with custom position metadata saver periodically.

[bejelith]

@taylorchu on #379 i agree with you, but it all depends if u process the statements/rows synchronously or in batches... my idea is to execute the updates in the backend in a transactional way (and synchronise my own way) so that i dont endup in half applied transactions at the destination.

The problem i have now is that in the OnSync event (end of a binlog transaction) it's impossible to retrieve the GTIDSet.
My initial idea was to change the paradigm of OnPosSynced from
OnPosSynced(p mysql.Position, f bool) to OnPosSynced(p mysql.Position, set mysql.GTIDSet, f bool)
but would have broken backward compatibility.

[siddontang]

Thanks @bejelith

My initial idea was to change the paradigm of OnPosSynced from
OnPosSynced(p mysql.Position, f bool) to OnPosSynced(p mysql.Position, set mysql.GTIDSet, f bool)

I prefer this way even to break the compatibility. But we can provide another better name.

@taylorchu
does this change work for you?

[siddontang]

PTAL @GregoryIan

[bejelith]

Also a problem of the single hook with both mysql.Position and mysql.GTIDSet is what the latter can be nil for some transactions (when the Canal is started with mysql.Position) and it would be up to the handler checking for nil condition.

i think OnPosSynced is quite self explanatory, an alternative could be OnTransactionEnd?

[bejelith]

@siddontang I changed OnPosSynced, we should be good to go now.. unless u have another name to suggest

[siddontang]

Thanks @bejelith

I guess your change is not conflicted with #379, right? if yes, I want to merge both of these two PRs.

[IANTHEREAL]

do we need it in somewhere except for L67?

[bejelith]

Didnt fully cleanup the previously implemented test case, doing it now

[IANTHEREAL]

anywhere to use it?

[bejelith]

Didnt fully cleanup the previously implemented test case, doing it now

[amyangfei]

it seems GTID has race condition in canal package, but I can't reproduce in my local environment

[bejelith]

Looks like it's sporadic

[bejelith]

@siddontang this change is compatible with #379, i think is better to merge #379 first and eventually i can work on the eventual conflicts.

[IANTHEREAL]

LGTM now, Thanks @bejelith

[IANTHEREAL]

@siddontang PTAL