opt: add NOWAIT to unlocked scans below NOWAIT #130103
Conversation
michae2
added
backport-23.2.x
|
This change is |
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @michae2 and @mw5h)
pkg/sql/opt/optbuilder/select.go line 548 at r1 (raw file):
WaitPolicy: waitPolicy, }, }}
[nit] Now that this logic has become fairly complex, do you think it's worth pulling it out into a separate method that can be called here and above?
There was a problem hiding this comment.
Nice!
Reviewed 1 of 3 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status:
-- commits line 15 at r2:
Where can I learn more about "NOWAIT false positives"?
(This is follow-up from cockroachdb#127718, basically a refinement of commit e74202c.) When using the new Lock operator to implement FOR UPDATE and FOR SHARE, we build unlocked scans that are then followed by a final Lock operator. If the Lock operator uses NOWAIT, however, similar to SKIP LOCKED, these unlocked scans still need to have the NOWAIT behavior to avoid blocking, even if they do not take locks themselves. Unlike SKIP LOCKED, however, we do _not_ keep the NOWAIT behavior on unlocked scans under weaker isolation levels. This is because NOWAIT false positives produce false positive locking errors, whereas SKIP LOCKED false positives are benign (and in fact a minor performance optimization in some cases). For example, consider the following scenario: ```sql CREATE TABLE xy (x INT PRIMARY KEY, y INT); INSERT INTO xy VALUES (1, 1), (2, 2); -- lock the x=1 row in the first session BEGIN; SELECT * FROM xy WHERE x = 1 FOR UPDATE; -- try to lock the y=2 row in the second session BEGIN; SELECT * FROM xy WHERE y = 2 FOR UPDATE NOWAIT; ``` Under serializable isolation, the final SELECT FOR UPDATE NOWAIT statement uses NOWAIT behavior in its initial full-table scan. This full-table scan first encounters the lock on x=1 which is a "false positive" in the sense that it is not the intended row to lock. This causes the statement to fail immediately. This might seem surprising, but under serializable isolation our only alternative is to instead block on the x=1 lock until the first transaction finishes. We choose to fail immediately (the NOWAIT behavior) instead of blocking because (a) it matches our behavior before this change, (b) it conceptually fits with the read-lock semantics of serializable isolation, in which we consider every read to acquire a shared lock, and (c) it preserves the expected serializable behavior in case the first transaction updates the x=1 row to have y=2. Under weaker isolation levels we do not use NOWAIT in the initial full-table scan to avoid this false positive error, instead reading "underneath" the lock and only trying to lock the y=2 row. If, instead of NOWAIT, we consider the final statement to use SKIP LOCKED, we can see that it doesn't hurt anything for us to always use the SKIP LOCKED behavior in the initial full table scan, even under weaker isolation levels. And sometimes it helps. Fixes: cockroachdb#129145 Release note (bug fix): Fix a bug in which some SELECT FOR UPDATE or SELECT FOR SHARE queries using NOWAIT could still block on locked rows when using `optimizer_use_lock_op_for_serializable` under Serializable isolation. This bug was present when `optimizer_use_lock_op_for_serializable` was introduced in v23.2.0.
There was a problem hiding this comment.
TFTRs! Test failure is unrelated.
bors r=DrewKimball,mgartner
Reviewable status:
Previously, mgartner (Marcus Gartner) wrote…
Where can I learn more about "NOWAIT false positives"?
Ah, sorry, I didn't explain this clearly. I've added to the commit message, hopefully it's more understandable now.
pkg/sql/opt/optbuilder/select.go line 548 at r1 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
[nit] Now that this logic has become fairly complex, do you think it's worth pulling it out into a separate method that can be called here and above?
I thought you would say this. 🙂 Done.
(This is follow-up from #127718, basically a refinement of commit e74202c.)
When using the new Lock operator to implement FOR UPDATE and FOR SHARE, we build unlocked scans that are then followed by a final Lock operator. If the Lock operator uses NOWAIT, however, similar to SKIP LOCKED, these unlocked scans still need to have the NOWAIT behavior to avoid blocking, even if they do not take locks themselves.
Unlike SKIP LOCKED, however, we do not keep the NOWAIT behavior on unlocked scans under weaker isolation levels. This is because NOWAIT false positives produce false positive locking errors, whereas SKIP LOCKED false positives are benign (and in fact a minor performance optimization in some cases).
For example, consider the following scenario:
Under serializable isolation, the final SELECT FOR UPDATE NOWAIT statement uses NOWAIT behavior in its initial full-table scan. This full-table scan first encounters the lock on x=1 which is a "false positive" in the sense that it is not the intended row to lock. This causes the statement to fail immediately. This might seem surprising, but under serializable isolation our only alternative is to instead block on the x=1 lock until the first transaction finishes. We choose to fail immediately (the NOWAIT behavior) instead of blocking because (a) it matches our behavior before this change, (b) it conceptually fits with the read-lock semantics of serializable isolation, in which we consider every read to acquire a shared lock, and (c) it preserves the expected serializable behavior in case the first transaction updates the x=1 row to have y=2.
Under weaker isolation levels we do not use NOWAIT in the initial full-table scan to avoid this false positive error, instead reading "underneath" the lock and only trying to lock the y=2 row.
If, instead of NOWAIT, we consider the final statement to use SKIP LOCKED, we can see that it doesn't hurt anything for us to always use the SKIP LOCKED behavior in the initial full table scan, even under weaker isolation levels. And sometimes it helps.
Fixes: #129145
Release note (bug fix): Fix a bug in which some SELECT FOR UPDATE or SELECT FOR SHARE queries using NOWAIT could still block on locked rows when using
optimizer_use_lock_op_for_serializableunder Serializable isolation. This bug was present whenoptimizer_use_lock_op_for_serializablewas introduced in v23.2.0.