storage: ExportRequest poorly leveraged to clean up abandoned intents

[nvanbenschoten]

Currently, an ExportRequest will take an extremely long time to resolve a collection of abandoned intents. This can make it look like the ExportRequest is stuck somewhere in KV. The reason for this (or at least part of it) is that ExportMVCCToSst currently only returns a single intent if it returns a WriteIntentError. This is different than a request like ScanRequest, which will collect a set of intents in a WriteIntentError, which allows higher levels to process all of them at once.

We should improve ExportRequest to collect multiple intents in a single WriteIntentError.

We should also write a test that exercises the path where an ExportRequest needs to clean up millions of abandoned intents.

gz#7529

Epic: CRDB-2554

[tbg]

We should also write a test that exercises the path where an ExportRequest needs to clean up millions of abandoned intents.

here's a random 2018 flashback for you #18661

[tbg]

(Nothing there is useful, just posting for entertainment)

[aayushshah15]

The support issue that this popped up on saw intents being resolved at a rate of less than 10/sec, is that right?

I'm wondering if you could give me any intuition around why resolving these intents one-by-one is so egregiously slow, as I recall that the cluster in question had all its nodes in the same region (or even the same AZ IIRC).

Pardon my over-simplification, but aren't we just talking about issuing a bunch of QueryIntent ResolveIntent requests?

[nvanbenschoten]

My current working theory is that there is some quadratic behavior going on here. Each time, the ExportRequest begins scanning the range from the front and has to scan (while building SSTs) to the next abandoned intent. So we end up scanning the range num_abandoned_intents times. Collecting all intents in one go would avoid this.

[tbg]

Another option (on top of making intent handling more robust, which needs to happen anyway) could be a new request type (much like scan) which does not return any keys. Export could issue that ahead of its export requests to sanitize the keyspace. It is an extra step (which could be parallelized for much of the keyspace and wouldn't be very expensive with the separate lock table), but could further reduce the likelihood of the Export running into an intent in the first place.

[sumeerbhola]

My current working theory is that there is some quadratic behavior going on here.
...

Export could issue that ahead of its export requests to sanitize the keyspace. It is an extra step (which could be parallelized for much of the keyspace and wouldn't be very expensive with the separate lock table) ...

btw, this quadratic behavior was also discussed in #41720 (comment) (second bullet)

[nvanbenschoten]

To be concrete here, the work item will be updating MVCCIncrementalIterator to collect multiple intents in a single WriteIntentError, instead of just a single intent. This is similar to how the pebbleMVCCScanner works. This is on the border of Bulk-I/O and Storage, so I'd appreciate guidance on which direction to route this.

[lunevalex]

cc: @mwang1026 for consideration for 21.2

[tbg]

Fyi imo it would be shameful to let this sit until 21.2 given the amount of ongoing burden we think this creates. Can we try to fast track this?

…

On Wed, Feb 10, 2021, 18:18 lunevalex ***@***.***> wrote: cc: @mwang1026 <https://github.com/mwang1026> for consideration for 21.2 — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#59704 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGXPZH3RRE7KSR4576JH2DS6K5WDANCNFSM4W6ZOZBQ> .

[mwang1026]

I'd selfishly like to fast-track this as well since as far as I can tell doesn't have clear mitigation steps and also isn't easy for a customer to self-diagnose. Who could we assign this to / what would process be for fast tracking? Do we consider this a bug?

[aayushshah15]

Spoke to @nvanbenschoten offline, I'll take this on my plate.

[dt]

I think this is a dupe of #31762 but since it now has more discussion on it, maybe we close that one instead

[nvanbenschoten]

Something I found while exploring #60585 was that on release-20.2, we do not consult the finalizedTxnCache when req.WaitPolicy == lock.WaitPolicy_Error in lockTableWaiterImpl.WaitOn (here). I think we'll want to for two reasons:

1. it will avoid a transaction record push per abandoned intent.
2. it will allow us to defer and batch intent resolution like we do here.

So while addressing this issue, we'll want to make this change as well on release-20.2. We won't need it on master, as the finalizedTxnCache was pulled into the lockTable by @sumeerbhola in #57947.

Ideally, the testing we'll add when addressing this issue will be comprehensive enough to have caught this difference between master and release-20.2. For this to be true, we'll likely want at least two new tests:

1. an integration test that shows that BACKUP will efficiently clean up large swaths of abandoned intents
2. a new test scenario in pkg/kv/kvserver/concurrency/testdata/concurrency_manager/wait_policy_error, similar to the "request resolves the abandoned lock and proceeds" scenario, that shows that abandoned lock cleanup is deferred and batched.

[joshimhoff]

Yay to this issue. Can we backport the fix? What CRDB versions will the fix eventually land in? Us SREs are pretty motivated to roll this out fleet wide as early as possible.

Does this help with BACKUP in addition to bulk IO ops like IMPORT / EXPORT?

[nvanbenschoten]

Can we backport the fix?

Yes, we intend to backport this fix to v21.1, v20.2, and v20.1.

Does this help with BACKUP in addition to bulk IO ops like IMPORT / EXPORT?

Yep! This will help with any operation that uses ExportRequest, which includes BACKUP.

[joshimhoff]

Amazing.

[aliher1911]

It turned out that 20.1 is not as straightforward. We use rocksdb as a default storage and the fix lives in the glue code between storage engine and layer above so it only applies to pebble. So unless there's read need for that, we'd rather encourage people to upgrade. As for backporting change to pebble storage in 20.1 it is possible and the pr exists but not sure if we should proceed or not.

[nvanbenschoten]

As for backporting change to pebble storage in 20.1 it is possible and the pr exists but not sure if we should proceed or not.

I don't think we should bother with this. From the sound of it, we don't have any more scheduled v20.1 patch releases. We also don't expect many v20.1 clusters to be using Pebble.