GC Txn entries through GC queue

[tbg]

see #2062. This is a bit of a late Friday throw-over-the-wall but should generally be ready for review.

on each run of the GC queue for a given range, the transaction
and sequence prefixes are scanned and the following actions taken:

• old pending transactions are pushed (which will succeed), effectively
  aborting them
• old aborted transactions are added to the GC request.
• aborted and committed transactions will have the intents referenced
  in their record resolved synchronously and are GCed (on success)
• sequence cache entries which are "old" and belong to "old" (or
  nonexistent) transactions are deleted.

The implementation here isn't as performant as it could be by a long shot,
but very few txn/sequence cache entries should actually persist long enough
to be observed by this queue. Normally, transaction records are eagerly
removed after commit along with their sequence cache entries (much like
intents are usually resolved).

fixes #2062.

[bdarnell]

Does this change imply that you've seen this error in practice now? In general we shouldn't both log and return an error; just pass them up the stack and log them we we can no longer return the error.

[tbg]

updated, PTAL

---

Review status: 0 of 16 files reviewed at latest revision, 5 unresolved discussions.

---

storage/gc_queue.go, line 54 [r1] (raw file):
No, these two can go either way and the current values are arbitrary. It makes some sense to have txnCleanupThreshold lower, though: In many cases, the intents will be linked to by the transaction record, in which case the GC of the transaction record will always reach out to resolve the intents (so the "later" intent GC wouldn't even have to look at those, since they'd already have been resolved).

---

storage/gc_queue.go, line 68 [r1] (raw file):
We don't get that info from MVCCStats, want me to file an issue to add that there? Scanning the engine is probably too expensive to do in-place (?).

---

storage/gc_queue.go, line 322 [r1] (raw file):
I think it's beneficial to deal with the sequence cache last since resolving intents removes a lot of those entries already. But I could pass in the txnMap as an argument so that the logic here could simply not push transactions which we've already pushed successfully.

---

storage/gc_queue_test.go, line 390 [r1] (raw file):
even worse, it's also a possible source of data races where it is now. Fixed.

---

storage/replica.go, line 1586 [r1] (raw file):
no, I haven't, removed the panic to deal with the TODO. I'll remove the log line.

---

Comments from the review on Reviewable.io

[bdarnell]

LGTM