sql: don't allow SELECT FOR UPDATE in weak isolation transactions #100144

nvanbenschoten · 2023-03-30T15:21:44Z

Until we can make this safe with replicated locks, don't allow it.

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 <[email protected]>

This is a follow-up from cockroachdb#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 ammends 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: cockroachdb#100144, cockroachdb#100156, cockroachdb#100193, 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 <[email protected]> Co-authored-by: Ricky Stewart <[email protected]> Co-authored-by: Nathan VanBenschoten <[email protected]> Co-authored-by: Marius Posta <[email protected]>

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

michae2 mentioned this issue May 22, 2023

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

Merged

craig bot closed this as completed in e633d5e May 25, 2023

michae2 mentioned this issue Jul 11, 2023

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

Merged

mgartner added this to SQL Queries Jul 24, 2023

mgartner moved this to Done in SQL Queries Jul 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: don't allow SELECT FOR UPDATE in weak isolation transactions #100144

sql: don't allow SELECT FOR UPDATE in weak isolation transactions #100144

nvanbenschoten commented Mar 30, 2023 •

edited by exalate-issue-sync bot

Loading

sql: don't allow SELECT FOR UPDATE in weak isolation transactions #100144

sql: don't allow SELECT FOR UPDATE in weak isolation transactions #100144

Comments

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

nvanbenschoten commented Mar 30, 2023 •

edited by exalate-issue-sync bot

Loading