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

michae2 · 2023-05-22T19:40:35Z

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:

Fixes: #100144

Release note: None

cockroach-teamcity · 2023-05-22T19:40:47Z

This change is

michae2 · 2023-05-22T19:43:31Z

pkg/sql/sem/tree/select.go

+
+var lockingDurabilityName = [...]string{
+	LockDurabilityBestEffort: "best-effort",
+	LockDurabilityGuaranteed: "guaranteed",


(I'm not attached to these names, so please feel free to suggest different ones if you don't like these.)

nvanbenschoten

Reviewed 1 of 1 files at r1, 8 of 8 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @DrewKimball, and @michae2)

pkg/sql/opt/locking.go line 56 at r2 (raw file):

}

// Max returns the maximum of two locks.

"of two locking properties"?

pkg/sql/opt/locking.go line 61 at r2 (raw file):

		Strength:   l.Strength.Max(l2.Strength),
		WaitPolicy: l.WaitPolicy.Max(l2.WaitPolicy),
		Durability: l.Durability.Max(l.Durability),

Should this be l2.Durability?

pkg/sql/opt/exec/execbuilder/mutation.go line 976 at r2 (raw file):

// forUpdateLocking is the row-level locking mode used by mutations during their
// initial row scan, when such locking is deemed desirable. The locking mode is
// equivalent that used by a SELECT ... FOR UPDATE statement except not durable.

Should we make this explicit by assigning the Durability field to tree.LockDurabilityBestEffort?

pkg/sql/opt/exec/execbuilder/relational.go line 634 at r3 (raw file):

				"cannot execute %s in a read-only transaction", locking.Strength.String())
		}
		if locking.Durability == tree.LockDurabilityGuaranteed &&

Is this sufficient to enforce the guardrail for all forms of locking expressions (e.g. IndexJoinExpr in buildIndexJoin)?

pkg/sql/opt/optbuilder/locking.go line 81 at r2 (raw file):

			Strength:   spec.Strength,
			WaitPolicy: spec.WaitPolicy,
			Durability: tree.LockDurabilityGuaranteed,

Does this warrant a comment?

pkg/sql/sem/tree/select.go line 1210 at r2 (raw file):

	// LockDurabilityBestEffort represents the default: make a best-effort attempt
	// to hold the lock until commit while keeping it unreplicated and
	// in-memory. This must not be used when correctness depends on locking.

"in-memory on the leaseholder of the locked row"

Also, consider describing why anyone would want to use this locking durability.

michae2

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @DrewKimball, and @nvanbenschoten)

pkg/sql/opt/locking.go line 56 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

"of two locking properties"?

Done.

pkg/sql/opt/locking.go line 61 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should this be l2.Durability?

Gah, yes. Done.

pkg/sql/opt/exec/execbuilder/mutation.go line 976 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should we make this explicit by assigning the Durability field to tree.LockDurabilityBestEffort?

Done.

pkg/sql/opt/exec/execbuilder/relational.go line 634 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Is this sufficient to enforce the guardrail for all forms of locking expressions (e.g. IndexJoinExpr in buildIndexJoin)?

Good catch! I forgot a few places. Added a helper function that is now called in all of them.

pkg/sql/opt/optbuilder/locking.go line 81 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Does this warrant a comment?

Yes, done.

pkg/sql/sem/tree/select.go line 1210 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

"in-memory on the leaseholder of the locked row"

Also, consider describing why anyone would want to use this locking durability.

Done.

nvanbenschoten

Reviewed 7 of 8 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @arulajmani, @DrewKimball, and @michae2)

pkg/sql/opt/locking.go line 50 at r3 (raw file):

under SNAPSHOT and READ COMMITTED isolation

You removed this, but it was partially correct. It's unclear whether we're going to be able to grab "guaranteed" locks for SFU under SSI in the next release, or whether this will cause too much of a latency regression for some workloads. At a minimum, we'll probably need a cluster setting to disable them.

pkg/sql/opt/exec/execbuilder/relational.go line 2881 at r6 (raw file):

		locking = locking.Max(forUpdateLocking)
	}
	if locking.IsLocking() {

nit: consider returning early if !locking.IsLocking() { so you can outdent the rest of the logic.

pkg/sql/sem/tree/select.go line 1221 at r3 (raw file):

Previously, michae2 (Michael Erickson) wrote…

(I'm not attached to these names, so please feel free to suggest different ones if you don't like these.)

I'm ok with these names. @arulajmani what do you think?

arulajmani

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball, @michae2, and @nvanbenschoten)

pkg/sql/sem/tree/select.go line 1221 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I'm ok with these names. @arulajmani what do you think?

The names, in conjunction with the comments, look great to me!

mgartner

Reviewed 1 of 1 files at r1, 10 of 10 files at r4, 8 of 8 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @DrewKimball, @michae2, and @nvanbenschoten)

pkg/sql/opt/locking.go line 47 at r5 (raw file):

	WaitPolicy tree.LockingWaitPolicy

	// The third property is the durability of the locking. A durable lock always

nit: It would be helpful to newcomers to explain that "durable" and "guaranteed" are synonyms, same with "non-durable" and "best effort", here, and maybe also where they are defined.

pkg/sql/sem/tree/select.go line 1220 at r5 (raw file):

	// lock was held until commit, even in the face of lease transfers, range
	// splits, range merges, node failures, memory limits, etc.  Fully-durable
	// locks *must* be used when correctness depends on locking.

nit: it's unclear to a newcomer that "guaranteed" is synonymous with "fully-durable".

pkg/sql/logictest/testdata/logic_test/select_for_update line 531 at r6 (raw file):

ROLLBACK

# It should also fail under snapshot isolation. TODO(michae2): uncomment after #103488 is merged.

nit: TODO on its own line

Remove the `IsCorrelated` flag from `querycache.CachedData`, which is no longer used. Release note: None

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

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

michae2

TFYRs!

bors r=nvanbenschoten,mgartner

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @DrewKimball, @mgartner, and @nvanbenschoten)

pkg/sql/opt/locking.go line 50 at r3 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

under SNAPSHOT and READ COMMITTED isolation

You removed this, but it was partially correct. It's unclear whether we're going to be able to grab "guaranteed" locks for SFU under SSI in the next release, or whether this will cause too much of a latency regression for some workloads. At a minimum, we'll probably need a cluster setting to disable them.

Ack, good point. I brought back the qualification.

pkg/sql/opt/locking.go line 47 at r5 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: It would be helpful to newcomers to explain that "durable" and "guaranteed" are synonyms, same with "non-durable" and "best effort", here, and maybe also where they are defined.

I changed the language here to use "guaranteed-durable" and "best-effort" to more closely match the code.

pkg/sql/sem/tree/select.go line 1220 at r5 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: it's unclear to a newcomer that "guaranteed" is synonymous with "fully-durable".

I changed this comment to not use "fully-durable" any more.

pkg/sql/logictest/testdata/logic_test/select_for_update line 531 at r6 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: TODO on its own line

Done. (I removed it and uncommented the testcase.)

craig · 2023-05-25T21:47:30Z

Build succeeded:

Bazel Essential CI (Cockroach)

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]>

michae2 requested review from nvanbenschoten, arulajmani and DrewKimball May 22, 2023 19:40

michae2 requested review from a team as code owners May 22, 2023 19:40

michae2 commented May 22, 2023

View reviewed changes

nvanbenschoten reviewed May 22, 2023

View reviewed changes

michae2 force-pushed the rcguardrails branch 2 times, most recently from be4f2f5 to 5a6c518 Compare May 23, 2023 05:04

michae2 commented May 23, 2023

View reviewed changes

nvanbenschoten approved these changes May 23, 2023

View reviewed changes

arulajmani reviewed May 23, 2023

View reviewed changes

mgartner approved these changes May 23, 2023

View reviewed changes

michae2 force-pushed the rcguardrails branch 3 times, most recently from e3e5bfb to 8f642f4 Compare May 25, 2023 17:14

michae2 added 3 commits May 25, 2023 12:26

querycache: remove unused field from CachedData

57b3bdf

Remove the `IsCorrelated` flag from `querycache.CachedData`, which is no longer used. Release note: None

michae2 force-pushed the rcguardrails branch from 8f642f4 to e633d5e Compare May 25, 2023 19:53

michae2 commented May 25, 2023

View reviewed changes

craig bot merged commit a1d645b into cockroachdb:master May 25, 2023

michae2 deleted the rcguardrails branch June 29, 2023 16:21

michae2 mentioned this pull request Jul 11, 2023

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

michae2 commented May 22, 2023

cockroach-teamcity commented May 22, 2023

michae2 May 22, 2023

nvanbenschoten left a comment

michae2 left a comment

nvanbenschoten left a comment

arulajmani left a comment

mgartner left a comment

michae2 left a comment

craig bot commented May 25, 2023

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

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

Conversation

michae2 commented May 22, 2023

cockroach-teamcity commented May 22, 2023

michae2 May 22, 2023

Choose a reason for hiding this comment

nvanbenschoten left a comment

Choose a reason for hiding this comment

michae2 left a comment

Choose a reason for hiding this comment

nvanbenschoten left a comment

Choose a reason for hiding this comment

arulajmani left a comment

Choose a reason for hiding this comment

mgartner left a comment

Choose a reason for hiding this comment

michae2 left a comment

Choose a reason for hiding this comment

craig bot commented May 25, 2023