This change is 

Awesome! I feel like most of the salient changes here are in the KV interface, no? I think we'd want a KV reviewer too.

One thing that I'd like to call to @rafiss's attention:

  // lock_timeout specifies the maximum amount of time that the batch request
  // will wait while attempting to acquire a lock on a key or while blocking on
  // an existing lock in order to perform a non-locking read on a key. The time
  // limit applies separately to each lock acquisition attempt. If the timeout
  // elapses when waiting for a lock, a WriteIntentError will be returned.
  //
  // Unlike in some other systems like PostgreSQL, where non-locking reads do
  // not wait on conflicting locks, in CockroachDB, non-locking reads do wait
  // for conflicting locks to be released. Because of this, the lock_timeout
  // configuration applies to non-locking reads in read-write and read-only
  // transactions as well.

This is a different definition than Postgres's lock_timeout. How much trouble do you think that'll put us in?

I feel like most of the salient changes here are in the KV interface, no? I think we'd want a KV reviewer too.

Absolutely, this cuts across SQL and KV, so we'll want a KV reviewer on the first two commits. I had pulled those out into #68026 so they can be reviewed separately. This PR is really just about the third commit. But also, there's no rush on this, so no need to review before #68026 is merged and the first two commits are rebased away.

This is a different definition than Postgres's lock_timeout. How much trouble do you think that'll put us in?

If anything, I was thinking that not deviating from Postgres here would put us in trouble. It would lead to behavior where the lock_timeout would apply to queries like SELECT * FROM t FOR UPDATE but not to queries like SELECT * FROM t. So the former would abort when stuck on a lock for longer than the timeout, while the latter would hang indefinitely. I'm actually imagining the deviation to be one of the biggest reasons why people will want to use this because people are often surprised that reads block on writes in CRDB. But I'd love to get others' opinions.

Oh, sorry, I ignored your disclaimer about the first 2 commit by accident. Makes sense.

One thing that I'd like to call to @rafiss's attention:

  // lock_timeout specifies the maximum amount of time that the batch request
  // will wait while attempting to acquire a lock on a key or while blocking on
  // an existing lock in order to perform a non-locking read on a key. The time
  // limit applies separately to each lock acquisition attempt. If the timeout
  // elapses when waiting for a lock, a WriteIntentError will be returned.
  //
  // Unlike in some other systems like PostgreSQL, where non-locking reads do
  // not wait on conflicting locks, in CockroachDB, non-locking reads do wait
  // for conflicting locks to be released. Because of this, the lock_timeout
  // configuration applies to non-locking reads in read-write and read-only
  // transactions as well.

This is a different definition than Postgres's lock_timeout. How much trouble do you think that'll put us in?

Is the concern that people may be surprised to see non-locking read transactions get cancelled due to lock_timeout?

I don't know either way if this is a significant/painful divergence. My personal feeling based on no data is that this divergence is acceptable.

One thing that might go a long way to making it more acceptable is if we make two changes in the message:

• a different message for the two cases you describe: i.e., "attempting to acquire a lock" versus "blocking on an existing lock for a non-locking read"
• return an error hint that links to documentation about CockroachDB locking behavior. or even better, documentation about what the user should do to resolve this problem.

Is the concern that people may be surprised to see non-locking read transactions get cancelled due to lock_timeout?

My guess is that no more than those who may be surprised to see non-locking reads block on locks. And once people understand that non-locking reads block on locks (which isn't crazy in serializable isolation, it's just not how PG works), they'll be happy that this setting applies there as well.

a different message for the two cases you describe: i.e., "attempting to acquire a lock" versus "blocking on an existing lock for a non-locking read"

Good idea. I removed the "while locking row" language of the error message and made it more general.

return an error hint that links to documentation about CockroachDB locking behavior. or even better, documentation about what the user should do to resolve this problem.

This is also a great idea! Unfortunately, it doesn't look like we have any documentation about CockroachDB's locking behavior. cc. @rmloveland.

bors r+

Timed out.

bors r+

Build succeeded:

• GitHub CI (Cockroach)

Unfortunately, it doesn't look like we have any documentation about CockroachDB's locking behavior

@nvanbenschoten we have some stuff on concurrency control that was added to the Transaction Layer architecture docs in cockroachdb/docs#7020

But given that this PR adds a session var, do you mean that we should add something focused on SQL users? E.g., something similar to the PG locking docs? If so I can create an issue, just wanted to be sure I understand ~what is needed

Right now I think the only things on locking we have in SQL user docs are a few mentions scattered around Indexes, Oracle migration, and the SELECT FOR UPDATE docs.

E.g., something similar to the PG locking docs? If so I can create an issue, just wanted to be sure I understand ~what is needed

I think this would be a fantastic addition! I also think our discussion about "Transactions, locking, and concurrency control" on the Oracle migration doc is great. Have we considered adding a similar discussion to the Postgres migration page? I imagine that that would be a useful resource for people comparing Cockroach's behavior to Postgres'.

But given that this PR adds a session var, do you mean that we should add something focused on SQL users?

That all said, I don't think improving the documentation on locking is a firm prerequisite to documenting this new session var.