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.

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

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

Open
nvanbenschoten opened this issue Oct 25, 2021 · 4 comments
Open

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

nvanbenschoten opened this issue Oct 25, 2021 · 4 comments
Assignees
Labels
A-multitenancy Related to multi-tenancy C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) E-starter Might be suitable for a starter project for new employees or team members. T-kv KV Team T-sql-queries SQL Queries Team

Comments

@nvanbenschoten
Copy link
Member

nvanbenschoten commented Oct 25, 2021

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

@nvanbenschoten nvanbenschoten added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-multitenancy Related to multi-tenancy labels Oct 25, 2021
@blathers-crl blathers-crl bot added T-kv KV Team T-sql-queries SQL Queries Team labels Oct 25, 2021
@nvanbenschoten nvanbenschoten added the E-starter Might be suitable for a starter project for new employees or team members. label Oct 29, 2021
@rytaft
Copy link
Collaborator

rytaft commented Mar 6, 2022

@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 added a commit to nvanbenschoten/cockroach that referenced this issue Mar 7, 2022
…ant pushee

Short-term mitigation for cockroachdb#71946.

Release justification: resolves concern for multi-tenancy.
@nvanbenschoten
Copy link
Member Author

Yes, I was thinking something like #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.

@nvanbenschoten nvanbenschoten self-assigned this Mar 9, 2022
@ajstorm
Copy link
Collaborator

ajstorm commented Jun 1, 2022

@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?

@exalate-issue-sync exalate-issue-sync bot removed the T-sql-queries SQL Queries Team label Jan 10, 2023
@nvanbenschoten
Copy link
Member Author

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.

@mgartner mgartner moved this to Backlog (DO NOT ADD NEW ISSUES) in SQL Queries Jul 24, 2023
@mgartner mgartner moved this from Backlog (DO NOT ADD NEW ISSUES) to New Backlog in SQL Queries Feb 1, 2024
@yuzefovich yuzefovich added the T-sql-queries SQL Queries Team label May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-multitenancy Related to multi-tenancy C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) E-starter Might be suitable for a starter project for new employees or team members. T-kv KV Team T-sql-queries SQL Queries Team
Projects
Status: Backlog
Development

No branches or pull requests

5 participants