search

cockroachdb / cockroach

CockroachDB — the cloud native, distributed SQL database designed for high availability, effortless scale, and control over data placement.
https://www.cockroachlabs.com
Other
30.15k stars 3.81k forks source link

sql/kv: split system and tenant "high-priority" levels #71946

Open nvanbenschoten opened 3 years ago

nvanbenschoten commented 3 years ago

Currently, both the system tenant and secondary tenants use the same MaxTxnPriority level for high-priority transactions. This means that a system-tenant transaction running with high priority is not guaranteed to never block on a secondary-tenant transaction. This is a property that we would like to have for cluster-wide BACKUP, CDC, etc. in multi-tenant clusters.

To allow us to make the claim that a secondary-tenant transaction can never block (at least some subset of) system-tenant transactions, we should separate MaxTxnPriority into a system level and a tenant level. The system-level max priority should take precedence over the tenant-level max priority.

This is not quite as simple as defining a MaxTenantTxnPriority = MaxTxnPriority - 1 and calling it a day. That's because there are a few places where MaxTxnPriority is given special treatment, such as in the txnWaitQueue, in the lockTableWaiter, and in the evaluation logic for PushTxn requests.

SQL transactions running with "high priority" (BEGIN PRIORITY HIGH) should be given a different priority depending on whether they are part of the system tenant or not. The KV host cluster should reject all BatchRequests from secondary tenants with the system-tenant's max priority level.

Jira issue: CRDB-10844

rytaft commented 2 years ago

@nvanbenschoten you mentioned that you thought there might be a short term fix that could alleviate this issue? Would be great to get your thoughts on exactly what's needed for 22.1.

nvanbenschoten commented 2 years ago

Yes, I was thinking something like https://github.com/cockroachdb/cockroach/pull/77435 could alleviate this issue in the short term. It changes the transaction precedence rules for high-priority tenant transactions, but I doubt anyone will notice. I still need to do some research into how much of this we document and confirm that no internal transactions rely on these rules before declaring this a feasible short-term solution.

ajstorm commented 2 years ago

@nvanbenschoten I know that we punted on this for 22.1. Based on your research, what's your assessment on the urgency of this for 22.2?

nvanbenschoten commented 1 year ago

Based on your research, what's your assessment on the urgency of this for 22.2?

At the time, we weren't sure about the urgency of this. Since then, we've seen this come up in at least one support case. So it is something that we need to resolve, but I don't know that the solution proposed in this issue is the right one. Interestingly, we avoid this issue entirely if reads don't block on in-progress writes.