Skip to content

GC Txn entries through GC queue#3185

Merged
tbg merged 2 commits intocockroachdb:masterfrom
tbg:txn_seq_gc
Nov 24, 2015
Merged

GC Txn entries through GC queue#3185
tbg merged 2 commits intocockroachdb:masterfrom
tbg:txn_seq_gc

Conversation

@tbg
Copy link
Copy Markdown
Member

@tbg tbg commented Nov 21, 2015

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.

Review on Reviewable

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Copy Markdown
Member Author

tbg commented Nov 23, 2015

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
Copy link
Copy Markdown
Contributor

LGTM

tbg added 2 commits November 24, 2015 09:46
see cockroachdb#2062.

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.
tbg added a commit that referenced this pull request Nov 24, 2015
GC Txn entries through GC queue
@tbg tbg merged commit c882ab0 into cockroachdb:master Nov 24, 2015
@tbg tbg deleted the txn_seq_gc branch November 24, 2015 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GC of Transaction table

3 participants