Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

storage: write to AbortSpan less, clean up AbortSpan more #42765

Merged
merged 7 commits into from
Jan 2, 2020
Merged

storage: write to AbortSpan less, clean up AbortSpan more #42765

merged 7 commits into from
Jan 2, 2020

Commits on Jan 2, 2020

  1. storage/batcheval: rename truncate_log_test.go to cmd_truncate_log_te… 

    …st.go

This mirrors `cmd_truncate_log.go`.
    @nvanbenschoten
    nvanbenschoten committed Jan 2, 2020
    Configuration menu
    Copy the full SHA
    0958a6e View commit details
    Browse the repository at this point in the history

  2. storage/batcheval: test requests that modify the AbortSpan 

    Up until this point, we had no testing around this. For instance, not a single
test would catch it if we removed the deletion path from SetAbortSpan.
    @nvanbenschoten
    nvanbenschoten committed Jan 2, 2020
    Configuration menu
    Copy the full SHA
    853d087 View commit details
    Browse the repository at this point in the history

  3. storage/batcheval: clean up span on non-poisoning, aborting EndTransa… 

    …ction request

Fixes cockroachdb#29128.

Before this change, an EndTransaction request sent to rollback a transaction record
would not remove any AbortSpan entries, even if its own Poison flag was set to false.
This allowed AbortSpan entries to leak. This commit fixes this behavior by removing
the AbortSpan entry in this case.

There were concerns raised in cockroachdb#29128 about this being safe. It turns out that we already
do this on every Range except the transaction record's Range, so this isn't introducing
any new concerns.

Release note (bug fix): AbortSpan records are now cleaned up more
aggresively when doing so is known to be safe.
    @nvanbenschoten
    nvanbenschoten committed Jan 2, 2020
    Configuration menu
    Copy the full SHA
    1328787 View commit details
    Browse the repository at this point in the history

  4. storage: only write AbortSpan entries if intents are removed 

    This reduces the frequency of AbortSpan entries that can be abandoned even
without a transaction coordinator failure. Specifically, it protects against
the case where intent resolution races with a transaction coordinator cleaning
up its own transaction record and intents. This can happen for both aborted and
committed transactions.

In the first case, a pusher might find a transaction's intent and then find its
record to be aborted after that transaction had cleanly rolled back its own intents.
Even though the transaction's coordinator had already cleaned up and potentially
"unpoisoned" AbortSpans, the pusher would happily re-introduce AbortSpan records when
it goes to resolve the intents that were already cleaned up. These AbortSpan entries
would be fully abandoned and have to wait out the GC.

Similarly, in the second case, the transaction might have committed. Here, the
pushee might hit an intent and the txn coordinator might clean up and auto-GC its
txn record before the pushee arrives at the txn record. Once the pushee gets there,
it would mistake the txn for aborted (by design) and proceed to write an AbortSpan
record where the intent it had once observed had been (not by design).

We can tell both of these cases by simply recognizing whether intent resolution actually
succeeds. If intent resolution doesn't find an intent, then we might be in either
case. That's fine, because we only need to ever poison the abort span if we actually
remove an intent that could confuse a zombie transaction.

Release note: None
    @nvanbenschoten
    nvanbenschoten committed Jan 2, 2020
    Configuration menu
    Copy the full SHA
    2e8ad39 View commit details
    Browse the repository at this point in the history

  5. storage: assert that Poison=false for EndTxn(Commit=true) reqs 

    Return an error if not.

Release note: None
    @nvanbenschoten
    nvanbenschoten committed Jan 2, 2020
    Configuration menu
    Copy the full SHA
    fce02e7 View commit details
    Browse the repository at this point in the history

  6. storage/batcheval: rename SetAbortSpan to UpdateAbortSpan 

    Purely a rename.

Release note: None
    @nvanbenschoten
    nvanbenschoten committed Jan 2, 2020
    Configuration menu
    Copy the full SHA
    663eff0 View commit details
    Browse the repository at this point in the history

  7. storage/batcheval: remove impossible cases in TestUpdateAbortSpan 

    Release note: None
    @nvanbenschoten
    nvanbenschoten committed Jan 2, 2020
    Configuration menu
    Copy the full SHA
    c7bbc4f View commit details
    Browse the repository at this point in the history