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: after restore with skip_localities its not possible to change primary regions #103290

Closed
fqazi opened this issue May 15, 2023 · 0 comments · Fixed by #103362
Closed

sql: after restore with skip_localities its not possible to change primary regions #103290

fqazi opened this issue May 15, 2023 · 0 comments · Fixed by #103362
Assignees
@fqazi
Labels
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 May 15, 2023 •
edited by exalate-issue-sync bot
Loading

In 22.2 we added extra validation for region configs, which made (#100913) the skip_localities options problematic if the primary region of the database being restored no longer exists. While this option will correctly skip the validation of regions for databases, there is no real path for actually fixing the region anymore, since the same validation blocks alter commands. Our options are to either:

  1. Have restore completely wipe / re-configure the multi-region properties of a database
  2. Allow ALTER DATABASE SET PRIMARY REGION to work within a transaction to fix this issue
  3. Have a temporary bypass via a session variable to disable validation for this use case

Jira issue: CRDB-27925

Epic CRDB-27601
@fqazi fqazi added 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) labels May 15, 2023
@fqazi fqazi changed the title restore: after restore with skip_localities its not possible to change primary regions sql: after restore with skip_localities its not possible to change primary regions May 15, 2023
@exalate-issue-sync exalate-issue-sync bot changed the title sql: after restore with skip_localities its not possible to change primary regions restore: after restore with skip_localities its not possible to change primary regions May 15, 2023
@fqazi fqazi changed the title restore: after restore with skip_localities its not possible to change primary regions sql: after restore with skip_localities its not possible to change primary regions May 16, 2023
@fqazi fqazi mentioned this issue May 16, 2023
Merged
fqazi added a commit to fqazi/cockroach that referenced this issue May 16, 2023
@fqazi
sql: validate primary / secondary region localities at end of txn
3db9d6b 
Previously, if a database was restored with skip_localities,
there was no way to modify this database to set the primary
region since validation would kick in too early during the
statement. This meant fixing the regions in a restored database
was impossible if the primary region was no longer valid. To
address this, this patch, delays locality validation till the
end of the transaction.

Fixes: cockroachdb#103290

Release note (bug fix): SET PRIMARY REGION and SET SECONDARY REGION
did not validate transactionally, which could prevent cleaning up
removed regions after a restore.
fqazi added a commit to fqazi/cockroach that referenced this issue May 16, 2023
@fqazi
sql: validate primary / secondary region localities at end of txn
d3137d1 
Previously, if a database was restored with skip_localities,
there was no way to modify this database to set the primary
region since validation would kick in too early during the
statement. This meant fixing the regions in a restored database
was impossible if the primary region was no longer valid. To
address this, this patch, delays locality validation till the
end of the transaction.

Fixes: cockroachdb#103290

Release note (bug fix): SET PRIMARY REGION and SET SECONDARY REGION
did not validate transactionally, which could prevent cleaning up
removed regions after a restore.
craig bot pushed a commit that referenced this issue May 18, 2023
@nvanbenschoten @fqazi
@arulajmani @RaduBerinde
Merge #103265 #103362 #103373 #103538
fe71b7c 
103265: kv: resolve conflicting intents immediately for latency-sensitive requests r=nvanbenschoten a=nvanbenschoten

Fixes #50390.
Related to #60585.
Related to #103126.

Closes #64500, which was an earlier attempt to solve this issue using a similar approach. This commit addresses the comments on that PR, which focused on the pagination of intent resolution when bypassing the request batcher. We still don't try to solve this issue, and instead limit the cases where intent resolution bypasses the request batcher to those where pagination is not necessary.

This commit adds a new `sendImmediately` option to the `IntentResolver` `ResolveOptions`, which instructs the `IntentResolver` to send the provided intent resolution requests immediately, instead of adding them to a batch and waiting up to 10ms (`defaultIntentResolutionBatchWait`) for that batch to fill up with other intent resolution requests. This can be used to avoid any batching-induced latency and should be used only by foreground traffic that is willing to trade off some throughput for lower latency.

The commit then sets this flag for single-key intent resolution requests initiated by the `lockTableWaiter`. Unlike most calls into the `IntentResolver`, which are performed by background tasks that are more than happy to trade increased latency for increased throughput, the `lockTableWaiter` calls into the `IntentResolver` on behalf of a foreground operation. It wants intent resolution to complete as soon as possible, so it is willing to trade reduced throughput for reduced latency.

I tested this out by writing 10,000 different intents in a normal-priority transaction and then scanning over the table using a high-priority transaction. The test was performed on a 3-node roachprod cluster to demonstrate the effect with realistic network and disk latencies.
```sql
-- session 1
CREATE TABLE keys (k BIGINT NOT NULL PRIMARY KEY);
BEGIN; INSERT INTO keys SELECT generate_series(1, 10000);

-- session 2
BEGIN PRIORITY HIGH; SELECT count(*) FROM keys;
```

Without this change, the scan took **70.078s**. With this change, the scan took **15.958s**. This **78%** speed-up checks out. Each encountered intent is resolved serially (see #103126), so the **per-intent latency** drops from **7ms** to **1.6ms.** This improvement by about 5ms agrees with the `defaultIntentResolutionBatchIdle`, which delays each resolution request that passes through a RequestBatcher. With this change, these resolve intent requests are issued immediately and this delay is not experienced.

While this is a significant improvement and will be important for Read Committed transactions after #102014, this is still not quite enough to resolve #103126. For that, we need to batch the resolve intent requests themselves using a form of deferred intent resolution like we added in #49218 (for finalized transactions).

A similar improvement is seen for scans that encounter many abandoned intents from many different transactions. This scenario bypasses the deferred intent resolution path added in #49218, because the intents are each written by different transactions. The speedup for this scenario was presented in #64500.

Release note (performance improvement): SQL statements that must clean up intents from many different previously abandoned transactions now do so moderately more efficiently.


103362: sql: validate primary / secondary region localities at end of txn r=fqazi a=fqazi

Previously, if a database was restored with skip_localities, there was no way to modify this database to set the primary region since validation would kick in too early during the statement. This meant fixing the regions in a restored database was impossible if the primary region was no longer valid. To address this, this patch, delays locality validation till the end of the transaction.

Fixes: #103290

Release note (bug fix): SET PRIMARY REGION and SET SECONDARY REGION did not validate transactionally, which could prevent cleaning up removed regions after a restore.

103373: concurrency: small refactors in preparation for reservation removal r=nvanbenschoten a=arulajmani

See individual commits for details. 

Informs: #103361

103538: go.mod: bump Pebble to 6f2788660198, rework shared storage wrapper r=RaduBerinde a=RaduBerinde

6f278866 shared: improve interface for more efficient reading
9eb2c407 db: log events to testing.T in unit tests
f32e7dc6 db: add reserved Pebblev4 sstable format
5a6b91b8 objstorage: improve test and add read ahead test
2bc4319e objstorage: remove genericFileReadable
8143ffb9 objstorage: fix readaheadState initialization
06d08888 db: add Compact.Duration metric
e7213de0 db: add Uptime metric
e9005aed db: don't delete files during ingest application
222b43ec internal/arenaskl: fix Skiplist doc typo

Release note: None
Epic: none

Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: Faizan Qazi <[email protected]>
Co-authored-by: Arul Ajmani <[email protected]>
Co-authored-by: Radu Berinde <[email protected]>
@craig craig bot closed this as completed in 8b9f052 May 18, 2023
@blathers-crl blathers-crl bot mentioned this issue May 18, 2023
Merged
blathers-crl bot pushed a commit that referenced this issue May 18, 2023
@fqazi
sql: validate primary / secondary region localities at end of txn
91c13ee 
Previously, if a database was restored with skip_localities,
there was no way to modify this database to set the primary
region since validation would kick in too early during the
statement. This meant fixing the regions in a restored database
was impossible if the primary region was no longer valid. To
address this, this patch, delays locality validation till the
end of the transaction.

Fixes: #103290

Release note (bug fix): SET PRIMARY REGION and SET SECONDARY REGION
did not validate transactionally, which could prevent cleaning up
removed regions after a restore.
@fqazi fqazi mentioned this issue May 18, 2023
Merged
fqazi added a commit to fqazi/cockroach that referenced this issue May 19, 2023
@fqazi
sql: validate primary / secondary region localities at end of txn
f198329 
Previously, if a database was restored with skip_localities,
there was no way to modify this database to set the primary
region since validation would kick in too early during the
statement. This meant fixing the regions in a restored database
was impossible if the primary region was no longer valid. To
address this, this patch, delays locality validation till the
end of the transaction.

Fixes: cockroachdb#103290

Release note (bug fix): SET PRIMARY REGION and SET SECONDARY REGION
did not validate transactionally, which could prevent cleaning up
removed regions after a restore.
@rafiss rafiss mentioned this issue Sep 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@fqazi fqazi

Labels
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: validate primary / secondary region localities at end of txn fqazi/cockroach
1 participant
@fqazi