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

sql/catalog/lease: Avoid cross descriptor validation when acquiring leases #95764

Closed
fqazi opened this issue Jan 24, 2023 · 5 comments · Fixed by #97630
Closed

sql/catalog/lease: Avoid cross descriptor validation when acquiring leases #95764

fqazi opened this issue Jan 24, 2023 · 5 comments · Fixed by #97630
Assignees
@fqazi
Labels
A-schema-leasing Pertains to leasing descriptors in the catalog. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@fqazi
Copy link
Collaborator

fqazi commented Jan 24, 2023 •
edited by cockroach-jira-scripts
Loading

As part of the lease acquisition process, we utilize high-priority transactions, which can cause cross-descriptor validation to potentially push out other work such as schema changes. In a schema where a single table has tons of foreign key references, this validation on a sizeable multi-region cluster could lead to a fairly high frequency of such events. Cross-descriptor validation is only strictly needed at modification time, and our approach of aggressively doing it for even reads can have a determinantal impact in certain cases by blocking schema changes (on lease renewals).

Jira issue: CRDB-23721
@fqazi fqazi added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-schema-deprecated Use T-sql-foundations instead labels Jan 24, 2023
@fqazi fqazi self-assigned this Jan 24, 2023
@ajwerner
Copy link
Contributor

I think for the code we have today, this is reasonable.

Cross-reference validation doesn't buy us much other than awareness of existing corruption at a time you don't really want to know about it.

However, there might be value in cross-reference validation if we used it to determine minimum versions of referenced descriptors and used that to bootstrap cross-descriptor coherence in our caching. That's a bigger topic for a document I have brewing.

@ajwerner ajwerner added the A-schema-leasing Pertains to leasing descriptors in the catalog. label Jan 31, 2023
@ajwerner
Copy link
Contributor

@fqazi if you haven't actively done anything here, I think we ought to deprioritize this. There's questions about the mechanism. In light of recent discoveries that this was not the root cause of the motivating customer issue, I'm not sure how much this matters.

@fqazi
Copy link
Collaborator Author

fqazi commented Jan 31, 2023

Nothing has been actively done on this issue, we can deprioritize this one. The other thing is if cross-descriptor validation is important during leasing we could also move to a sampled model, we don't have to do this stuff on every renewal.

@ajwerner ajwerner mentioned this issue Feb 8, 2023
Open
@ajwerner ajwerner mentioned this issue Feb 21, 2023
Closed
@fqazi fqazi mentioned this issue Feb 24, 2023
Merged
fqazi added a commit to fqazi/cockroach that referenced this issue Feb 24, 2023
@fqazi
sql/lease: support skipping descriptor validation lease renewals
3877697 
Fixes: cockroachdb#95764

Previously, when renewing a lease we would fetch dependent
descriptors for cross-descriptor validation. This could
lead to perpetual transaction retries, in the schema where
there are tons of dependencies between descriptors since
lease renewals would happen in a high-priority transaction.
Additionally, this incurs additional latency for lease renewal.
To address this, this patch disables cross-validation by default for
lease renewals.

Epic: none
Release note (bug fix): Disable cross-descriptor validation on lease renewal, which can lead to transactions getting pushed during schema changes on a multi-region cluster.
@fqazi fqazi mentioned this issue Feb 24, 2023
Merged
craig bot pushed a commit that referenced this issue Feb 24, 2023
@fqazi
Merge #97630
0a60831 
97630: sql/lease: support skipping descriptor validation lease renewals r=fqazi a=fqazi

Fixes: #95764

Previously, when renewing a lease we would fetch dependent
descriptors for cross-descriptor validation. This could
lead to perpetual transaction retries, in the schema where
there are tons of dependencies between descriptors since
lease renewals would happen in a high-priority transaction.
Additionally, this incurs additional latency for lease renewal.
To address this, this patch disables cross-validation by default for
lease renewals.

Epic: none
Release note (bug fix): Disable cross-descriptor validation on lease renewal,
which can be problematic when there are lots of descriptors with lots
of foreign key references, in which cases, the cross-reference validation could
starve schema changes.

Co-authored-by: Faizan Qazi <[email protected]>
@fqazi fqazi mentioned this issue Feb 24, 2023
Merged
fqazi added a commit to fqazi/cockroach that referenced this issue Feb 24, 2023
@fqazi
sql/lease: support skipping validation lease renewals
1e2edd8 
Fixes: cockroachdb#95764

Previously, when renewing a lease we would fetch dependent
descriptors for cross-descriptor validation. This could
lead to perpetual transaction retries, in the schema where
there are tons of dependencies between descriptors since
lease renewals would happen in a high-priority transaction.
Additionally, this incurs additional latency for lease renewal.
To address this, this patch disables cross-validation by default for
lease renewals.

Epic: none
Release note (bug fix): Add support for disabling cross-descriptor validation
on lease renewal, which can be problematic when there are lots of descriptors
with lots of foreign key references, in which cases, the cross-reference
validation could starve schema changes. This can be enabled with
sql.catalog.descriptor_lease_renewal_cross_validation.
@craig craig bot closed this as completed in 063cbfe Feb 24, 2023
@fqazi fqazi reopened this Feb 24, 2023
@fqazi
Copy link
Collaborator Author

fqazi commented Feb 24, 2023

Will close this once the backports are merged.

fqazi added a commit to fqazi/cockroach that referenced this issue Feb 24, 2023
@fqazi
Fixes: cockroachdb#95764
9b9d7d4 
Previously, when renewing a lease we would fetch dependent
descriptors for cross-descriptor validation. This could
lead to perpetual transaction retries, in the schema where
there are tons of dependencies between descriptors since
lease renewals would happen in a high-priority transaction.
Additionally, this incurs additional latency for lease renewal.
To address this, this patch disables cross-validation by default for
lease renewals.

Epic: none
Release note (bug fix): Add support for disabling cross-descriptor validation
on lease renewal, which can be problematic when there are lots of descriptors
with lots of foreign key references, in which cases, the cross-reference
validation could starve schema changes. This can be enabled with
sql.catalog.descriptor_lease_renewal_cross_validation.
@fqazi
Copy link
Collaborator Author

fqazi commented Feb 24, 2023

All backports are merged

@fqazi fqazi closed this as completed Feb 24, 2023
fqazi added a commit to fqazi/cockroach that referenced this issue Feb 24, 2023
@fqazi
Fixes: cockroachdb#95764
2053874 
Previously, when renewing a lease we would fetch dependent
descriptors for cross-descriptor validation. This could
lead to perpetual transaction retries, in the schema where
there are tons of dependencies between descriptors since
lease renewals would happen in a high-priority transaction.
Additionally, this incurs additional latency for lease renewal.
To address this, this patch disables cross-validation by default for
lease renewals.

Epic: none
Release note (bug fix): Add support for disabling cross-descriptor validation
on lease renewal, which can be problematic when there are lots of descriptors
with lots of foreign key references, in which cases, the cross-reference
validation could starve schema changes. This can be enabled with
sql.catalog.descriptor_lease_renewal_cross_validation.
fqazi added a commit to fqazi/cockroach that referenced this issue Mar 6, 2023
@fqazi
release-22.2: disable cross descriptor validation during lease renewal
5c6ae46 
Previously, we added support for disabling descriptor lease
validation during lease renewal to avoid regressions
due to the overhead on multi-region clusters, where in some
cases schema change transactions would hit retry errors.
This was inadequate because the default still exposed users
to a regression in behaviour. To address this, this patch will
disable cross-descriptor validation by default during lease renewal.

Informs: cockroachdb#95764

Release note: None
@fqazi fqazi mentioned this issue Mar 6, 2023
Merged
@fqazi fqazi mentioned this issue Mar 6, 2023
Merged
fqazi added a commit to fqazi/cockroach that referenced this issue Mar 6, 2023
@fqazi
release-22.2: disable cross descriptor validation during lease renewal
7630ada 
Previously, we added support for disabling descriptor lease
validation during lease renewal to avoid regressions
due to the overhead on multi-region clusters, where in some
cases schema change transactions would hit retry errors.
This was inadequate because the default still exposed users
to a regression in behaviour. To address this, this patch will
disable cross-descriptor validation by default during lease renewal.

Informs: cockroachdb#95764

Release note: None
@exalate-issue-sync exalate-issue-sync bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) and removed T-sql-schema-deprecated Use T-sql-foundations instead labels May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@fqazi fqazi

Labels
A-schema-leasing Pertains to leasing descriptors in the catalog. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sql/lease: support skipping descriptor validation lease renewals fqazi/cockroach
2 participants
@fqazi @ajwerner