Find Errant GTIDs

[PrismaPhonic]

This PR adds a function called FindErrantGTIDs which finds the errant GTIDs for a given replica if all other replica slave status' are supplied as well.

This includes:

1. A Difference method on Mysql56GTIDSet which can be used to find the difference between the receiver GTIDSet, and the supplied GTIDSet.
2. A FindErrantGTIDs method on SlaveStatus which can tell us the receivers errant GTIDs, when we've supplied a comprehensive list of all known SlaveStatus for replicas of the same shard. I choose to make this a method on SlaveStatus rather than a standalone function because I believe it makes it more clear that we are finding the errant GTIDs of the receiver SlaveStatus.

Related Issue: #6206

[shlomi-noach]

Hi @PrismaPhonic! Saw your request for preliminary reviews for this draft.
Thought I'd share a bit of insight since I similarly developed errant GTID detection in orchestrator (work spread over openark/orchestrator#607, openark/orchestrator#617, openark/orchestrator#707 and probably more).

I apologize in advance that this turns a bit lengthy.

1. First thing to consider what the reference (base, or other) GTID is and what it means.

• Naively, that would be the master of the replica being examined. Compare GTID with the immediate master and get the errant value.
  This unfortunately is only half correct, since the immediate master may in itself be a replica. The immediate master may itself have errant GTID, thereby making all of its own replicas (including the one we're looking at) have errant GTIDs. But as compared with its direct master, maybe our replica does not have an errant GTID. It is a matter of definition to declare whether our replica does or does not have errant GTID. That may depend on what operations we wish to achieve. Some refactoring operations involving our replica may make the problem worse, others won't.
• Compare with the "real" master of the topology? This is more correct, because the master is the source of truth. But then, we may want to understand where the errant transaction originated. On our replica? Or, on its immediate or any of its ancestry masters? Back to previous clause.

Whichever we decide, this leads us to the question of "when and how" we collected the executed_gtid_set for our replica and for the base.
Say we first sample the base. For simplicity, let's assume it's the direct master. If we happen to first sample the master and then the replica, then it's possible that in between our sampling, new transactions will have been applied on the master and propagated to the replica. In which case, by the time we read the GTID set from the replica, it makes the appearance of having an errant transaction, although in reality nothing is wrong.

This is why in evaluating the diff, we must disregard any part of the GTID set that contains the UUID of the replica's master or any of its ancestors. It is safe to skip a UUID that belongs to an ancestor because there is no way for our replica to have a transaction with such UUID that is errant. The transaction must have originated from an ancestor, therefore it is applied on the ancestor.

So all this lengthy preface is to note that other GTIDSet should come with context, and by comparing the difference of two GTID sets does not imply an errant transaction.

And all of the above may be premature, since current code merely runs a Difference method; but I thought I'd throw this as heads up.

2. Asking out of complete ignorance. In orchestrator my choice was to not evaluate the difference in code, but instead delegate it to MySQL by executing GTID_SUBTRACT(). The line of thought was that if I wanted to run an operation on some replica and was able to read its executed_gtid_set, then I'd also have access to run GTID_SUBTRACT(). I'm merely wondering if the same line of thought can be applied here and if it at all makes sense to contact MySQL to evaluate the diff. Obviously, the downside is the need to connect to a MySQL server.

[PrismaPhonic]

Hi @shlomi-noach!

Thank you very much for your in depth reply. Since it's pretty late over here I'll give a brief reply for now and try to expand tomorrow if necessary.

1. This is just a pure difference function. The intention of FindErrantGTIDs function (to be added next) is to throw out GTIDs with the UUID of the the current master, which can be determined for each replica via slave status. I'm curious how orchestrator knows definitively who all valid ancestors are?
2. We could potentially defer to GTID_SUBTRACT() however, that involves more calls to MySQL. We already have to deal with potential failure of not getting back some slave statuses. This would involve a number of calls to GTID_SUBTRACT() for each replica, and that dependence feels like a more fragile approach to me. As for executed_gtid_set, we are actually planning to do something a bit different in this case. We actually are going to take a union of the retrieved_gtid_set and the executed_gtid_set (because retrieved_gtid_set can be cleared for a number of reasons outside of our control), to ensure that we are making the best choice possible when considering which replica to failover to. It's that union that we ideally want to check for errant gtids. I'm guessing that wouldn't make much of a difference in terms of calling GTID_SUBTRACT.

Thanks for the great reply! If you have some time for a video chat later this week, or early next week I would love to pick your brain more about this.

[shlomi-noach]

The intention of FindErrantGTIDs function (to be added next) is to throw out GTIDs with the UUID of the the current master, which can be determined for each replica via slave status.

That makes sense. Also, in the context of a failover, I guess you will only be looking at 1st tier replicas? That simplifies things.

I'm curious how orchestrator knows definitively who all valid ancestors are?

That's basically what orchestrator does first: map the topology and be able to answer questions about relations (e.g. it can print out the tree, or you can inquire about parent/children).

that dependence feels like a more fragile approach to me.

Makes sense.

take a union of the retrieved_gtid_set and the executed_gtid_set

Sounds good! One thing to note is whether you necessarily intend to wait for the replica to consume its relay logs (hence, its retrieved_gtid_set). Users of orchestrator have represented 4 different and contradicting approaches: wait forever; wait until timeout then fail; wait until timeout then promote; promote immediate regardless of dataloss. I'm not sure whether vitess needs to be opinionated or not.

[systay]

if this is something that is used in the hot path, and will contain more than just a few items, you might want to consider using make with a given size, to avoid resizing hash maps

[PrismaPhonic]

I was thinking about that, but we don't know yet what the size of the differenceSet will be. Are you thinking that we should just make it the same size as the receiver to cover all potential SIDs we might find that have valid diffs?

[systay]

⭐ Really nice code, well commented code

[shlomi-noach]

The rewrite looks good!

[shlomi-noach]

Can you please add the following two test cases:

1. Where the result range is a single value or single-value range (e.g. {1, 7} - {2, 6})
2. Where the result range is empty (e.g. {2, 10}, {20-30} - {1, 40})

[PrismaPhonic]

2 is technically covered by {20, 30} - {20, 30} although that's a very trivial case. I'll include yours. Great suggestions!

[PrismaPhonic]

Added both, and pushed up. Tests still passing :-)

[deepthi]

Concur with @systay, nicely written and well-commented.

[deepthi]

It might be possible to replace advance and advanceOther with a single func and passing the relevant []interval to it (and having it return the 0th element for later use).

[PrismaPhonic]

I had wanted to do that at first. The reason why I choose to use two different closures is that what we do for advance and advanceOther is different. In the case that we advance we always want to push a new interval onto diffIntervals. In the case that we advanceOther we do not. Unless I'm missing something, I don't think these can be merged into a single helper method.

[deepthi]

That is why I suggested returning the 0th element. As of now you always call advance with intervals and advanceOther with otherIntervals. Depending on which one you are operating on, you can do different things.
For instance, the first time you use a common advance func could look like this:

		if i, ok := advance(intervals); ok {
			diffIntervals = append(diffIntervals, i)
		}
		else {
			continue
		}
		if _, ok := advance(otherIntervals); !ok {
			differenceSet[sid] = intervals
			continue
		}

I'm not going to insist on this. If you feel the current version is more readable, that is fine.

[PrismaPhonic]

Ahhhh I see what you're saying. Yes, we could do it this way - although then we would have this kind of code in every switch branch. To me it reads cleaner to contain this logic into advance and advanceOther but this is certainly another way we could do it - and not worse. Likely just a matter of personal preference. Thanks for the feedback!

[PrismaPhonic]

Thanks for all of the great feedback everyone! I've finished addressing all existing feedback.