sql: acquire replicated locks on explicit SELECT FOR UPDATE #100194

nvanbenschoten · 2023-03-30T19:13:39Z

Once we implement replicated key-level locks (#100193), we should then switch explicit SELECT FOR {SHARE/UPDATE} over to acquire locks with a Replicated durability.

Implicit uses of SELECT FOR UPDATE as part of mutation statements can continue to acquire cheaper, best-effort unreplicated locks.

Jira issue: CRDB-26582

Epic CRDB-25322

The locks created by a SELECT FOR UPDATE command are unreplicated and fragile if the leaseholder for the associated range is moved. This change creates an explicit lease for the destination schema to work around the limitation. cockroachdb/cockroach#100194

Temporarily disallow `SELECT FOR UPDATE` statements under all isolation levels that are not `SERIALIZABLE` (i.e. `SNAPSHOT` and `READ COMMITTED`). We will allow them again when the following issues are fixed: - cockroachdb#57031 - cockroachdb#75457 - cockroachdb#100193 - cockroachdb#100194 Fixes: cockroachdb#100144 Release note: None

103734: opt: disallow SELECT FOR UPDATE under weak isolation levels r=nvanbenschoten,mgartner a=michae2 **querycache: remove unused field from CachedData** Remove the `IsCorrelated` flag from `querycache.CachedData`, which is no longer used. Release note: None --- **sql/opt: add locking durability** In addition to strength and wait policy, we now add a third property to locks: durability. Locks with `LockDurabilityGuaranteed` are guaranteed to be held until commit (if the transaction commits). Durable locks must be used when correctness depends on locking. This is never the case under our `SERIALIZABLE` isolation, but under `SNAPSHOT` and `READ COMMITTED` isolation it will be the case for `SELECT FOR UPDATE` statements, which will be the first users of durable locks. This commit adds locking durability to the optimizer and `EXPLAIN` output, but does not plumb it into KV yet. It will be used in the next commit to temporarily disallow `SELECT FOR UPDATE` statements under `SNAPSHOT` and `READ COMMITTED` isolation. Release note: None --- **opt: disallow SELECT FOR UPDATE under weak isolation levels** Temporarily disallow `SELECT FOR UPDATE` statements under all isolation levels that are not `SERIALIZABLE` (i.e. `SNAPSHOT` and `READ COMMITTED`). We will allow them again when the following issues are fixed: - #57031 - #75457 - #100193 - #100194 Fixes: #100144 Release note: None Co-authored-by: Michael Erickson <michae2@cockroachlabs.com>

michae2 · 2023-06-29T16:17:26Z

This is pretty much done on the SQL side, just need to plumb the durability / replication flag into Scan requests once KV supports it.

Because the "guaranteed-durable locking not yet implemented" error condition is checked in execbuilder, it prevents not only execution but also `EXPLAIN` of queries using guaranteed-durable locking. Thankfully `EXPLAIN (OPT)` bypasses execbuilder, and hence still works, so use this for now to verify that we are enabling durable locking for `SELECT FOR UPDATE` under read committed isolation. (Note that we have not yet fixed the `SELECT FOR UPDATE` plans to use more precise locking, that will come in a later PR.) Informs: cockroachdb#100194 Release note: None

105857: sql: add implicit SELECT FOR SHARE locking to FK checks r=DrewKimball,nvanbenschoten,rytaft,mgartner a=michae2 **explain: add transaction information to EXPLAIN ANALYZE** Add transaction isolation, priority, and quality-of-service to the output of `EXPLAIN ANALYZE`. Release note (sql change): `EXPLAIN ANALYZE` output now includes: - the isolation level of the statement's transaction - the priority of the statement's transaction - the quality of service level of the statement's transaction --- **opt: do not use LockDurabilityGuaranteed under serializable isolation** This is a follow-up from #103734. We do not want to use guaranteed-durable (a.k.a. replicated) locking under serializable isolation, because it prevents pipelining and other optimizations, and is unnecessary for correctness. This commit amends 8cbc6d1 to only set durability for `SELECT FOR UPDATE` locking under weaker isolation levels. This means that query plans will be slightly different under different isolation levels, and so we must add isolation level to the optimizer memo staleness calculation. Furthermore, this commit changes the error message added by e633d5e to be about guaranteed-durable locking rather than `SELECT FOR UPDATE`, because in a later commit this specific error will also be triggered by foreign key checks under weaker isolation levels. Informs: #100144, #100156, #100193, #100194 Release note: None --- **opt: show locking durability in EXPLAIN (OPT) output** Because the "guaranteed-durable locking not yet implemented" error condition is checked in execbuilder, it prevents not only execution but also `EXPLAIN` of queries using guaranteed-durable locking. Thankfully `EXPLAIN (OPT)` bypasses execbuilder, and hence still works, so use this for now to verify that we are enabling durable locking for `SELECT FOR UPDATE` under read committed isolation. (Note that we have not yet fixed the `SELECT FOR UPDATE` plans to use more precise locking, that will come in a later PR.) Informs: #100194 Release note: None --- **sql: add implicit SELECT FOR SHARE locking to FK parent checks** Add SELECT FOR SHARE locking to FK parent checks. Under serializable isolation, this locking is only used when `enable_implicit_fk_locking_for_serializable` is set. Under weaker isolation levels (snapshot and read committed) this locking is always used. We only need to lock during the insertion-side FK checks, which verify the existence of a parent row. Deletion-side FK checks verify the non-existence of a child row, and these do not need to lock. Instead, to prevent concurrent inserts or updates to the child that would violate the FK constraint, we rely on the intent(s) created by the deletion conflicting with the FK locking of those concurrent inserts or updates. Fixes: #80683 Informs: #100156 Epic: CRDB-25322 Release note (sql change): Add a new session variable, `enable_implicit_fk_locking_for_serializable`, which controls locking during foreign key checks under serializable isolation. With this set to true, foreign key checks of the referenced (parent) table, such as those performed during an INSERT or UPDATE of the referencing (child) table, will lock the referenced row using SELECT FOR SHARE locking. (This is somewhat analogous to the existing `enable_implicit_select_for_update` variable but applies to the foreign key checks of a mutation statement instead of the initial row fetch.) Under weaker isolation levels such as read committed, SELECT FOR SHARE locking will always be used to ensure the database maintains the foreign key constraint, regardless of the current setting of `enable_implicit_fk_locking_for_serializable`. 107212: ui-e2e-tests: steps to enable cypress tests r=maryliag a=rickystewart This doesn't get the job fully working yet, but it's an improvement. Epic: none Part of #106584 Release note: None 107517: roachtest: add read committed variants of ycsb r=michae2 a=nvanbenschoten Closes #107112. This PR adds the following six roachtest variants: ``` ycsb/A/nodes=3/cpu=32/isolation-level=read-committed ycsb/B/nodes=3/cpu=32/isolation-level=read-committed ycsb/C/nodes=3/cpu=32/isolation-level=read-committed ycsb/D/nodes=3/cpu=32/isolation-level=read-committed ycsb/E/nodes=3/cpu=32/isolation-level=read-committed ycsb/F/nodes=3/cpu=32/isolation-level=read-committed ``` It does so after adding an `--isolation-level` flag to ycsb, which controls the isolation level to run the workload transactions under. If unset, the workload will run with the default isolation level of the database. Release note: None 107636: schemachanger: deflake TestConcurrentDeclarativeSchemaChanges r=postamar a=postamar This commit deflakes this test by checking that the second schema change actually does block because of the first one, rather than checking that it has blocked. The bug was that the latter wasn't always guaranteed to happen because we didn't force the schema changes to run in parallel. Fixes #106732. Release note: None Co-authored-by: Michael Erickson <michae2@cockroachlabs.com> Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com> Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com> Co-authored-by: Marius Posta <marius@cockroachlabs.com>

Follow-up from cockroachdb#105857 This commit ammends 6a3e43d to add a session variable to control whether guaranteed-durable locks are used under serializable isolation. Informs: cockroachdb#100194 Epic: CRDB-25322 Release note (sql change): Add a new session variable, `enable_durable_locking_for_serializable`, which controls locking durability under serializable isolation. With this set to true, SELECT FOR UPDATE locks, SELECT FOR SHARED locks, and constraint check locks (e.g. locks acquired during foreign key checks if `enable_implicit_fk_locking_for_serializable` is set to true) will be guaranteed-durable under serializable isolation, meaning they will always be held to transaction commit. (These locks are always guaranteed-durable under weaker isolation levels.) By default, under serializable isolation these locks are best-effort rather than guaranteed-durable, meaning in some cases (e.g. leaseholder transfer, node loss, etc.) they could be released before transaction commit. Serializable isolation does not rely on locking for correctness, only using it to improve performance under contention, so this default is a deliberate choice to avoid the performance overhead of lock replication.

…107752 #107802 #107803 106508: util/must: add runtime assertion API r=erikgrinaker a=erikgrinaker For details and usage examples, see the [package documentation](https://github.com/erikgrinaker/cockroach/blob/must/pkg/util/must/must.go). --- This patch adds a convenient and canonical API for runtime assertions, inspired by the Testify package used for Go test assertions. It is intended to encourage liberal use of runtime assertions throughout the code base, by making it as easy as possible to write assertions that follow best practices. It does not attempt to reinvent the wheel, but instead builds on existing infrastructure. Assertion failures are fatal in all non-release builds, including roachprod clusters and roachtests, to ensure they will be noticed. In release builds, they instead log the failure and report it to Sentry (if enabled), and return an assertion error to the caller for propagation. This avoids excessive disruption in production environments, where an assertion failure is often scoped to an individual RPC request, transaction, or range, and crashing the node can turn a minor problem into a full-blown outage. It is still possible to kill the node when appropriate via `log.Fatalf`, but this should be the exception rather than the norm. It also supports expensive assertions that must be compiled out of normal dev/test/release builds for performance reasons. These are instead enabled in special test builds. This is intended to be used instead of other existing assertion mechanisms, which have various shortcomings: * `log.Fatalf`: kills the node even in release builds, which can cause severe disruption over often minor issues. * `errors.AssertionFailedf`: only suitable when we have an error return path, does not fatal in non-release builds, and are not always notified in release builds. * `logcrash.ReportOrPanic`: panics rather than fatals, which can leave the node limping along. Requires the caller to implement separate assertion handling in release builds, which is easy to forget. Also requires propagating cluster settings, which aren't always available. * `buildutil.CrdbTestBuild`: only enabled in Go tests, not roachtests, roachprod clusters, or production clusters. * `util.RaceEnabled`: only enabled in race builds. Expensive assertions should be possible to run without the additional overhead of the race detector. For more details and examples, see the `must` package documentation. Resolves #94986. Epic: none Release note: None 107094: streamingest: unskip TestTenantStreamingUnavailableStreamAddress r=lidorcarmel a=lidorcarmel Changing a few things to get this test to pass under stress: - use 50 ranges instead of 10, because there are already 50-ish system ranges, so if we write only 10 more ranges those might not get distributed on all servers. - avoid reading from the source cluster after stopping a node, it's flaky, see #107499 for more info. Epic: none Fixes: #107023 Fixes: #106865 Release note: None 107717: server/profiler: remove `server.cpu_profile.enabled` setting r=xinhaoz a=xinhaoz Cpu profiling can be enabled by setting the cluster setting `server.cpu_profile.cpu_usage_combined_threshold`. This makes `server.cpu_profile.enabled` redundant and makes it more difficult and confusing to enable cpu profiling. This commit removes the `server.cpu_profile.enabled` setting entirely. Note that both jdefault values for the cluster settings set profiling off. Closes: #102024 Release note (sql change): The cluster setting `server.cpu_profile.enabled` has been removed. `server.cpu_profile.cpu_usage_combined_threshold` can enable and disable cpu profiling. 107720: cli: add probe_range in debug.zip r=j82w a=j82w PR #79546 introduces `crdb_internal.probe_range`. This PR adds the `crdb_internal.probe_range` to the debug.zip. The LIMIT gives a very approximately ~1000ms*100 target on how long this can take, so that running debug.zip against an unavailable cluster won't take too long. closes: #80360 Release note (cli change): The debug.zip now includes the `crdb_internal.probe_range` table with a limit of 100 rows to avoid the query from taking to long. 107727: server: deflake TestServerShutdownReleasesSession r=rafiss a=rafiss The tenant was not being fully stopped, so the test could encounter flakes. fixes #107592 Release note: None 107742: ui: show txn fingerprint details page with unspecified app r=xinhaoz a=xinhaoz Previously, when the app was not specified in the url search params for the txn details fingerprint page, the page would fail to load. This commit allows the page to load when there is no app specified but a fingerprint id that matches the requested page in the payload. The first matching fingerprint id is loaded. Additionally, the TransactionDetailsLink will not include the appNames search param unless the provided prop is non-nullish. Fixes: #107731 Release note (bug fix): Txn fingerprint details page in the console UI should load with the fingerprint details even if no app is specified in the URL. Demo: https://www.loom.com/share/810308d3dcd74ca888c42287ebafaecf 107745: kvserver: fix test merge queue when grunning unsupported r=irfansharif a=kvoli `TestMergeQueue/load-based-merging/switch...below-threshold` asserts that switching the split objective between CPU and QPS will not cause ranges to merge, even if their pre-switch load qualified them for merging. This test was broken when `grunning` was unsupported, as the objective never actually switches to anything other than QPS. Add a check for `grunning` support, and assert that a merge occurs if unsupported. Fixes: #106937 Epic: none Release note: None 107749: opt: add enable_durable_locking_for_serializable session variable r=DrewKimball,nvanbenschoten a=michae2 Follow-up from #105857 This commit ammends 6a3e43d to add a session variable to control whether guaranteed-durable locks are used under serializable isolation. Informs: #100194 Epic: CRDB-25322 Release note (sql change): Add a new session variable, `enable_durable_locking_for_serializable`, which controls locking durability under serializable isolation. With this set to true, SELECT FOR UPDATE locks, SELECT FOR SHARED locks, and constraint check locks (e.g. locks acquired during foreign key checks if `enable_implicit_fk_locking_for_serializable` is set to true) will be guaranteed-durable under serializable isolation, meaning they will always be held to transaction commit. (These locks are always guaranteed-durable under weaker isolation levels.) By default, under serializable isolation these locks are best-effort rather than guaranteed-durable, meaning in some cases (e.g. leaseholder transfer, node loss, etc.) they could be released before transaction commit. Serializable isolation does not rely on locking for correctness, only using it to improve performance under contention, so this default is a deliberate choice to avoid the performance overhead of lock replication. 107752: changefeedccl: prevent deadlock in TestChangefeedKafkaMessageTooLarge r=miretskiy a=jayshrivastava Previously, this test would deadlock due to kafka retrying messages too many times. These messages are stored in a buffer of size 1024 created by the CDC testing infra: https://github.com/cockroachdb/cockroach/blob/5c3f96d38cdc3a2d953ca3ffb1e39e97d7e5110e/pkg/ccl/changefeedccl/testfeed_test.go#L1819 The test asserts that 2000 messages pass through the buffer. When the test finishes, it stops reading from the buffer. The problem is that due to retries, there may be more messages sent to the buffer than that are read out of the buffer. Even after the 2000 messages are read and the test is shutting down, the sink may be blocked trying to put resolved messages (plus retries) in the buffer. If this happens, the changefeed resumer (same goroutine as the kafka sink) gets blocked and does not terminate when the job is cancelled at the end of the test. This change caps the number of retries at 200 for this test, so there should be no more than 200 extra messages plus a few resolved messages during this test. This is far less than the buffer size of 1024. See detailed explanation in #107591. Fixes: #107591 Epic: none Release note: None 107802: teamcity-trigger: don't start a job for an empty target r=healthy-pod a=rickystewart This makes no sense, so skip these cases. Closes: #107779 Closes: #107780 Closes: #107781 Epic: none Release note: None 107803: githubpost: set `map` field if `null` r=healthy-pod a=rickystewart Go is a really good language. Informs: #107779 Epic: none Release note: None Co-authored-by: Erik Grinaker <grinaker@cockroachlabs.com> Co-authored-by: Lidor Carmel <lidor@cockroachlabs.com> Co-authored-by: Xin Hao Zhang <xzhang@cockroachlabs.com> Co-authored-by: j82w <jwilley@cockroachlabs.com> Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com> Co-authored-by: Austen McClernon <austen@cockroachlabs.com> Co-authored-by: Michael Erickson <michae2@cockroachlabs.com> Co-authored-by: Jayant Shrivastava <jayants@cockroachlabs.com> Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com>

Building on top of cockroachdb#110201, plumb locking durability from the optimizer to creation of Get/Scan/ReverseScan requests in txnKVFetcher and txnKVStreamer. Fixes: cockroachdb#100194 Release note: None

110945: sql: plumb locking durability into Get/Scan/ReverseScan requests r=arulajmani,nvanbenschoten,yuzefovich a=michae2 **execinfrapb: remove IndexSkipTableReaderSpec** The `indexSkipTableReader` was deleted in #51178 but the distsql spec has stuck around for a few versions. I think we can delete the spec now, too. If someone wants to dust off #39668 they can bring it back. Release note: None **sql: plumb locking durability into Get/Scan/ReverseScan requests** Building on top of #110201, plumb locking durability from the optimizer to creation of Get/Scan/ReverseScan requests in txnKVFetcher and txnKVStreamer. Also add a version gate for usage of guaranteed-durable locking. Fixes: #100194 Release note: None Co-authored-by: Michael Erickson <michae2@cockroachlabs.com>

Building on top of cockroachdb#110201, plumb locking durability from the optimizer to creation of Get/Scan/ReverseScan requests in txnKVFetcher and txnKVStreamer. Also add a version gate for usage of guaranteed-durable locking. Fixes: cockroachdb#100194 Release note: None

In cockroachdb#110945 I moved the locking durability check from optbuilder to execbuilder to allow a memo prepared under serializable isolation to execute under read committed isolation. As it turns out, however, memo staleness prevents this. Move the durability check back to optbuilder. We have to keep the shared locking check in execbuilder to correctly throw errors if shared locking is used in a read-only transaction. Informs: cockroachdb#100194 Release note: None

112082: opt: move locking durability check back to optbuilder r=DrewKimball a=michae2 In #110945 I moved the locking durability check from optbuilder to execbuilder to allow a memo prepared under serializable isolation to execute under read committed isolation. As it turns out, however, memo staleness prevents this. Move the durability check back to optbuilder. We have to keep the shared locking check in execbuilder to correctly throw errors if shared locking is used in a read-only transaction. Informs: #100194 Release note: None Co-authored-by: Michael Erickson <michae2@cockroachlabs.com>

nvanbenschoten mentioned this issue Mar 30, 2023

kv: implement replicated key-level locks #100193

Closed

exalate-issue-sync bot assigned michae2 Apr 5, 2023

bobvawter mentioned this issue Apr 12, 2023

cdc: Use lease package when retrieving work cockroachdb/replicator#297

Merged

michae2 mentioned this issue May 22, 2023

opt: disallow SELECT FOR UPDATE under weak isolation levels #103734

Merged

michae2 mentioned this issue Jul 11, 2023

sql: add implicit SELECT FOR SHARE locking to FK checks #105857

Merged

michae2 mentioned this issue Jul 27, 2023

opt: add enable_durable_locking_for_serializable session variable #107749

Merged

michae2 mentioned this issue Sep 20, 2023

sql: plumb locking durability into Get/Scan/ReverseScan requests #110945

Merged

craig bot closed this as completed in 3d593e1 Oct 2, 2023

michae2 mentioned this issue Oct 10, 2023

opt: move locking durability check back to optbuilder #112082

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: acquire replicated locks on explicit SELECT FOR UPDATE #100194

sql: acquire replicated locks on explicit SELECT FOR UPDATE #100194

nvanbenschoten commented Mar 30, 2023 •

edited by exalate-issue-sync bot

Loading

michae2 commented Jun 29, 2023

sql: acquire replicated locks on explicit SELECT FOR UPDATE #100194

sql: acquire replicated locks on explicit SELECT FOR UPDATE #100194

Comments

nvanbenschoten commented Mar 30, 2023 • edited by exalate-issue-sync bot Loading

michae2 commented Jun 29, 2023

nvanbenschoten commented Mar 30, 2023 •

edited by exalate-issue-sync bot

Loading