sql: use the correct locking strength for FOR SHARE clause

[arulajmani]

Previously, FOR SHARE and FOR KEY SHARE would use non-locking KV scans.
Now that the lock table supports shared locks, we can use lock.Shared as
the locking strength for KV scans. This patch does that, and in doing
so, wires up SHARED locks end to end.

By default, we turn of this functionality for serializable transactions.
Instead, it's gated behind a session setting called
enable_shared_locking_for_serializable.

Closes #91545

Release note (sql change): SELECT FOR SHARE and SELECT FOR UPDATE
previously did not acquire any locks. Users issuing these statements
would expect them to acquire shared locks (multiple readers allowed,
no writers though). This patch switches over the behavior to acquire
such read locks.

For serializable transactions, we default to the old behaviour, unless
the enable_shared_locking_for_serializable session setting is set
to true. We'll probably switch this behavior in the future, but for
now, given shared locks are a preview feature, we gate things behind
a session setting.

[cockroach-teamcity]

This change is 

[arulajmani]

@michae2 it looks like the optbuilder and execbuilder tests already handle the FOR SHARE clause correctly. Are there any other tests we would want to add here?

Separately, @nvanbenschoten and @michae2, do we want to add a cluster setting to revert back to the pre-23.2FOR SHARE behavior?

[nvanbenschoten]

Separately, @nvanbenschoten and @michae2, do we want to add a cluster setting to revert back to the pre-23.2FOR SHARE behavior?

That sounds like a good idea.

Reviewed 10 of 10 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @michae2)

---

pkg/sql/row/locking.go line 30 at r2 (raw file):

		fallthrough
	case descpb.ScanLockingStrength_FOR_SHARE:
		return lock.Shared

Do we need to hide this behind a cluster version check?

---

pkg/sql/logictest/testdata/logic_test/shared_locks line 1 at r2 (raw file):

statement ok

Should we call this file select_for_share to mirror the select_for_update file?

[arulajmani]

I'm going to let this run through CI to catch any plumbing issues, but should be good for a look.

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

---

pkg/sql/row/locking.go line 30 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Do we need to hide this behind a cluster version check?

Added a cluster version check and hid this behaviour behind a cluster setting (defaults to 23.1 behaviour).

---

pkg/sql/logictest/testdata/logic_test/shared_locks line 1 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should we call this file select_for_share to mirror the select_for_update file?

Done.

[michae2]

I should have commented on this earlier, I'm sorry I didn't. GetKeyLockingStrength is too low to make the decision about whether to use locking or not. By this point we should already have picked correctly based on session variables, version gates, isolation level, etc, and GetKeyLockingStrength should merely be translating from execinfrapb locking to kv locking. These decisions about whether to lock live in optbuilder. I think the place to put the check would be in buildScan:

cockroach/pkg/sql/opt/optbuilder/select.go

Lines 700 to 724 in af05d81

	if b.evalCtx.TxnIsoLevel != isolation.Serializable ||
	b.evalCtx.SessionData().DurableLockingForSerializable {
	// Under weaker isolation levels we use fully-durable locks for SELECT FOR
	// UPDATE statements, SELECT FOR SHARE statements, and constraint checks
	// (e.g. FK checks), regardless of locking strength and wait policy.
	// Unlike mutation statements, SELECT FOR UPDATE statements do not lay
	// down intents, so we cannot rely on the durability of intents to
	// guarantee exclusion until commit as we do for mutation statements. And
	// unlike serializable isolation, weaker isolation levels do not perform
	// read refreshing, so we cannot rely on read refreshing to guarantee
	// exclusion.
	//
	// Under serializable isolation we only use fully-durable locks if
	// enable_durable_locking_for_serializable is set. (Serializable isolation
	// does not require locking for correctness, so by default we use
	// best-effort locks for better performance.)
	private.Locking.Durability = tree.LockDurabilityGuaranteed
	}
	if private.Locking.WaitPolicy == tree.LockWaitSkipLocked && tab.FamilyCount() > 1 {
	// TODO(rytaft): We may be able to support this if enough columns are
	// pruned that only a single family is scanned.
	panic(pgerror.Newf(pgcode.FeatureNotSupported,
	"SKIP LOCKED cannot be used for tables with multiple column families",
	))
	}

After locking.get() we know the locking strength the optimizer wants. This would be a good spot to silently turn off locking if FOR SHARE has been requested and we're not using RC and we can't do it due to a version gate.

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

---

pkg/sql/row/locking.go line 34 at r4 (raw file):

	false,
	settings.WithPublic,
)

Whenever the optimizer makes decisions based on settings, (a) they should be session settings rather than cluster settings, and (b) they need to be added to Memo.IsStale in pkg/sql/opt/memo/memo.go. There's an example of that in this PR: bb857aa

[arulajmani]

Thanks for the pointers @michae2. This should be good for a look now.

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

---

pkg/sql/row/locking.go line 34 at r4 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Whenever the optimizer makes decisions based on settings, (a) they should be session settings rather than cluster settings, and (b) they need to be added to Memo.IsStale in pkg/sql/opt/memo/memo.go. There's an example of that in this PR: bb857aa

Makes sense. I've added a session variable, similar in spirit to the one you added for durable locks for serializable transactions.

[nvanbenschoten]

Looks like there are some failures in CI. Where possible, I think we should just set the session variable to avoid the test churn.

The failures that look like the following are more interesting:

expected "cannot execute FOR SHARE in a read-only transaction", but no error occurred

I guess the change to Builder.buildScan is masking the check in Builder.buildLocking. I don't think we want SELECT FOR SHARE to be usable in read-only transactions, even if the session variable is unset. If we let it succeed when the session variable is unset then we risk breaking apps if we change the default (or remove the variable entirely) in the future.

Maybe we should call buildLocking as a form of error checking before discarding the locking mode in buildScan. What do you think @michae2?

Reviewed 14 of 33 files at r3, 29 of 29 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @michae2)

---

-- commits line 9 at r5:
s/of/off/

---

-- commits line 15 at r5:
Heh, you'll want to help the docs writers out a little more than that. Maybe also mention the session variable and how this is still a preview feature.

---

pkg/sql/BUILD.bazel line 582 at r5 (raw file):

        "//pkg/util/uint128",
        "//pkg/util/uuid",
        "@com_github_cockroachdb_apd//:apd",

Not sure why this got pulled in again.

---

pkg/sql/opt/optbuilder/select.go line 720 at r5 (raw file):

			private.Locking.Durability = tree.LockDurabilityGuaranteed
		}
		// Check if we can actually use shared locks here , or we need to use

s/here ,/here,/

---

pkg/sql/sessiondatapb/local_only_session_data.proto line 426 at r5 (raw file):

  // better performance.) Weaker isolation levels always use durable locking.
  bool durable_locking_for_serializable = 109;
  // SharedLockingForSerializable, if set to true, means SELECT FOR  SHARE and

nit: double space between "FOR" and "SHARE".

---

pkg/sql/sessiondatapb/local_only_session_data.proto line 428 at r5 (raw file):

  // SharedLockingForSerializable, if set to true, means SELECT FOR  SHARE and
  // SELECT FOR KEY SHARE statements issued by transactions that run with
  // Serializable isolation will acquiring shared locks; otherwise, they'll

nit: lower-case Serializable.

---

pkg/sql/sessiondatapb/local_only_session_data.proto line 428 at r5 (raw file):

  // SharedLockingForSerializable, if set to true, means SELECT FOR  SHARE and
  // SELECT FOR KEY SHARE statements issued by transactions that run with
  // Serializable isolation will acquiring shared locks; otherwise, they'll

s/acquiring/acquire/

---

pkg/sql/opt/exec/execbuilder/testdata/select_for_update line 45 at r5 (raw file):

  locking strength: for no key update

# By default, SELECT FOR SHARE doesn't acquire any locks.

It's nice that this is now reflected in the EXPLAIN output!

---

pkg/sql/logictest/testdata/logic_test/select_for_share line 47 at r5 (raw file):

# TODO(arul): Until https://github.com/cockroachdb/cockroach/issues/107766 is
# addressed, we'll incorrectly report shared locks as having "Exclusive" lock
# strength; however, having this query in here is useful to make sure there are

We'll also incorrectly only report a single "granted" lock holder, right? That's why we only see two rows instead of three?

---

pkg/sql/logictest/testdata/logic_test/select_for_share line 112 at r5 (raw file):

# TODO(arul): Add a test to show that the session setting doesn't apply to read
# committed transactions. We currently can't issue SELECT FOR SHARE statmenets

"statements"

[arulajmani]

The failures that look like the following are more interesting:

expected "cannot execute FOR SHARE in a read-only transaction", but no error occurred

I was looking into this as well -- what's interesting is that just setting the session variable doesn't get rid of it, which Im surprised by. Changing the global default does. My best guess is that this is because I've added this session to "local only session data", instead of session data.

This may be a side quest, but I'll see if making the switch over fixes things. This might be interesting, as it relates to the DurableLockingForSerializable field. I could also be doing something stupid here.

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

---

pkg/sql/BUILD.bazel line 582 at r5 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Not sure why this got pulled in again.

My GoLand keeps doing this for some reason. It keeps replacing apd/v3 with apd, so every time I re-run ./dev generate this happens. I'll make sure things are clean before I actually get to bors-ing this.

---

pkg/sql/logictest/testdata/logic_test/select_for_share line 47 at r5 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

We'll also incorrectly only report a single "granted" lock holder, right? That's why we only see two rows instead of three?

Ah right, I missed that -- didn't realize we include waiters in here as well. I'll update this comment.

[nvanbenschoten]

they need to be added to Memo.IsStale in pkg/sql/opt/memo/memo.go

Maybe this is related? There's no update to memo.go in this PR.

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

[arulajmani]

Maybe this is related? There's no update to memo.go in this PR.

Yup, you're right. I'll fix this in my next push.

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

[arulajmani]

Alright, addressed all the comments here and worked through some of the CI flakes. I think the main ones left now are related to:

I guess the change to `Builder.buildScan` is masking the check in `Builder.buildLocking`. I don't think we want SELECT FOR SHARE to be usable in read-only transactions, even if the session variable is unset. If we let it succeed when the session variable is unset then we risk breaking apps if we change the default (or remove the variable entirely) in the future.

I agree. @michae2, I'll defer to you on calling buildLocking for error checking or rolling out something a bit more special cased here.

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

---

-- commits line 15 at r5:

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Heh, you'll want to help the docs writers out a little more than that. Maybe also mention the session variable and how this is still a preview feature.

Haha changed this to actually be a useful release note. Good call on the preview call out.

---

pkg/sql/opt/exec/execbuilder/testdata/select_for_update line 45 at r5 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

It's nice that this is now reflected in the EXPLAIN output!

Heh I never considered the old thing to be a bug until you pointed it out!

[michae2]

Thank you for turning this around so quickly! I've got fixes for the remaining CI failures here, if you want them: #111478

I agree. @michae2, I'll defer to you on calling buildLocking for error checking or rolling out something a bit more special cased here.

I think we just need to add a small check to optbuilder.(*Builder).validateLockingInFrom. I've done that in the draft PR I linked to, as well.

Reviewed 7 of 29 files at r5, 7 of 10 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @nvanbenschoten)

---

-- commits line 15 at r5:

Previously, arulajmani (Arul Ajmani) wrote…

Haha changed this to actually be a useful release note. Good call on the preview call out.

I think "SELECT FOR UPDATE" should be removed from this sentence. (It previously did acquire locks, right?)

---

pkg/sql/opt/optbuilder/select.go line 701 at r6 (raw file):

	if locking.isSet() {
		private.Locking = locking.get()
		// TODO(arul): This needs a cluster version check.

Gah, good catch! I should have done this. If you want to, you can put me as the TODO.

---

pkg/sql/opt/optbuilder/select.go line 725 at r6 (raw file):

			// Shared locks weren't a thing prior to v23.2, so we must use non-locking
			// reads.
			if !b.evalCtx.Settings.Version.IsActive(context.TODO(), clusterversion.V23_2) ||

b.ctx instead of context.TODO()

---

pkg/sql/opt/optbuilder/select.go line 736 at r6 (raw file):

				// won't be too happy if we start asking for nonsensical "replicated
				// non-locking locks".
				private.Locking.Durability = tree.LockDurabilityBestEffort

I think we should also set private.Locking.WaitPolicy = tree.LockWaitBlock as well. And should also clear private.Locking.Form.

(In fact, it might be more straightforward to simply do private.Locking = opt.Locking{} instead of setting individual fields.)

---

pkg/sql/row/locking.go line 34 at r4 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Makes sense. I've added a session variable, similar in spirit to the one you added for durable locks for serializable transactions.

Thank you for doing this!

---

pkg/sql/row/locking.go line 37 at r6 (raw file):

	case descpb.ScanLockingStrength_FOR_UPDATE:
		// We currently perform exclusive per-key locking when FOR_UPDATE is
		// used because Update locks have not yet been implemented.

suggestion: I think we should remove this old comment, too.

[arulajmani]

I've got fixes for the remaining CI failures here, if you want them: Sl sql integration #111478

Thanks a lot Michael, this was breezy! I cherry picked your changes and merged them into my commit. The only test that's failing now is the one with prepared statements -- it makes sense, because by the time we come around to executing the statement in a transaction (which is the point at which we know it's read-only), we've already thrown away the locking information.

One option here would be to store an additional boolean on opts.Locking struct -- maybe a opts.Locking.OriginalLockingStrength. We could then use this for our read-only transaction rejection check. There may be something cleaner than this, though, so @michae2 I'll defer to you on the approach here.

Dismissed @michae2 from a discussion.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @michae2 and @nvanbenschoten)

---

-- commits line 15 at r5:

Previously, michae2 (Michael Erickson) wrote…

I think "SELECT FOR UPDATE" should be removed from this sentence. (It previously did acquire locks, right?)

I'd meant to say "SELECT FOR KEY SHARE". Fixed.

---

pkg/sql/opt/optbuilder/select.go line 701 at r6 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Gah, good catch! I should have done this. If you want to, you can put me as the TODO.

I'll try and follow this up with a quick patch, but just in case I don't get to it before I head out for OOO, I'll add both ours.

---

pkg/sql/opt/optbuilder/select.go line 725 at r6 (raw file):

Previously, michae2 (Michael Erickson) wrote…

b.ctx instead of context.TODO()

Done.

---

pkg/sql/opt/optbuilder/select.go line 736 at r6 (raw file):

Previously, michae2 (Michael Erickson) wrote…

I think we should also set private.Locking.WaitPolicy = tree.LockWaitBlock as well. And should also clear private.Locking.Form.

(In fact, it might be more straightforward to simply do private.Locking = opt.Locking{} instead of setting individual fields.)

Good call; switched to private.Locking = opt.Locking{}

---

pkg/sql/row/locking.go line 34 at r4 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Thank you for doing this!

Thanks for the pointers! This approach is much nicer.

---

pkg/sql/row/locking.go line 37 at r6 (raw file):

Previously, michae2 (Michael Erickson) wrote…

suggestion: I think we should remove this old comment, too.

Removed.

[nvanbenschoten]

Reviewed 6 of 10 files at r6, 8 of 8 files at r7, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani and @michae2)

---

pkg/sql/opt/optbuilder/select.go line 724 at r7 (raw file):

			// Shared locks weren't a thing prior to v23.2, so we must use non-locking
			// reads.
			//if !b.evalCtx.Settings.Version.IsActive(b.ctx, clusterversion.V23_2) ||

Why did this get commented out? Intentional?

[arulajmani]

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

---

pkg/sql/opt/optbuilder/select.go line 724 at r7 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Why did this get commented out? Intentional?

No, I was trying to see if this branch was tripping up a test failure and forgot to uncomment the thing. 🤦

[michae2]

Ah, drat, prepared statements. 🤔 There's also the possibility that we would prepare with a different isolation level or different enable_shared_locking_for_serializable setting than when we execute the prepared statement.

Let's try moving your code from optbuilder.(*Builder).buildScan to this part of execbuilder.(*Builder).buildLocking. That should only run when executing the prepared statement.

Then we should also be able to get rid of the small read-only check I added to optbuilder.(*Builder).validateLockingInFrom.

Now that you've pointed this out, I think I will also have to move my DurableLockingForSerializable check there as well, but I want to hold off on that until after #110945 goes in (or perhaps as part of it) because I need to think about how it will interact with implicit locking in UPDATE etc.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)

[michae2]

(Also I think you'll have to do a git restore -s HEAD^ pkg/sql/opt/optbuilder/testdata/* after moving the code from optbuilder to execbuilder.)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)

[arulajmani]

Yup, that worked out quite well. I'll let this run through CI one more time to make sure I didn't break anything (I tested a few tests locally, and things seemed fine). Should be good for another look.

Now that you've pointed this out, I think I will also have to move my DurableLockingForSerializable check there as well.

Good point!

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

[michae2]

Reviewed 1 of 10 files at r6, 3 of 8 files at r7, 4 of 16 files at r9, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @arulajmani and @nvanbenschoten)

---

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

			locking = opt.Locking{}
		}
	}

Oh, did this not work if it was inside buildLocking? We'll need it to be inside, because by this point in the optimizer SELECT FOR SHARE could have turned into a locking join rather than a locking scan.

(I can do this part in another PR if you need to go.)

[arulajmani]

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

---

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

Previously, michae2 (Michael Erickson) wrote…

Oh, did this not work if it was inside buildLocking? We'll need it to be inside, because by this point in the optimizer SELECT FOR SHARE could have turned into a locking join rather than a locking scan.

(I can do this part in another PR if you need to go.)

This wasn't intentional, I moved it inside. The other thing I realized was that we need to be careful with setting b.ContainsNonDefaultKeyLocking. Not doing so was causing the test timeouts in CI.

[arulajmani]

Alright, all tests are passing. Thanks again for all the pointers @michae2.

bors r=michae2

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)