rfc: SELECT FOR UPDATE

[rytaft]

RFC regarding support of SELECT ... FOR UPDATE SQL syntax. See Issue #6583.

[cockroach-teamcity]

This change is 

[bdarnell]

---

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171024_select_for_update.md, line 32 at r1 (raw file):

error if it is unable to immediately lock all target rows. This is useful for latency-critical situations,
and could also be useful for auto-retrying transactions in CockroachDB.
`SELECT ... FOR UPDATE SKIP LOCKED` is another option, which returns only the rows that could be

Are the NOWAIT and SKIP LOCKED modifiers usable without FOR UPDATE?

---

docs/RFCS/20171024_select_for_update.md, line 184 at r1 (raw file):

simple row-level intents.  It's also not clear that customers would use `FOR UPDATE` with 
large ranges, so this may be an unneeded performance optimization.  Furthermore, range-level
locks would not support the `SKIP LOCKED` option very well, so if we want to add `SKIP LOCKED`

It's not clear to me why this would be the case. If this is a serious alternative it needs to be spelled out in more detail. (but I'm also OK moving ahead with writing intents. My experience is that FOR UPDATE is mostly used to select single rows)

---

Comments from Reviewable

[nvanbenschoten]

Review status: 0 of 1 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171024_select_for_update.md, line 124 at r1 (raw file):

basic functionality in which clients use `FOR UPDATE` to lock the rows returned by the query.
Initially we won't include the SKIP LOCKED or NOWAIT options, but it may be worth implementing
these at some point.

How would SELECT FOR UPDATE interact with views?

---

docs/RFCS/20171024_select_for_update.md, line 185 at r1 (raw file):

large ranges, so this may be an unneeded performance optimization.  Furthermore, range-level
locks would not support the `SKIP LOCKED` option very well, so if we want to add `SKIP LOCKED`
later, we would probably need to revert to row-level intents for these queries.

Another downside is that locking the entire range based on the predicate could result in locking rows that should not be observable by the SELECT. For instance, if the transaction performing the SELECT FOR UPDATE query over some range is at a lower timestamp than a later write within that range, the FOR UPDATE lock should not apply to the newly written row. I don't think this is any worse than the other problems with locking precision that we'll already have here though.

---

Comments from Reviewable

[rytaft]

Review status: 0 of 1 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171024_select_for_update.md, line 32 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Are the NOWAIT and SKIP LOCKED modifiers usable without FOR UPDATE?

Postgres doesn't allow them outside of the explicit locking clause. I don't think we should either, since it's not clear how they would behave. I can update the text to say that.

---

Comments from Reviewable

[rytaft]

Review status: 0 of 1 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171024_select_for_update.md, line 184 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

It's not clear to me why this would be the case. If this is a serious alternative it needs to be spelled out in more detail. (but I'm also OK moving ahead with writing intents. My experience is that FOR UPDATE is mostly used to select single rows)

I was thinking that SKIP LOCKED wouldn't work very well because it would be hard for many workers to share a table with only a few ranges, since each one would lock an entire range or more. But I think I'll take this text out because the semantics would still be correct. Nothing in the Postgres documentation promises that many workers can share the same table, only that locked rows are skipped.

I didn't include much detail since there is a lot of detail in the Revert Command RFC, but I'll add a more explicit link to it. I've added Spencer as a reviewer in case he thinks this is an option we should more seriously consider.

---

Comments from Reviewable

[rytaft]

Review status: 0 of 1 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171024_select_for_update.md, line 185 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Another downside is that locking the entire range based on the predicate could result in locking rows that should not be observable by the SELECT. For instance, if the transaction performing the SELECT FOR UPDATE query over some range is at a lower timestamp than a later write within that range, the FOR UPDATE lock should not apply to the newly written row. I don't think this is any worse than the other problems with locking precision that we'll already have here though.

Thanks! I'll add this.

---

Comments from Reviewable

[rytaft]

Review status: 0 of 1 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171024_select_for_update.md, line 124 at r1 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

How would SELECT FOR UPDATE interact with views?

Right now it looks like it would be ignored, similar to ORDER BY and LIMIT in views. See comment from @a-robinson in data_source.go:getViewPlan(). If ORDER BY and LIMIT are supported, FOR UPDATE would come for free.

---

Comments from Reviewable

[rytaft]

Thanks! I've made updates based on the comments. Waiting to hear if @spencerkimball has any additional comments about range locks.

---

Review status: 0 of 1 files reviewed at latest revision, 4 unresolved discussions.

---

Comments from Reviewable

[vivekmenezes]

LGTM

[vivekmenezes]

(avoiding write-skew)

[vivekmenezes]

"a later INSERT within that range", could make this easier to understand

[vivekmenezes]

Nicely argued!

[rytaft]

Review status: 0 of 1 files reviewed at latest revision, 7 unresolved discussions, some commit checks pending.

---

docs/RFCS/20171024_select_for_update.md, line 66 at r2 (raw file):

Previously, vivekmenezes wrote…

(avoiding write-skew)

Done.

---

Comments from Reviewable

[rytaft]

Review status: 0 of 1 files reviewed at latest revision, 7 unresolved discussions, some commit checks pending.

---

docs/RFCS/20171024_select_for_update.md, line 190 at r2 (raw file):

Previously, vivekmenezes wrote…

"a later INSERT within that range", could make this easier to understand

Done.

---

Comments from Reviewable

[rytaft]

Review status: 0 of 1 files reviewed at latest revision, 7 unresolved discussions, some commit checks pending.

---

docs/RFCS/20171024_select_for_update.md, line 192 at r2 (raw file):

Previously, vivekmenezes wrote…

Nicely argued!

Thanks!

---

Comments from Reviewable

[rytaft]

Final comment period starting now, lasting until mid-day Wednesday.

---

Review status: 0 of 1 files reviewed at latest revision, 7 unresolved discussions, some commit checks pending.

---

Comments from Reviewable

[knz]

nit: in the past we've not started the FCP until 1 week after initial submission of the RFC -- this leaves time to people who take a week vacation to not feel rushed to review when they are back.

---

Review status: 0 of 1 files reviewed at latest revision, 13 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171024_select_for_update.md, line 124 at r1 (raw file):

Previously, rytaft wrote…

Right now it looks like it would be ignored, similar to ORDER BY and LIMIT in views. See comment from @a-robinson in data_source.go:getViewPlan(). If ORDER BY and LIMIT are supported, FOR UPDATE would come for free.

If the responsibility to lay down the intents is at the row fetcher, then views would be automatically supported and I do not foresee special issues.

It would be nice however to analyze / report here in the RFC what pg does with views, as an additional point of consideration for the reviewer(s).

---

docs/RFCS/20171024_select_for_update.md, line 10 at r3 (raw file):

# Summary

Support the `SELECT ... FOR UPDATE` SQL syntax, which locks rows returned by the `SELECT`

Color me ignorant, but I do not understand what the word "locks" means in this context. Also, I notice it is not explained elsewhere in the RFC. This reviewer would be grateful for a refresher on the context.

---

docs/RFCS/20171024_select_for_update.md, line 22 at r3 (raw file):

Postgres.  Several third party products such as the [Quartz Scheduler](http://www.quartz-scheduler.org),
[OpenJPA](http://openjpa.apache.org) and [Liquibase](http://www.liquibase.org)
also rely on this feature, preventing some potential customers from switching to CockroachDB.

This section says "other databases support it", and "there are multiple variations of the thing"

However neither this section, nor the RFC as a whole actually:

• explains / reminds the reader what the feature is useful for.
• specifies what the user expectations are wrt this feature and how far the proposed feature matches these expectations.

---

docs/RFCS/20171024_select_for_update.md, line 26 at r3 (raw file):

In some cases, `SELECT ... FOR UPDATE` is required to maintain correctness when running CockroachDB
in `SNAPSHOT` mode.  `SELECT ... FOR UPDATE` is essentially a no-op when running in `SERIALIZABLE` mode, 
but it may still be useful for controlling lock ordering and avoiding deadlocks.

I'd like a clarification on this:

• under which circumstances can you get a deadlock without select for update?
• would these deadlocks occur with cockroachdb? if yes, how?
• what other databases have such deadlock problems?

---

docs/RFCS/20171024_select_for_update.md, line 37 at r3 (raw file):

The default behavior of `SELECT ... FOR UPDATE` is for the transaction to block if some of the
target rows are already locked by another transaction.  Note that it is not possible to use the
`NOWAIT` and `SKIP LOCKED` modifiers without `FOR { UPDATE | SHARE | ... }`.

Another thing that bugs me is that pg also functions with MVCC and therefore does not typically uses "locks". Therefore, the abstract concept of locks invoked by this feature/statement is probably translated by pg into something else in the code. I would like to understand more concretely what pg does, because that might inform how to appreciate the rest of the proposal in this RFC.

---

docs/RFCS/20171024_select_for_update.md, line 86 at r3 (raw file):

As a result, the `UPDATE employees ...` statement at the end of the transaction will not need
to acquire any additional locks.  Note that `FOR UPDATE` will have no effect if it is used in a
stand-alone query that is not part of any transaction.

Please clarify in this section (guide-level explanation) what happens in this case:

SELECT * FROM employees WHERE name LIKE '%Smith' FOR UPDATE;

while the transaction containing that statement is ongoing, should another transaction that only touches the rows with name Mary Poppins (no Smith) be allowed to proceed?

Consider separately:

1. what would users expect in that case, based on the documentation of the feature?
2. considering that the LIKE operator in this specific instance cannot translate the restriction into a key span, and therefore requires a full table lookup, are we OK with "locking" the entire table?
3. what would pg do?
4. extension of 2+3: in which cases is pg able to "lock" fewer rows than CockroachDB for a given query? then how likely is a difference in "accuracy" between pg and crdb a problem for users who request this feature?

I understand you have some words about this below (I also have comments there) but there must be a high-level explanation of this here too.

---

docs/RFCS/20171024_select_for_update.md, line 122 at r3 (raw file):

may be excessively complicated, and it's not clear that our customers actually need
it.  To avoid spending too much time on this, as mentioned above, we will probably just implement the most
basic functionality in which clients use `FOR UPDATE` to lock the rows returned by the query.

"to lock the rows returned by the query" -- this contradict the paragraph below.

• Here you suggest we let the query run, then take the "final" result rows of the entire query, and use that as input to the "locking function" (I fail to see exactly in the RFC how that would happen btw, so if that is indeed the behavior I'll have a subsequent request to clarify the mechanism)
• Below you suggest that the table readers (kvrowfetcher) would be in charge of laying down the intents, that is before any processing by the query (like filtering, aggregation, etc).

These two approaches are contradictory. Which one are you taking? Maybe this sentence is the one that is problematic and needs to be removed, so that the rest of the explanation below stands unimpeded.

---

docs/RFCS/20171024_select_for_update.md, line 138 at r3 (raw file):

be precise with locking in CockroachDB, since locking happens at the KV layer, and
predicates may be applied later.  This should not affect correctness, but could affect
performance if there is high contention.

See my various comments above.

I think the word "correctness" here is ... dangerous. The main problem I see is that nowhere so far in the RFC has the reader seen what "correct" even means -- because, as I explained first thing above, there is not yet any discussion of what the feature is intended to achieve and what user expectations are. The correctness only derives from the specification and user expectations -- as long as we don't know whether those two sources of information are flexible wrt precision, we can't derive the statement "lack of precision does not affect correctness".

And note that I am already yielding a lot of ground here. If we were to apply our usual relationship with pg docs, "whatever pg does is our spec" -- and therefore, we'd be constrained to be exactly as precise as pg, and therefore the RFC would fall short of this.

I'd strongly recommend finding various sources of information that spell out clearly what the intent is and why it's ok to over-lock, and possibly outline briefly / suggest that we must communicate to users that they might need to review their own uses of this feature, if they are already having it in their apps, accordingly. And seriously approach Andy and/or other PMs and ask them to contact our existing "close customers" and ask their opinion on this.

---

Comments from Reviewable

[andreimatei]

I think this RFC is missing a couple of things that prevent me from being able to review it:

1. It doesn't make it clear what people use these row locks / it makes contradictory statements about whether they should be no-ops in crdb's serializable mode or not.
   I think the locks are used for a couple of major reasons:

• increase the isolation guarantees of low isolation levels in an ad-hoc manner
• change the conflict resolution policy/preference between conflicting transactions
• use them as general-purpose advisory locks

The RFC should make it more clear what our position on each of these uses is, and how the proposal relates to them.

2. The RFC doesn't mention at all the advisory locks family of functions that postgres has (pg_advisory_xact_lock and friends). Those are, in my opinion, quite related to row locking both in uses and in the possible implementations, plus they're a contentious topic within our product, and so I'd be really good if this RFC would discuss them a bit and phrase crdb's stance on the topic.
   See sql: use Vault with CockroachDB #14988 and sql: fill out pg_advisory_lock stubs #13546

3. The RFC is a bit light on implementation details. It says that scans will leave intents, but do those intents need to be committed? Will this work with DistSQL at all?

---

Review status: 0 of 1 files reviewed at latest revision, 16 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171024_select_for_update.md, line 25 at r3 (raw file):

In some cases, `SELECT ... FOR UPDATE` is required to maintain correctness when running CockroachDB
in `SNAPSHOT` mode.  `SELECT ... FOR UPDATE` is essentially a no-op when running in `SERIALIZABLE` mode, 

well, is it a no-op or not?

---

docs/RFCS/20171024_select_for_update.md, line 64 at r3 (raw file):

i.e., no variation in locking strength, no specified tables, and no options for avoiding
waiting on locks.  Using `FOR UPDATE` will result in locking the rows returned by the `SELECT` query

nit: there's many double spaces in this document. Is that intended?

---

docs/RFCS/20171024_select_for_update.md, line 85 at r3 (raw file):

will lock the rows of all employees named John Smith at the beginning of the transaction.
As a result, the `UPDATE employees ...` statement at the end of the transaction will not need
to acquire any additional locks.  Note that `FOR UPDATE` will have no effect if it is used in a

the word "lock" is not one we generally use (at least, we haven't used it before). It's not clear what "any additional locks" means, and it's generally not clear what it means to "lock" a row in crdb. As such, I don't know what this example is telling me, and this "guide" section doesn't really guide me.
In postgres these things are defined at length, with whole chapters detailing the interactions between different types of locks. I'd move away from this vocabulary.

---

docs/RFCS/20171024_select_for_update.md, line 122 at r3 (raw file):

Previously, knz (kena) wrote…

"to lock the rows returned by the query" -- this contradict the paragraph below.

• Here you suggest we let the query run, then take the "final" result rows of the entire query, and use that as input to the "locking function" (I fail to see exactly in the RFC how that would happen btw, so if that is indeed the behavior I'll have a subsequent request to clarify the mechanism)
• Below you suggest that the table readers (kvrowfetcher) would be in charge of laying down the intents, that is before any processing by the query (like filtering, aggregation, etc).

These two approaches are contradictory. Which one are you taking? Maybe this sentence is the one that is problematic and needs to be removed, so that the rest of the explanation below stands unimpeded.

+1

---

Comments from Reviewable

[rytaft]

@knz - Makes sense - should I update the README in the RFC directory to mention this 1 week period?

@andreimatei - I've made some changes related to your point (1). Still working on (2) and (3).

---

Review status: 0 of 1 files reviewed at latest revision, 16 unresolved discussions, some commit checks pending.

---

docs/RFCS/20171024_select_for_update.md, line 10 at r3 (raw file):

Previously, knz (kena) wrote…

Color me ignorant, but I do not understand what the word "locks" means in this context. Also, I notice it is not explained elsewhere in the RFC. This reviewer would be grateful for a refresher on the context.

I've added one high-level sentence here, and a lot of detail in the motivation section. Please let me know if you'd like me to add more.

---

docs/RFCS/20171024_select_for_update.md, line 22 at r3 (raw file):

Previously, knz (kena) wrote…

This section says "other databases support it", and "there are multiple variations of the thing"

However neither this section, nor the RFC as a whole actually:

• explains / reminds the reader what the feature is useful for.
• specifies what the user expectations are wrt this feature and how far the proposed feature matches these expectations.

I added examples of how it's useful to prevent write skew anomalies and deadlocks. I've also added a paragraph explaining how this should match expectations. But related to your point below, I'll check with Andy about whether the locking precision issues will be a problem for users.

---

docs/RFCS/20171024_select_for_update.md, line 25 at r3 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

well, is it a no-op or not?

Fixed - it's not actually a no-op.

---

docs/RFCS/20171024_select_for_update.md, line 26 at r3 (raw file):

Previously, knz (kena) wrote…

I'd like a clarification on this:

• under which circumstances can you get a deadlock without select for update?
• would these deadlocks occur with cockroachdb? if yes, how?
• what other databases have such deadlock problems?

Added some examples to address these questions.

---

docs/RFCS/20171024_select_for_update.md, line 37 at r3 (raw file):

Previously, knz (kena) wrote…

Another thing that bugs me is that pg also functions with MVCC and therefore does not typically uses "locks". Therefore, the abstract concept of locks invoked by this feature/statement is probably translated by pg into something else in the code. I would like to understand more concretely what pg does, because that might inform how to appreciate the rest of the proposal in this RFC.

It seems that Postgres uses row-level locks to prevent concurrent updates to the same rows. My understanding is that the difference between 2PL and Postgres' MVCC implementation is that Postgres allows concurrent read/write conflicts, while 2PL uses shared and exclusive locks to prevent these conflicts. But I think both prevent write/write conflicts with locks.

---

docs/RFCS/20171024_select_for_update.md, line 64 at r3 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

nit: there's many double spaces in this document. Is that intended?

I've always put two spaces after periods out of habit, but I think you are right that style guides now discourage it. I've removed them :)

---

docs/RFCS/20171024_select_for_update.md, line 85 at r3 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

the word "lock" is not one we generally use (at least, we haven't used it before). It's not clear what "any additional locks" means, and it's generally not clear what it means to "lock" a row in crdb. As such, I don't know what this example is telling me, and this "guide" section doesn't really guide me.
In postgres these things are defined at length, with whole chapters detailing the interactions between different types of locks. I'd move away from this vocabulary.

I've added an explanation that "lock" in CockroachDB corresponds to "write intent"

---

docs/RFCS/20171024_select_for_update.md, line 86 at r3 (raw file):

Previously, knz (kena) wrote…

Please clarify in this section (guide-level explanation) what happens in this case:

SELECT * FROM employees WHERE name LIKE '%Smith' FOR UPDATE;

while the transaction containing that statement is ongoing, should another transaction that only touches the rows with name Mary Poppins (no Smith) be allowed to proceed?

Consider separately:

1. what would users expect in that case, based on the documentation of the feature?
2. considering that the LIKE operator in this specific instance cannot translate the restriction into a key span, and therefore requires a full table lookup, are we OK with "locking" the entire table?
3. what would pg do?
4. extension of 2+3: in which cases is pg able to "lock" fewer rows than CockroachDB for a given query? then how likely is a difference in "accuracy" between pg and crdb a problem for users who request this feature?

I understand you have some words about this below (I also have comments there) but there must be a high-level explanation of this here too.

Added this example.

---

docs/RFCS/20171024_select_for_update.md, line 122 at r3 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

+1

Changed it to say "lock the rows touched by the query".

---

docs/RFCS/20171024_select_for_update.md, line 138 at r3 (raw file):

Previously, knz (kena) wrote…

See my various comments above.

I think the word "correctness" here is ... dangerous. The main problem I see is that nowhere so far in the RFC has the reader seen what "correct" even means -- because, as I explained first thing above, there is not yet any discussion of what the feature is intended to achieve and what user expectations are. The correctness only derives from the specification and user expectations -- as long as we don't know whether those two sources of information are flexible wrt precision, we can't derive the statement "lack of precision does not affect correctness".

And note that I am already yielding a lot of ground here. If we were to apply our usual relationship with pg docs, "whatever pg does is our spec" -- and therefore, we'd be constrained to be exactly as precise as pg, and therefore the RFC would fall short of this.

I'd strongly recommend finding various sources of information that spell out clearly what the intent is and why it's ok to over-lock, and possibly outline briefly / suggest that we must communicate to users that they might need to review their own uses of this feature, if they are already having it in their apps, accordingly. And seriously approach Andy and/or other PMs and ask them to contact our existing "close customers" and ask their opinion on this.

I've added some clarification here. I'll also ask Andy about customer requirements per your suggestion.

---

Comments from Reviewable

[rytaft]

Review status: 0 of 1 files reviewed at latest revision, 16 unresolved discussions, some commit checks pending.

---

docs/RFCS/20171024_select_for_update.md, line 124 at r1 (raw file):

Previously, knz (kena) wrote…

If the responsibility to lay down the intents is at the row fetcher, then views would be automatically supported and I do not foresee special issues.

It would be nice however to analyze / report here in the RFC what pg does with views, as an additional point of consideration for the reviewer(s).

Added one sentence to say that Postgres supports these options.

---

Comments from Reviewable

[a-robinson]

Review status: 0 of 1 files reviewed at latest revision, 18 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171024_select_for_update.md, line 97 at r4 (raw file):

The first implementation of `FOR UPDATE` in CockroachDB will not include `NOWAIT` or `SKIP LOCKED` options.
It seems that some users want these features, but many would be satisfied with `FOR UPDATE` alone.

To be more specific, do any of the third party products that you looked at use them?

---

docs/RFCS/20171024_select_for_update.md, line 129 at r4 (raw file):

i.e., no variation in locking strength, no specified tables, and no options for avoiding
waiting on locks. Using `FOR UPDATE` will result in locking the rows touched by the `SELECT` query

Nit, but it might be worth calling out that it specifically only locks rows that already exist, and that preventing inserts matching the SELECT query is not desired. I went through this whole doc wondering how dummy write intents could block matching inserts, only to infer based on the discussion in the drawbacks section that we actually don't want to block matching inserts at a later timestamp.

---

Comments from Reviewable

[knz]

Yes maybe updating the README (in a separate PR) would be a good idea.

---

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 20 unresolved discussions, all commit checks successful.

---

docs/RFCS/20171024_select_for_update.md, line 12 at r4 (raw file):

Support the `SELECT ... FOR UPDATE` SQL syntax, which locks rows returned by the `SELECT`
statement. This pessimistic locking feature prevents concurrent transactions from updating
any of the locked rows until the locking transaction commits or aborts. Several potential customers

I'd like another sentence here on "why this is important".

Also, now you have written this down, I'd like to ask: what of different transaction priorities? In CockroachDb currently if there is a conflict and the 2nd transaction has a higher priority, the 1st transaction even if it has laid down intents already will be pushed out of the way. Is it OK, in your view, that the SELECT FOR UPDATE feature proposed here is only "locking" the rows for transactions of the same priority level or below? If so that needs to be considered in the discussion below.

---

docs/RFCS/20171024_select_for_update.md, line 40 at r4 (raw file):

each deducts $200 from an account, and then verifies that the new total is zero, using the other account
value that held when the snapshot was taken. Since neither update conflicts, both commit successfully,
leaving V1 = V2 = -$100, and V1 + V2 = -$200.

After this paragraph and before the next one, you should drive home what happens with SELECT FOR UPDATE and therefore why it prevents write skews.

Also a sentence like "... therefore, using SELECT FOR UPDATE lets users obtain some of the performance benefits of using SNAPSHOT isolation instead of SERIALIZABLE, without paying the price of write skews." which justifies the (business) value of doing this.

---

docs/RFCS/20171024_select_for_update.md, line 59 at r4 (raw file):

must acquire shared locks for reads in addition to exclusive locks for writes. But deadlocks like the
one shown above also happen in databases that use MVCC like PostgreSQL and CockroachDB, since writes must
acquire locks on all rows that will be updated. Postgres and many other systems detect deadlocks by

I do not see how this is true of CockroachDB: 1) CockroachDB only uses intents, no "locks" like pg 2) the scenario above in CockroachDB causes a txn conflict and an abort, not a deadlock.

Can you clarify?

(If you want to make the point that CockroachDB contains extra code already to avoid deadlock situations, then say so and explain what mechanism is actually used, to clarify.)

---

docs/RFCS/20171024_select_for_update.md, line 66 at r4 (raw file):

deadlock causes delays and aborted transactions.

`SELECT ... FOR UPDATE` will help avoid deadlocks by allowing transactions to aqcuire all of their locks

aqcuire -> acquire

Also I think this paragraph is the point in the discussion where you should start using the word "intent" instead of "lock", and continue doing so for the rest of the RFC.
You can signal to the reader you are transitioning from one word to the other by phrasing like this:

"SELECT FOR UPDATE will ... by allowing transaction to acquire their "locks" (intents in CockroachDB) up front"

then below

"Since T1 laid intents ("locked") on rows A and B at the start of the txn, the deadlock was prevented.

(although this specific example is problematic, as I outline/question in my comment below. But you can use this overall literary approach elsewhere too.)

Meanwhile, every time you write something about another engine that does use locks, then use the word "lock". This way, the reader of the text will gradually see with more clarity the difference between when you write about CockroachDB and when you write about something else.

---

docs/RFCS/20171024_select_for_update.md, line 82 at r4 (raw file):

Since T1 locked rows A and B at the start of the transaction, the deadlock was prevented.

Also, I do not believe this would cause a deadlock in CockroachDB currently. Have you observed this deadlock to be possible in CockroachDB already?

If so, this should also be filed as an issue on github, and that issue linked from the top of the RFC.

---

docs/RFCS/20171024_select_for_update.md, line 161 at r4 (raw file):

which key spans are affected. This lack of precision will be an issue for any predicate that
does not directly translate to particular key spans. This is a departure from Postgres, since Postgres
generally locks exactly the rows returned by the query.

It would do good to the RFC to either here or below outline how PostgreSQL achieves this. The reason why it's hard in CockroachDB is that we do not flow the primary key of rows being touched during the execution, from the point/node where the rows are read from tables to the point/node where the query's final results are known. If we did flow the PK details (at great throughput and memory expense, we we technically could), we'd be able to be as precise as postgres. I would be very curious to know how pg achieves this without paying the price we'd need to pay.

---

docs/RFCS/20171024_select_for_update.md, line 201 at r4 (raw file):

these at some point.

At the moment it is not possible to use `FOR UPDATE` in views (there will not be an error, but it will

"it is not possible to use FOR UPDATE in views."

I don't think that's true. Unless you take extra, non-trivial precautions, any FOR UPDATE specification in a view definition will be reloaded transparently every time the view is used and actually have an effect (laying down extra intents). So I'd say that unless the implementation makes extra effort to disable this behavior, FOR UPDATE will work with views with predictable results.

---

docs/RFCS/20171024_select_for_update.md, line 205 at r4 (raw file):

@a-robinson in [data_source.go:getViewPlan()](https://github.com/cockroachdb/cockroach/blob/5a6b4312a972b74b0af5de53dfdfb204dc0fd6d7/pkg/sql/data_source.go#L680). If `ORDER BY` and `LIMIT` are supported later, `FOR UPDATE` would come for free.
Postgres supports all of these options in views, since it supports any `SELECT` query, and re-runs
the query each time the view is used.

That's also what CockroachDB does.

---

Comments from Reviewable

[rytaft]

@tschottdorf - thanks for the comment. Does the above discussion about rewriting the FOR UPDATE query tree to use an updateNode change your opinion at all (with the assumption that this solution will eliminate the locking precision issue)?

Also, regarding your second point:

we lock only on the primary index and Postgres semantics imply that reads must wait.

I don't think that's true - Postgres is still MVCC-based and writes (including explicit row locks) shouldn't block reads. I just confirmed this with two Postgres terminals. FOR UPDATE in Postgres successfully blocks writes but not reads. It does block someone from running SELECT FOR SHARE, but since we're not implementing that feature I don't think we need to worry...

I agree that it's unfortunate that implementing FOR UPDATE with intents would make debugging more difficult. And I also agree that a simple solution is very appealing - it makes bugs less likely and saves developer hours. I still think laying down intents is probably closer to what our users would expect, but I don't feel strongly. @bdarnell, I think it's your call...

---

Review status: 0 of 1 files reviewed at latest revision, 27 unresolved discussions, all commit checks successful.

---

Comments from Reviewable

[bdarnell]

+1 to constraining the first version of SFU to "simple" queries where the PK is used in the WHERE clause (and maybe even limiting the kinds of predicates supported to == and IN). We may need to expand this later but we can start with a more restrictive implementation.

@andreimatei

Well, except that in the Quartz example, a row is used to lock an abstract resource; it is an advisory lock. I assumed to that row doesn't even exist in the DB, but that can't be because Postgres does indeed seem to allow two clients to lock the same non-existent row at the same time.

In Quartz, if the SFU returns no rows, it inserts the row and then re-runs the SFU, precisely because SFU only locks rows that exist.

@rytaft

I think this approach (1) would fix the issue of lock precision, and (2) would be less work since we wouldn't need to implement a new kv api. Do you think I am missing anything, @bdarnell?

So this would turn SFU into a two-step operation, a read followed by a write of the same values, right? This would have a bit of a performance penalty but I like the fact that we don't need to introduce as much new stuff to do it. I think at this point it's probably better to optimize for ease of implementation than for performance, though, so this sounds good to me.

@tschottdorf

If I were to summarize the main points in the RFC in favor of the "laying down
intents" flavor of locking, they would be
it is closer to the behavior Postgres users expect
it is useful in the context of snapshot isolation.

@spencerkimball also found a use for SFU in SERIALIZABLE transactions when testing high-contention scenarios - laying an intent early to get queuing behavior instead of restart behaviors is useful.

we lock only on the primary index and Postgres semantics imply that reads must wait.
But CockroachDB can easily serve reads from a suitable secondary index
covering the required primary index columns so that this isn't generally true.
So using FOR UPDATE as a RWMutex may not work as planned.

FOR UPDATE in Postgres successfully blocks writes but not reads

Hmm. Write intents in cockroach do block reads, so this has multiple surprising effects. First, we block more than postgres, which may be a surprise but at least it's a conservative surprise that won't introduce any new anomalies. Second, only some reads are blocked since some reads can be served from indexes.

Given that there is a simple alternative (roughly disallow FOR UPDATE in
SNAPSHOT and make it a no-op otherwise)

I do not think this alternative meets the requirements for this feature. FOR UPDATE is not necessary to prevent transactional anomalies in SERIALIZABLE transactions, but I still think that implementing it as a no-op would be too surprising since it does have reasonable uses. So I think we have to either A) give up and say that we can't implement FOR UPDATE in an acceptable way (note that SQL Server doesn't support FOR UPDATE, so it's not an absolute requirement, although this is a big hit to our postgres compatibility) or B) implement it by laying down intents and document the differences with postgres. I think the feature is still worth having so I would go forward with the implementation.

[andreimatei]

@spencerkimball also found a use for SFU in SERIALIZABLE transactions when testing high-contention scenarios - laying an intent early to get queuing behavior instead of restart behaviors is useful.

But getting queuing behavior only happens if the timestamps align monotonically. With the proposed implementation, as soon as a lower timestamp queues beyond a higher timestamp, the sucker will have to restart as soon as its woken up! (right?) And this is compounded by the fact that the order of our wake-ups is non-deterministic (I think?).
So, should we also be considering some form of bumping up the timestamp when possible when a txn wakes up from a blocked SFU?

[tbg]

@rytaft at first glance, I could imagine warming up to that alternative! There has been so much discussion here that I'd have to recollect all the arguments and see how they apply to it, but I think this might be the sweet spot due to the decreased complexity and scope (I assume we were planning to update all indexes, as this is what the vanilla SQL machinery would do). I also assume (but don't know) that that restricted version covers most "reasonable" usage.

I'm a bit worried that the mutual exclusion use case still is not addressed (and it does seem that we do want to address it or give up on the FOR UPDATE as a whole?). It seems that we'd be close to addressing it if the intent were laid down with high priority. Unfortunately, priorities are per-txn, so this would necessarily bump the whole txn to HIGH (plus we can't change the priority after the transaction has started, at least not trivially; so we couldn't use FOR UPDATE after the first INSERT/UPDATE unless the txn is already at high priority). There's the option of running a second txn just for the FOR UPDATE, but then we have to solve the problem of letting the real txn overwrite that one's intents, at which point it gets pretty hairy again.

---

Review status: 0 of 1 files reviewed at latest revision, 27 unresolved discussions, all commit checks successful.

---

Comments from Reviewable

[rytaft]

Hi all -

Based on our meeting today, I have updated the RFC to mark it as rejected and tried to capture all of our reasoning in the document. @bdarnell, @tschottdorf, @andreimatei, @knz, @vivekmenezes - please let me know if you think I missed anything or if you have other feedback.

Thanks!

---

Review status: 0 of 1 files reviewed at latest revision, 27 unresolved discussions, some commit checks pending.

---

docs/RFCS/20171024_select_for_update.md, line 338 at r7 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

The downside is also that it is so complex and all-interfering that we decided not to implement it.

Added this point.

---

docs/RFCS/20171024_select_for_update.md, line 340 at r7 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Your earlier examples about the employee table indicate that users could quickly and accidentally cover whole tables with these. For example, WHERE employee LIKE '%John Smith' instead WHERE employee = 'John Smith.

Added this point as a drawback for using scans to set intents.

---

Comments from Reviewable

[tbg]

Looks good! I wrote a summary earlier just to keep it fresh in my mind, which I'm dumping below in case you want to use any of it.

We realized that it is problematic how FOR UPDATE conflates two use cases
(advisory locks, and reducing transaction contention). This is troubling since
we can't reasonably implement the first usage properly, and that the second one
could be a potential pessimization when applied to CockroachDB.

The first point implied that we would require a sort of "opt-in" into FOR UPDATE as its usage does not communicate to the server whether performance or
mutual exclusion was the desired goal. It was noted that implementing this
opt-in implied extra complexity, and that we also weren't so sure about whether
performance gains to be had warranted the implementation at this stage.
Consequently, we'd need multiple "modes": a no-op mode and the actual mode.
Nobody really felt that there was enough data to suggest that this was presently
worth the effort. Additionally, to some extent the functionality can be emulated
today by executing an UPDATE x SET y=y WHERE y=y.

PS: rejected sounds pretty harsh and I hope that isn't what it feels like. I'd be in favor of introducing another state for this sort of outcome - we have something that we think may be worth implementing at some point, but decided the point isn't now. "postponed", "tabled"? cc @bdarnell

---

Review status: 0 of 1 files reviewed at latest revision, 27 unresolved discussions, all commit checks successful.

---

Comments from Reviewable

[a-robinson]

Does "rejected" mean we aren't going to support the FOR UPDATE syntax at all? Or does it mean that it's just going to be a no-op for serializable transactions?

Either way, kudos for kicking off all this good discussion and bringing it to a conclusion.

[knz]

Yes I concur that "later" or "postponed" is better.

[tbg]

@a-robinson it means that we won't support the syntax at all. Supporting the syntax as a no-op without an opt-in was deemed too dangerous as many folks use it as a stand-in for advisory locks. (As a precedent, we apparently implemented advisory lock functions as no-ops, but we agreed that that was done at a point in time when we weren't as rigorous as we are today).

[bdarnell]

"Postponed" SGTM.

[vivekmenezes]

LGTM

[rytaft]

Thanks, all. @tschottdorf thanks for your summary - I added a couple of sentences to the Drawbacks section with points I had missed.

I've updated the status to "postponed".

If I don't hear from anyone else, I'll merge this at the end of the day today.

---

Review status: 0 of 1 files reviewed at latest revision, 27 unresolved discussions, some commit checks pending.

---

Comments from Reviewable

[rytaft]

Review status: 0 of 1 files reviewed at latest revision, 27 unresolved discussions, some commit checks pending.

---

docs/RFCS/20171024_select_for_update.md, line 243 at r4 (raw file):

Previously, awoods187 (Andy Woods) wrote…

I'm pro holding off on the remaining features (beyond potentially for update) as we should be looking to customers to influence the additional workload and performance is a top priority

👍

---

Comments from Reviewable

[hoeghh]

Any status on this ?

[tbg]

@hoeghh I'm not aware that this is anywhere on our roadmap, though our PMs would know more. cc @awoods187

[knz]

@hoeghh the data model required to support the traditional (really: old-school) SQL locking facilities is that of a single-node database with shared, highly consistent memory.

Trying to implement this data model on top of a distributed data store (any, really, not just CockroachDB) would require serializing operations to a single lock manager shared by all nodes, thereby severely limiting performance and also creating a single point of failure. It is therefore antithetical to the overall resilience mission of CockroachDB, not to say of the general performance requirements of a cloud-native database.

So no, we're not planning to implement this, and would like to spend time with you studying your use case and find a different design for your SQL queries.

[awoods187]

I'll chime in to also ask, why do you want to use locking? I'm hopeful that we can support your need in another way

[hoeghh]

@knz @awoods187 Thanks for your answers. I have a big application that supports Postgresql, and i want to run that with cockroachdb instead. But they use the SELECT FOR UPDATE when initializing the database for some reason. I can't change the application as i don't own it. Was hoping it would work, which it didn't.

The application is Atlassian Jira.

[andreimatei]

An option that was discussed around this RFC is to allow clients to opt into a no-op implementation of FOR UPDATE. This would allow some apps to get what they wanted out of that clause (since crdb always runs txns with the SERIALIZABLE isolation which is what some applications really wanted). It would not be perfectly appropriate for other use cases.
This would probably not be too hard to do.
Jira is a big name, so it'd be cool to (see if we can) support it. @hoeghh do you happen to have any clues about to the extent that the application would work had it not been for this issue?

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @a-robinson, @andreimatei, @awoods187, @bdarnell, @knz, @nvanbenschoten, @tbg, and @vivekmenezes)

[knz]

Andrei actually JIRA combines FOR UPDATE (and locks in general) and relatively long lived txns and expects exceptions in concurrent txns accordingly. So implementing it as a no-op will break functionality in JIRA in strange and mysterious ways.
Same for any Hibernate app where a code path arrives at a use of FOR UPDATE -- these apps use frameworks that don't use SQL locks in general and when they do, they do it to provide information about txn interference to apps (or control over contention). By implementing a no-op we're violating the API guarantees of these frameworks, and thus throwing correctness out of the window for higher-level apps that use them.

I agree that in many cases the app should not be using the framework API that manipulates lock, and instead use some other API (and the frameworks should be modified to remove this legacy crap that requires locks, to force app developers to not use them) - however that would be a different kind of work.