Reviewed 2 of 5 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @AlexTalks, @arulajmani, and @RaduBerinde)

-- commits line 2 at r1:
After thinking about this more, I agree with @shralex that the naming change isn't quite right. Interestingly, PG's code has a notion of a lock "strength" and a lock "mode", but their meanings are roughly switched from what you have here. A locking mode includes a strength and a few other details about the target of the lock. So I'd suggest we switch the two names.

I'd also suggest that so that the LockingStrength concept in SQL (which is analogous to PG's LockClauseStrength) doesn't need to change.

pkg/kv/kvserver/concurrency/lock/locking.go line 97 at r2 (raw file):

// MakeStrength constructs a well-formed lock Strength.
func MakeStrength(mode LockMode, opts ...StrengthOption) Strength {

The functional option pattern is nice, but it's also expensive. For instance, I think AtTimestamp allocates the returned closure.

We only have a few strengths, so I wonder if we could get away with specialized constructors for each locking strength. Would that make the code easier to read at callers?

pkg/kv/kvserver/concurrency/lock/locking.proto line 157 at r2 (raw file):

//  | None       |         |           |           |      X^*    |    X^†   |
//  +------------+---------+-----------+-----------+-------------+----------+
//  | Shared     |         |           |     X     |      X      |    X     |

You were planning to fix the Upgrade lock description, right? An Upgrade lock does not conflict with Shared locks, but does conflict with other Upgrade locks.

Also, I got the name wrong. They're actually called Update locks (see https://learn.microsoft.com/en-us/sql/relational-databases/sql-server-transaction-locking-and-row-versioning-guide?view=sql-server-ver16#update) 🤦

pkg/kv/kvserver/concurrency/lock/locking.proto line 180 at r2 (raw file):

// TODO(arul): Add a reference to the cluster setting here.
//
// TODO(XXX): Do we need this to be a proto? Or are we happy with a regular go

I think a proto makes sense, in case we want to store these in persisted locks if/when we get there.

I'll do the s/Update/Upgrade/g switch in another commit so that reviewing it is easy. I'll tack it on this PR tomorrow; pushing out all other changes before I sign off for the day though.

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

-- commits line 2 at r1:

Done. Dropped the first commit entirely and added lock.Mode to encapsulate lock.Strength + auxiliary information, like you suggested.

pkg/kv/kvserver/concurrency/lock/locking.go line 97 at r2 (raw file):

Changed explicit constructors for all (what is now) lock modes.

pkg/kv/kvserver/concurrency/lock/locking.proto line 157 at r2 (raw file):

Yeah, I didn't get a chance to push changes here after our discussion -- changed this matrix and the implementation/tests of our compatibility function.

pkg/kv/kvserver/concurrency/lock/locking.proto line 180 at r2 (raw file):

Makes sense!

I'll do the s/Update/Upgrade/g switch in another commit so that reviewing it is easy.

Done.

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

Reviewed 27 of 27 files at r3, 9 of 9 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @AlexTalks, @arulajmani, and @RaduBerinde)

pkg/kv/kvserver/concurrency/lock/locking.go line 19 at r3 (raw file):

	"github.com/cockroachdb/cockroach/pkg/settings"
	hlc "github.com/cockroachdb/cockroach/pkg/util/hlc"

nit: no need for the alias here, Goland

pkg/kv/kvserver/concurrency/lock/locking.go line 39 at r3 (raw file):

	"dictates the locking interactions between exclusive locks and non-locking reads",
	true,
).WithPublic()

We should probably keep this setting hidden for now.

pkg/kv/kvserver/concurrency/lock/locking.go line 55 at r3 (raw file):

// the receiver. The conflict rules are as described in the compatibility matrix
// in locking.pb.go.
func (m *Mode) CheckLockConflicts(sv *settings.Values, o *Mode) bool {

Consider just naming this Conflicts for succinctness. Same thing with the method below. Compatible would be enough.

pkg/kv/kvserver/concurrency/lock/locking.go line 59 at r3 (raw file):

}

// CheckLockCompatibility returns whether the supplied lock mode is

It might be confusing to have two methods here. Callers will need to ask which one they want to use and will need to jump to this code to convince themselves that the two predicates are inverses. Do we need them both?

pkg/kv/kvserver/concurrency/lock/locking.proto line 18 at r3 (raw file):

import "util/hlc/timestamp.proto";

// Strength represents the different locking strength that determines how

"locking strengths that determine how"?

pkg/kv/kvserver/concurrency/lock/locking.proto line 90 at r3 (raw file):

  // holders are not waiting on the Upgrade lock in any way.
  //
  // Under pure pessimistic concurrency control, an Upgrade lock is equivalent

Should these two paragraphs be moved to Exclusive and reworded?

pkg/kv/kvserver/concurrency/lock/locking.proto line 109 at r3 (raw file):

  // TODO(XXX): How should we frame the wording around "being forced to increase
  // write timestamp when converting to an Exclusive lock" -- is this something
  // that will (can?) happen? Or will this only happen when laying down intents.

Yeah, this should only happen when laying down intents.

pkg/kv/kvserver/concurrency/lock/locking_test.go line 26 at r3 (raw file):

// TestCheckLockCompatibility ensures the compatibility semantics, as
// illustrated in the compatibility matrix in locking.pb.go, are upheld.
func TestCheckLockCompatibility(t *testing.T) {

Nice test!

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @AlexTalks, @arulajmani, and @RaduBerinde)

pkg/kv/kvserver/concurrency/lock/locking.proto line 180 at r4 (raw file):

// above the Exclusive lock's timestamp. Now that there is a distinction between
// Exclusive locks and Intents, non-locking reads do not block on Exclusive
// locks, regardless of their timestamp. However, there is a desire to let users

(drive-by) will these exclusive locks be persisted, like we currently persist intents, or will they be best-effort, like our current unreplicated locks?

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @AlexTalks, @RaduBerinde, and @sumeerbhola)

pkg/kv/kvserver/concurrency/lock/locking.proto line 180 at r4 (raw file):

They won't be persisted and will continue to be best effort, like today. We've been discussing how to make this "best effort" situation better, in the context of lower isolation levels where we need locks for correctness, but I don't think we've bottomed out there yet.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @AlexTalks, @nvanbenschoten, @RaduBerinde, and @sumeerbhola)

pkg/kv/kvserver/concurrency/lock/locking.go line 39 at r3 (raw file):

Done.

pkg/kv/kvserver/concurrency/lock/locking.go line 55 at r3 (raw file):

Done.

pkg/kv/kvserver/concurrency/lock/locking.go line 59 at r3 (raw file):

Got rid of the Compatible method -- like we discussed, we don't need both.

pkg/kv/kvserver/concurrency/lock/locking.proto line 90 at r3 (raw file):

Done. I ended up hinting at buffered writes here:

  // Note that for this to be meaningful, the write transaction
  // must acquire Exclusive locks as it is executing and only lay down Intents
  // once it is ready to commit.

Let me know if you're not a fan.

pkg/kv/kvserver/concurrency/lock/locking.proto line 109 at r3 (raw file):

Thanks, clarified.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

pkg/kv/kvserver/concurrency/lock/locking.proto line 125 at r6 (raw file):

  // that key. The lock holder is free to read from and write to the key as
  // frequently as it would like.
  Intent = 4;

We serialize the lock.Strength in LockTableKey and currently Exclusive is used for intents. I assume we will want to switch to Intent, but then we need to assign Intent = 3.