sql: support FOR {UPDATE,SHARE} NOWAIT#52522
Conversation
|
This change is |
62ba834 to
89f5781
Compare
jordanlewis
left a comment
There was a problem hiding this comment.
Wow, this was surprisingly simple - most of the "meat" of the change happens somewhere in KV. Amazing!
How much more work is SKIPLOCKED? I'm guessing a lot? That one would be really useful for queue workloads.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @jordanlewis, and @nvanbenschoten)
pkg/sql/row/errors.go, line 172 at r2 (raw file):
var relation string if desc, ok := descForKey.KeyToDesc(wiErr.Intents[0].Key); ok { relation = fmt.Sprintf("relation %q", desc.Name)
I think we like to use the table@index form a lot and might be nice here, if we have enough information. Maybe we don't? But in general, the more information we can extract here the better, I think... unless you were just copying the Postgres message.
pkg/sql/row/errors.go, line 174 at r2 (raw file):
relation = fmt.Sprintf("relation %q", desc.Name) } else { relation = "interleaved relation"
can't wait to never have to see stuff like this ever again!
89f5781 to
0144b6a
Compare
nvb
left a comment
There was a problem hiding this comment.
How much more work is SKIPLOCKED? I'm guessing a lot? That one would be really useful for queue workloads.
Yeah, SKIP LOCKED will be significantly more work. Instead of just throwing an error when we detect that the owner of a conflicting lock is still active, we'd instead need to coordinate with the storage layer to ignore the value that is locked. This actually becomes feasible once the iterators that we use at the MVCC layer start capturing lock-table state (part of #41720), but it's still tricky.
SKIP LOCKED is also deliberately returning an inconsistent view of the table's data, but still with very specific semantics, so it's a little more questionable whether it's something we want to implement in the first place.
Reviewable status:
pkg/sql/row/errors.go, line 172 at r2 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
I think we like to use the table@index form a lot and might be nice here, if we have enough information. Maybe we don't? But in general, the more information we can extract here the better, I think... unless you were just copying the Postgres message.
This was just copying the Postgres message word for word.
pkg/sql/row/errors.go, line 174 at r2 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
can't wait to never have to see stuff like this ever again!
😃
We're not bound to the Postgres error messages necessarily. We like to copy them because often they're better than what we come up with. That being said, in this circumstance, can't we do significantly better? One of the biggest complaints customers tend to have about our transaction model is that they don't understand what conflicts are caused by when they get error messages. Can we do better in this circumstance? I'd love to see, if it were possible, the conflicting row, index and table. Overall this change LGTM though! Happy to see this go in. |
All fair points. We have the full key here, so we should be able to return as much information as we do with unique constraint violations. I'll try that out later this week and see how it goes. Thanks again for the review. |
0144b6a to
bdb0514
Compare
Done. I think this turned out nicely. PTAL when you get a chance. |
Fixes half of cockroachdb#40476. In cockroachdb#52388, we added support for a new `Error` wait policy to be attached to BatchRequests. This wait policy dictates that if a request encounters a conflicting locks held by another active transaction, it should raise an error instead of blocking. These semantics closely match those of FOR {UPDATE,SHARE} NOWAIT in PostgreSQL. With NOWAIT, the statement reports an error, rather than waiting, if a selected row cannot be locked immediately. This means that we are able to use the new kv-level wait policy to implement NOWAIT in SQL. Doing so is fairly mechanical, as a lot of the plumbing necessary was already put into place in cockroachdb#44429. The only difference I can see in the semantics is that in PostgreSQL, rows that are being INSERTed by one transaction are not even considered for locking by other transactions, so NOWAIT will not throw an error when encountering these rows. Instead, it will silently ignore them. This has less to do with NOWAIT itself, and more to do with a difference in the concurrency control implementation between Cockroach and PostgreSQL. Since this isn't specific to NOWAIT, I don't think it's a blocker for this change. Release note (sql change): SELECT ... FOR {UPDATE,SHARE} NOWAIT is now supported. The option can be used to throw and error instead of blocking on contended row-level lock acquisition.
bdb0514 to
7aed72c
Compare
|
bors r+ |
|
Build succeeded: |
Fixes half of #40476.
In #52388, we added support for a new
Errorwait policy to be attachedto BatchRequests. This wait policy dictates that if a request encounters
a conflicting locks held by another active transaction, it should raise
an error instead of blocking.
These semantics closely match those of FOR {UPDATE,SHARE} NOWAIT in
PostgreSQL. With NOWAIT, the statement reports an error, rather than
waiting, if a selected row cannot be locked immediately. This means that
we are able to use the new kv-level wait policy to implement NOWAIT in
SQL. Doing so is fairly mechanical, as a lot of the plumbing necessary
was already put into place in #44429.
The only difference I can see in the semantics is that in PostgreSQL,
rows that are being INSERTed by one transaction are not even considered
for locking by other transactions, so NOWAIT will not throw an error
when encountering these rows. Instead, it will silently ignore them.
This has less to do with NOWAIT itself, and more to do with a difference
in the concurrency control implementation between Cockroach and
PostgreSQL. Since this isn't specific to NOWAIT, I don't think it's a
blocker for this change.
Release note (sql change): SELECT ... FOR {UPDATE,SHARE} NOWAIT is now
supported. The option can be used to throw and error instead of blocking
on contended row-level lock acquisition.