Document nested SAVEPOINTs #6675

rmloveland · 2020-02-25T19:33:16Z

Summary of changes:

Update SAVEPOINT page with more examples of "general" savepoints, in addition to the existing "retry" savepoints
Add a new SHOW SAVEPOINT STATUS statement page, and add it to the sidebar
Update RELEASE SAVEPOINT docs to clarify "retry" savepoints vs. "general", and update the example

cockroach-teamcity · 2020-02-25T19:33:23Z

This change is

cockroach-teamcity · 2020-02-26T17:39:08Z

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/849f5f427f31c1bc19c88142e098feafa4f16542/

Edited pages:

cockroach-teamcity · 2020-02-28T21:09:18Z

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/3805e8c2ee87abf5e99f8f42c779679bfc47c5d3/

Edited pages:

cockroach-teamcity · 2020-02-28T21:16:19Z

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/74493f1667f931a21a4a995b7ec3ca3d850d753d/

Edited pages:

cockroach-teamcity · 2020-02-28T21:24:52Z

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/1f1dd1f7b7c1b361f17fc1cc7aa63f2098bd5350/

Edited pages:

rmloveland · 2020-02-28T21:33:38Z

Raphael, these doc changes are based on what I found on your knz/20191208-sql-savepoints branch thanks to cockroachdb/cockroach#43051

(I realize I'm a bit early. Trying to get ahead of the curve for the 20.1 release.)

If you think there are too many things that will eventually change based on your in-flight PR or the scheduled follow-up work you mentioned over there, we can hold off for a bit.

knz

The changes overall look good but are indeed preliminary.
I would recommend that you also pick up the various examples and specific notes mentioned in the RFC here: https://github.com/knz/cockroach/blob/20191014-rfc-savepoints/docs/RFCS/20191014_savepoints.md

Reviewed 9 of 9 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rmloveland)

_includes/v20.1/misc/customizing-the-savepoint-name.md, line 6 at r1 (raw file):

{{site.data.alerts.callout_danger}}
The `force_savepoint_restart` variable changes the semantics of CockroachDB savepoints so that `RELEASE SAVEPOINT <your-custom-name>` functions as a real commit. Note that the existence of this variable and its behavior does not change the fact that CockroachDB savepoints can only be used as a part of the transaction retry protocol.

I think this entire callout may need an adjustment after @andreimatei gets a say on the implementation. Not sure yet. Please keep it on the radar (we likely won't mention it in release notes).

v20.1/begin-transaction.md, line 47 at r1 (raw file):

#### Use default settings

Without modifying the `BEGIN` statement, the transaction uses `SERIALIZABLE` isolation and `NORMAL` priority.

I don't know if this change belongs to this PR?

knz · 2020-03-02T17:07:55Z

note that the RFC mentions rolling back DDL but this is not yet implemented
in your changes you say "rollback operates on non-DDL statements". I think this does not convey the intention properly. The proper description is "rollback operates on any statement" and SEPARATELY "known limitation: DDL rollbacks are not supported yet" (they will be in the future).

cockroach-teamcity · 2020-03-04T19:35:33Z

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/60edb97e90c1ad8e5807ca6e8a036b32e714e9f6/

Edited pages:

rmloveland

Thanks for taking a look Raphael!

The changes overall look good but are indeed preliminary.

Duly noted

in your changes you say "rollback operates on non-DDL statements". I think this does not convey the intention properly. The proper description is "rollback operates on any statement" and SEPARATELY "known limitation: DDL rollbacks are not supported yet" (they will be in the future).

Updated the PR with this info, on both the SAVEPOINT and ROLLBACK pages.

I would recommend that you also pick up the various examples and specific notes mentioned in the RFC here: https://github.com/knz/cockroach/blob/20191014-rfc-savepoints/docs/RFCS/20191014_savepoints.md

I have gone back over the "Guide-level explanation" from the RFC and adapted the examples there more explicitly to the SAVEPOINT page. You will see that I added but commented out an example based on the Behavior in case of errors section since that does not work yet in the binary I built. (But I will revisit as you mentioned.)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @knz)

_includes/v20.1/misc/customizing-the-savepoint-name.md, line 6 at r1 (raw file):

Previously, knz (kena) wrote…

I think this entire callout may need an adjustment after @andreimatei gets a say on the implementation. Not sure yet. Please keep it on the radar (we likely won't mention it in release notes).

Noted. Will keep it on the radar for now.

v20.1/begin-transaction.md, line 47 at r1 (raw file):

Previously, knz (kena) wrote…

I don't know if this change belongs to this PR?

Unrelated edit. Will remove from this PR.

knz

Some general comments:

I would strongly recommend creating a new "concept" page that explains sub-transactions.
I might suggest to split the docs for COMMIT/ROLLBACK for an entire transaction, from the docs for RELEASE/ROLLBACK TO SAVEPOINT for sub-transactions.
(this is an unrelated issue: I just noticed the docs for the COMMIT statement do not explain that COMMIT is equivalent to ROLLBACK after an error.)

Reviewed 1 of 9 files at r1, 4 of 4 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @rmloveland)

_includes/v20.1/misc/customizing-the-savepoint-name.md, line 6 at r1 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Noted. Will keep it on the radar for now.

I think the entire callout can be removed now.

v20.1/release-savepoint.md, line 3 at r2 (raw file):

---
title: RELEASE SAVEPOINT
summary: Remove all open savepoints from the current transaction with the RELEASE SAVEPOINT statement in CockroachDB.

summary: Commit a sub-transaction.

(No need for more words)

v20.1/release-savepoint.md, line 8 at r2 (raw file):

The `RELEASE SAVEPOINT` statement causes all open savepoints back to and including the named savepoint to be removed from the transaction stack.

"
The RELEASE SAVEPOINT statement commits the sub-transaction starting at the corresponding SAVEPOINT statement using the same savepoint name, including all its nested sub-transactions.
"

v20.1/release-savepoint.md, line 9 at r2 (raw file):

The `RELEASE SAVEPOINT` statement causes all open savepoints back to and including the named savepoint to be removed from the transaction stack.

It functions as a [`COMMIT`](commit-transaction.html) when it releases the outermost savepoint in a stack of nested savepoints. This includes when it is used as part of the legacy [client-side transaction retry protocol](transactions.html#client-side-intervention). Note that although issuing this statement commits the transaction, you must also issue a subsequent [`COMMIT`](commit-transaction.html) statement to prepare the connection for the next transaction.

This entire paragraph is not necessary any more since Andrei completed his recent work.

v20.1/release-savepoint.md, line 11 at r2 (raw file):

It functions as a [`COMMIT`](commit-transaction.html) when it releases the outermost savepoint in a stack of nested savepoints. This includes when it is used as part of the legacy [client-side transaction retry protocol](transactions.html#client-side-intervention). Note that although issuing this statement commits the transaction, you must also issue a subsequent [`COMMIT`](commit-transaction.html) statement to prepare the connection for the next transaction.

If statements in the transaction [generated any non-retry errors](transactions.html#error-handling), `RELEASE SAVEPOINT` is equivalent to [`ROLLBACK`](rollback-transaction.html), which aborts the transaction and discards all updates made by its statements.

This is not true any more.

Try this:
"""
The RELEASE SAVEPOINT statement is invalid after the sub-transaction has encountered an error. After an error, the ROLLBACK TO SAVEPOINT statement must be used instead.
When a (sub-)transaction encounters a retry error, the client should repeat ROLLBACK TO SAVEPOINT and the statements in the transaction until the statements complete without error, then issue RELEASE.

To completely remove the marker of a sub-transaction after it encounters an error and begin other work in the outer transaction, use ROLLBACK TO SAVEPOINT immediately followed by RELEASE.
"""

v20.1/release-savepoint.md, line 31 at r2 (raw file):

## Customizing the name of a retry savepoint

{% include {{ page.version.version }}/misc/customizing-the-savepoint-name.md %}

This entire section should be removed for 20.1 - all savepoints can serve as retry savepoints now.

v20.1/rollback-transaction.md, line 3 at r2 (raw file):

---
title: ROLLBACK
summary: Abort the current transaction, discarding all updates made by statements included in the transaction with the ROLLBACK statement in CockroachDB.

summary: Rolls back the current transaction or sub-transaction, discarding all transactional updates made by statements inside the transaction.

Note the word "transactional" here: certain updates are not transactional and thus are not reverted by ROLLBACK. This includes (but not limited to):

sequence increments with nextval()
session variable updates
cluster setting updates

v20.1/rollback-transaction.md, line 7 at r2 (raw file):

---

The `ROLLBACK` [statement](sql-statements.html) aborts the current [transaction](transactions.html), discarding all updates made by statements included in the transaction.

"rolls back the current transaction or sub-transaction, ..."

v20.1/rollback-transaction.md, line 9 at r2 (raw file):

The `ROLLBACK` [statement](sql-statements.html) aborts the current [transaction](transactions.html), discarding all updates made by statements included in the transaction.

When used with [savepoints](savepoint.html), rollbacks cause the effects of all statements in the transaction issued after the savepoint to be discarded.  For more information, see [the documentation for `SAVEPOINT`](savepoint.html).

"
The base ROLLBACK syntax rolls back the entire transaction.

The ROLLBACK TO SAVEPOINT syntax rolls back and restarts the sub-transaction started at the corresponding SAVEPOINT statement. For more details, ...
"

v20.1/rollback-transaction.md, line 29 at r2 (raw file):

 Parameter | Description
-----------|-------------
 `TO SAVEPOINT cockroach_restart` | If using [advanced client-side transaction retries](advanced-client-side-transaction-retries.html), retry the transaction. You should execute this statement when a transaction returns a `40001` / `retry transaction` error.

This table needs to be updated. The name cockroach_restart is not special any more.

v20.1/savepoint.md, line 3 at r2 (raw file):

---
title: SAVEPOINT
summary: Define a SAVEPOINT within the current transaction

summary: Start a sub-transaction.

v20.1/savepoint.md, line 7 at r2 (raw file):

---

A savepoint is a placeholder inside a transaction that allows all statements that are executed after the savepoint to be [rolled back](rollback-transaction.html). When rolled back, the database is restored to the state that existed prior to the savepoint. Clients can use multiple nested savepoints in a transaction.

"A savepoint is a marker that defines the beginning of a sub-transaction. This marker can be later used to commit or roll back just the effects of the sub-transaction without affecting the progress of the enclosing (sub-)transaction. Sub-transactions can be nested."

v20.1/savepoint.md, line 219 at r2 (raw file):

### Savepoints for client-side transaction retries

If you intend to implement [advanced client-side transaction retries](advanced-client-side-transaction-retries.html), after you `BEGIN` the transaction, you must create an outermost savepoint named `cockroach_restart`. This will identify that if the transaction contends with another transaction for resources and "loses", you intend to use [client-side transaction retries](transactions.html#transaction-retries).

The name cockroach_restart is not necessary any more.

v20.1/savepoint.md, line 265 at r2 (raw file):

~~~
  savepoint_name | is_restart_savepoint

The name of the column was changed from is_restart_savepoint to is_initial_savepoint.

v20.1/savepoint.md, line 272 at r2 (raw file):

~~~

Note that the `is_restart_savepoint` column will be true if the savepoint is [being used to implement client-side transaction retries](#savepoints-for-client-side-transaction-retries).

This sentence needs to be updated. I don't exactly know how though, maybe @andreimatei has a suggestion.

v20.1/show-savepoint-status.md, line 20 at r2 (raw file):

------|------------
`savepoint_name` | The name of the savepoint.
`is_restart_savepoint` | Whether the savepoint is [being used to implement client-side transaction retries](savepoint.html#savepoints-for-client-side-transaction-retries).

Column name was updated.

v20.1/show-savepoint-status.md, line 44 at r2 (raw file):

~~~

Remove the savepoint from the transaction with [`RELEASE SAVEPOINT`](release-savepoint.html), and exit the transaction with [`COMMIT`](commit-transaction.html):

This sentence must be updated to explain "commit a sub-transaction", "restart a sub-transaction".

The word "exit" is not appropriate.

rmloveland

Thanks Raphael! I have made a bunch of updates (details below). RFAL at your convenience.

I would strongly recommend creating a new "concept" page that explains sub-transactions.

I have created a new "sub-transactions" section of the 'Transactions' page that explains them at a high level. I would appreciate your feedback on it.

I might suggest to split the docs for COMMIT/ROLLBACK for an entire transaction, from the docs for RELEASE/ROLLBACK TO SAVEPOINT for sub-transactions.

I think this distinction is now covered in the new "sub-transactions" section I mentioned above, and also (hopefully?) in the examples.

(this is an unrelated issue: I just noticed the docs for the COMMIT statement do not explain that COMMIT is equivalent to ROLLBACK after an error.)

In the 19.2 and 20.1 COMMIT statement docs there is this bit that I think explains it? Or am I misunderstanding this?

For non-retryable transactions, if statements in the transaction generated any errors, COMMIT is equivalent to ROLLBACK, which aborts the transaction and discards all updates made by its statements.

Finally, in the latest round of changes I have added diagrams for SHOW SAVEPOINT STATUS based on your PR in cockroachdb/cockroach#45794 (I will PR my additional changes to the CRDB diagram generator ASAP), as well as revising all uses of cockroach_restart that I could find in the 20.1 docs, since that is no longer a special/meaningful name.

Finally, thanks again very much for your helpful review and suggestions on this PR!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @knz)

_includes/v20.1/misc/customizing-the-savepoint-name.md, line 6 at r1 (raw file):

Previously, knz (kena) wrote…

I think the entire callout can be removed now.

I've removed this callout from everywhere it was used in the docs.

v20.1/release-savepoint.md, line 3 at r2 (raw file):

Previously, knz (kena) wrote…

summary: Commit a sub-transaction.

(No need for more words)

Updated.

v20.1/release-savepoint.md, line 8 at r2 (raw file):

Previously, knz (kena) wrote…

"
The RELEASE SAVEPOINT statement commits the sub-transaction starting at the corresponding SAVEPOINT statement using the same savepoint name, including all its nested sub-transactions.
"

Updated to use that language, with links to the 'sub-transactions' section of the transactions doc.

v20.1/release-savepoint.md, line 9 at r2 (raw file):

Previously, knz (kena) wrote…

This entire paragraph is not necessary any more since Andrei completed his recent work.

Deleted. :-)

v20.1/release-savepoint.md, line 11 at r2 (raw file):

Previously, knz (kena) wrote…

This is not true any more.

Try this:
"""
The RELEASE SAVEPOINT statement is invalid after the sub-transaction has encountered an error. After an error, the ROLLBACK TO SAVEPOINT statement must be used instead.
When a (sub-)transaction encounters a retry error, the client should repeat ROLLBACK TO SAVEPOINT and the statements in the transaction until the statements complete without error, then issue RELEASE.

To completely remove the marker of a sub-transaction after it encounters an error and begin other work in the outer transaction, use ROLLBACK TO SAVEPOINT immediately followed by RELEASE.
"""

Updated to use this language.

v20.1/release-savepoint.md, line 31 at r2 (raw file):

Previously, knz (kena) wrote…

This entire section should be removed for 20.1 - all savepoints can serve as retry savepoints now.

Removed this section from this page, and the other pages where it was used.

v20.1/rollback-transaction.md, line 3 at r2 (raw file):

Previously, knz (kena) wrote…

summary: Rolls back the current transaction or sub-transaction, discarding all transactional updates made by statements inside the transaction.

Note the word "transactional" here: certain updates are not transactional and thus are not reverted by ROLLBACK. This includes (but not limited to):

sequence increments with nextval()

session variable updates

cluster setting updates

Thanks! I have updated the summary, and the first sentence of the page with this description.

v20.1/rollback-transaction.md, line 7 at r2 (raw file):

Previously, knz (kena) wrote…

"rolls back the current transaction or sub-transaction, ..."

Updated, also linking to sub-transactions description

v20.1/rollback-transaction.md, line 9 at r2 (raw file):

Previously, knz (kena) wrote…

"
The base ROLLBACK syntax rolls back the entire transaction.

The ROLLBACK TO SAVEPOINT syntax rolls back and restarts the sub-transaction started at the corresponding SAVEPOINT statement. For more details, ...
"

Updated.

v20.1/rollback-transaction.md, line 29 at r2 (raw file):

Previously, knz (kena) wrote…

This table needs to be updated. The name cockroach_restart is not special any more.

Glad to hear it! I have updated this to say TO SAVEPOINT <name>.

v20.1/savepoint.md, line 3 at r2 (raw file):

Previously, knz (kena) wrote…

summary: Start a sub-transaction.

Updated.

v20.1/savepoint.md, line 7 at r2 (raw file):

Previously, knz (kena) wrote…

"A savepoint is a marker that defines the beginning of a sub-transaction. This marker can be later used to commit or roll back just the effects of the sub-transaction without affecting the progress of the enclosing (sub-)transaction. Sub-transactions can be nested."

Updated.

v20.1/savepoint.md, line 219 at r2 (raw file):

Previously, knz (kena) wrote…

The name cockroach_restart is not necessary any more.

Removed reference to that name.

v20.1/savepoint.md, line 265 at r2 (raw file):

Previously, knz (kena) wrote…

The name of the column was changed from is_restart_savepoint to is_initial_savepoint.

Changed to is_initial_savepoint here and elsewhere

v20.1/savepoint.md, line 272 at r2 (raw file):
I have updated it to say

Note that the is_initial_savepoint column will be true if the savepoint is the outermost savepoint in the transaction.

and left the mention of retries etc. to the (many) other sections of the docs where they are discussed.

v20.1/show-savepoint-status.md, line 20 at r2 (raw file):
Updated the column name and description to:

is_initial_savepoint | Whether the savepoint is the outermost savepoint in the transaction.

v20.1/show-savepoint-status.md, line 44 at r2 (raw file):

Previously, knz (kena) wrote…

This sentence must be updated to explain "commit a sub-transaction", "restart a sub-transaction".

The word "exit" is not appropriate.

I rewrote this section to remove the use of "exit" and explain more about "commit a sub-transaction" and "restart a sub-transaction". PTAL.

cockroach-teamcity · 2020-03-12T19:27:37Z

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/ba35ff312b9a9d0a7b708ec2331f33f46204e52a/

Edited pages:

knz

This is going almost good, I think only the following two need attention:

more clearly separate ROLLBACK and ROLLBACK TO SAVEPOINT
integrate the new knowledge from sql: ROLLBACK TO SAVEPOINT does not release locks cockroach#46075 in all the proper places.

Reviewed 14 of 14 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rmloveland)

v20.1/advanced-client-side-transaction-retries.md, line 35 at r3 (raw file):

1. The transaction starts with the [`BEGIN`](begin-transaction.html) statement.

2. The [`SAVEPOINT`](savepoint.html) statement declares the intention to retry the transaction in the case of contention errors. It must be executed after [`BEGIN`](begin-transaction.html) but before the first statement that manipulates a database.  In other words, it must be the outermost statement in an XXX: YOU ARE HERE.

What's this XXX?

v20.1/rollback-transaction.md, line 3 at r3 (raw file):

---
title: ROLLBACK
summary: Rolls back the current transaction or sub-transaction, discarding all transactional updates made by statements inside the transaction.

see my comment below

v20.1/rollback-transaction.md, line 7 at r3 (raw file):

---

The `ROLLBACK` [statement](sql-statements.html) aborts the current [transaction](transactions.html) or [sub-transaction](transactions.html#sub-transactions), discarding all transactional updates made by statements included in the transaction.

This sentence, as is, is incorrect. The true sentence is "The ROLLBACK statement aborts the current txn and all its sub-transactions."

However, the confusion exists because ROLLBACK and ROLLBACK TO SAVEPOINT are two separate, somewhat unrelated statements.

It may be OK to provide a single doc page for both, but my preference would be to separate them, since you've already separated COMMIT and RELEASE SAVEPOINT, and the separation between ROLLBACK and ROLLBACK TO SAVEPOINT is equivalent to COMMIT/RELEASE.

In any case they should be documented as separate statements even if there's a single doc page.

andreimatei

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rmloveland)

v20.1/advanced-client-side-transaction-retries.md, line 27 at r3 (raw file):

~~~ sql
> BEGIN;                                  -- #1
> SAVEPOINT cockroach_restart;            -- #2

I haven't gone through this PR yet, but just in case this catches something - the savepoint named cockroach_restart is still special. It's still the only one that should be used in the client-side retry protocol. It is the only one for which a RELEASE have special semantics of actually attempting to commit the txn and returning deferred serializability errors. The release of other savepoints doesn't fail.
Other savepoints placed at the beginning of a transaction also have the property that you can always rollback to them, but don't have special release semantics.

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @rmloveland)

v20.1/advanced-client-side-transaction-retries.md, line 27 at r3 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I haven't gone through this PR yet, but just in case this catches something - the savepoint named cockroach_restart is still special. It's still the only one that should be used in the client-side retry protocol. It is the only one for which a RELEASE have special semantics of actually attempting to commit the txn and returning deferred serializability errors. The release of other savepoints doesn't fail.
Other savepoints placed at the beginning of a transaction also have the property that you can always rollback to them, but don't have special release semantics.

Huh are you sure you want to keep that? What's the rationale?

andreimatei

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @rmloveland)

v20.1/advanced-client-side-transaction-retries.md, line 27 at r3 (raw file):

Previously, knz (kena) wrote…

Huh are you sure you want to keep that? What's the rationale?

Well you can't generally commit on a RELEASE... The transaction is supposed to keep going on.

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @rmloveland)

v20.1/advanced-client-side-transaction-retries.md, line 27 at r3 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Well you can't generally commit on a RELEASE... The transaction is supposed to keep going on.

That's my point - the client is going to send COMMIT afterwards in any case. Why prematurely commit upon RELEASE?

andreimatei

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @rmloveland)

v20.1/advanced-client-side-transaction-retries.md, line 27 at r3 (raw file):

Previously, knz (kena) wrote…

That's my point - the client is going to send COMMIT afterwards in any case. Why prematurely commit upon RELEASE?

The idea is that, if THE client gets an error from the RELEASE, it doesn't COMMIT. It sends a ROLLBACK TO SAVEPOINT cockroach_restart. That's the point of this hacky protocol - to give a way to clients of finding out about errors without them actually sending a COMMIT (cause if they sent a commit, then we'd have to release all tht locks)

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rmloveland)

v20.1/advanced-client-side-transaction-retries.md, line 27 at r3 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

The idea is that, if THE client gets an error from the RELEASE, it doesn't COMMIT. It sends a ROLLBACK TO SAVEPOINT cockroach_restart. That's the point of this hacky protocol - to give a way to clients of finding out about errors without them actually sending a COMMIT (cause if they sent a commit, then we'd have to release all tht locks)

Dang it I had forgotten that.

@rmloveland this means that some of the things I commented here and elsewhere in this PR were mistaken, and some of the changes in this PR need to be reverted. I'm going to populate a few examples in the RFC (and the internal crdb test suite) and then we can revisit this docs PR.

rmloveland · 2020-03-16T15:26:14Z

this means that some of the things I commented here and elsewhere in this PR were mistaken, and some of the changes in this PR need to be reverted. I'm going to populate a few examples in the RFC (and the internal crdb test suite) and then we can revisit this docs PR.

OK thanks @knz . I will pause on this for a bit and revisit when you tell me the RFC examples and test suite are ready.

rmloveland · 2020-03-16T15:27:39Z

(PS thanks @andreimatei for your review. I look forward to updating this soon with your feedback.)

rmloveland

Thank you, Raphael!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @knz)

v20.1/advanced-client-side-transaction-retries.md, line 51 at r5 (raw file):

Previously, knz (kena) wrote…

I think you're making a hyperlink from this section to itself?

In a movie this is where my line would be "I meant to do that".

However, I did not in fact mean to do that. :-)

Fixed by removing the self-linkage.

v20.1/release-savepoint.md, line 31 at r5 (raw file):

In case of error, COMMIT is synonymous with ROLLBACK/ABORT and also rolls back the entire transaction.

Added - thanks!

cockroach-teamcity · 2020-04-06T18:49:45Z

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/1dec38d7b67b28e7b2faed0eb300b783c3fba096/

Edited pages:

rmloveland · 2020-04-09T19:12:49Z

@andreimatei do you have any additional changes you'd like to this PR?

If not, I'd like to pass this along for the Docs team to review.

lnhsingh

Overall, LGTM - nice job! Added a few small edits + adding > to code blocks

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @knz, and @rmloveland)

_includes/v20.1/sql/retry-savepoints.md, line 1 at r6 (raw file):

A savepoint defined with the name `cockroach_restart` is a "retry savepoint" and is used to implement [advanced client-side transaction retries](advanced-client-side-transaction-retries.html).  For more information, see [Retry savepoints](advanced-client-side-transaction-retries.html#retry-savepoints).

Remove the double space here

v20.1/advanced-client-side-transaction-retries.md, line 51 at r5 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

In a movie this is where my line would be "I meant to do that".

However, I did not in fact mean to do that. :-)

Fixed by removing the self-linkage.

Remove double space here (and below)

v20.1/advanced-client-side-transaction-retries.md, line 35 at r6 (raw file):

1. The transaction starts with the [`BEGIN`](begin-transaction.html) statement.

2. The [`SAVEPOINT`](savepoint.html) statement shown here is a [retry savepoint](#retry-savepoints); that is, it declares the intention to retry the transaction in the case of contention errors. It must be executed after [`BEGIN`](begin-transaction.html) but before the first statement that manipulates a database.  Although [nested transactions](savepoint.html#savepoints-for-nested-transactions) are supported in versions of CockroachDB >= 20.1, a retry savepoint must be the outermost savepoint in a transaction.

Add comma after BEGIN and remove the double space. Also, I'd suggest writing out "...nested transactions are supported in CockroachDB v20.1 and beyond..." instead of ">= 20.1"

v20.1/release-savepoint.md, line 31 at r5 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

In case of error, COMMIT is synonymous with ROLLBACK/ABORT and also rolls back the entire transaction.

Added - thanks!

Is ABORT a supported statement? If not, I'd just change it to say ROLLBACK. If it is, I think it should say "ROLLBACK / ABORT" (nit: slash is not monospaced)

v20.1/release-savepoint.md, line 49 at r6 (raw file):

{% include copy-clipboard.html %}
~~~ sql
BEGIN;

nit: add > to beginning of code block

v20.1/release-savepoint.md, line 71 at r6 (raw file):

{% include copy-clipboard.html %}
~~~ sql
BEGIN;

Add >

v20.1/rollback-transaction.md, line 9 at r6 (raw file):

The `ROLLBACK` [statement](sql-statements.html) aborts the current [transaction](transactions.html) and all of its [nested transactions](transactions.html#nested-transactions), discarding all transactional updates made by statements included in the transaction.

`ROLLBACK` comes in two flavors:

"two flavors" seems a little too colloquial. I suggest saying something like "There are two ways to suse ROLLBACK:"

but I'll leave it up to you! Feel free to ignore me if you disagree.

v20.1/savepoint.md, line 37 at r6 (raw file):

{% include copy-clipboard.html %}
~~~ sql
CREATE TABLE kv (k INT PRIMARY KEY, v INT);

Add >

v20.1/savepoint.md, line 46 at r6 (raw file):

{% include copy-clipboard.html %}
~~~ sql
SAVEPOINT foo;

Add >

v20.1/savepoint.md, line 57 at r6 (raw file):

{% include copy-clipboard.html %}
~~~ sql
ROLLBACK TO SAVEPOINT foo;

Add >

v20.1/savepoint.md, line 64 at r6 (raw file):

{% include copy-clipboard.html %}
~~~ sql
RELEASE SAVEPOINT foo;

Add >

v20.1/savepoint.md, line 71 at r6 (raw file):

{% include copy-clipboard.html %}
~~~ sql
BEGIN;

Add >

v20.1/savepoint.md, line 98 at r6 (raw file):

{% include copy-clipboard.html %}
~~~ sql
BEGIN;

Add >

v20.1/savepoint.md, line 115 at r6 (raw file):

{% include copy-clipboard.html %}
~~~ sql
BEGIN;

Add >

v20.1/savepoint.md, line 132 at r6 (raw file):

{% include copy-clipboard.html %}
~~~ sql
BEGIN;

Add >

v20.1/savepoint.md, line 159 at r6 (raw file):

{% include copy-clipboard.html %}
~~~ sql
BEGIN;

Add >

v20.1/savepoint.md, line 171 at r6 (raw file):

{% include copy-clipboard.html %}
~~~ sql
SHOW TRANSACTION STATUS;

Add >

v20.1/savepoint.md, line 183 at r6 (raw file):

{% include copy-clipboard.html %}
~~~ sql
ROLLBACK TO SAVEPOINT error1;

Add >

v20.1/savepoint.md, line 196 at r6 (raw file):

{% include copy-clipboard.html %}
~~~ sql
BEGIN;

Add >

v20.1/savepoint.md, line 226 at r6 (raw file):

{% include copy-clipboard.html %}
~~~ sql
BEGIN;

Add >

v20.1/savepoint.md, line 249 at r6 (raw file):

{% include copy-clipboard.html %}
~~~ sql
BEGIN;

Add >

v20.1/savepoint.md, line 271 at r6 (raw file):

{% include copy-clipboard.html %}
~~~ sql
SHOW SAVEPOINT STATUS;

Add >

v20.1/show-savepoint-status.md, line 34 at r6 (raw file):

{% include copy-clipboard.html %}
~~~ sql
BEGIN;

Add >

v20.1/show-savepoint-status.md, line 42 at r6 (raw file):

{% include copy-clipboard.html %}
~~~ sql
SHOW SAVEPOINT STATUS;

Add >

v20.1/show-savepoint-status.md, line 58 at r6 (raw file):

{% include copy-clipboard.html %}
~~~ sql
RELEASE SAVEPOINT foo;

Add >

v20.1/sql-statements.md, line 96 at r6 (raw file):

[`RELEASE SAVEPOINT`](release-savepoint.html) | Commit a [nested transaction](transactions.html#nested-transactions).
[`ROLLBACK TO SAVEPOINT`](rollback-transaction.html#rollback-a-nested-transaction) | Roll back and restart the [nested transaction](transactions.html#nested-transactions) started at the corresponding `SAVEPOINT` statement.
[`ROLLBACK`](rollback-transaction.html) | Rolls back the current [transaction](transactions.html) and all of its [nested transaction](transactions.html#nested-transactions), discarding all transactional updates made by statements inside the transaction.

"Rolls back" > "Roll back"

cockroach-teamcity · 2020-04-16T14:33:04Z

Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/ca7b9fe784f5825e5eda37390585f1afb1f8d679/

Edited pages:

rmloveland · 2020-04-23T15:51:01Z

@jseldess if you have other tasks taking priority, I think this PR is in reasonable shape to merge already - it's had a bunch of review passes.

(Sorry to ping, just trying to get everything in by EOW for the release.)

jseldess

This looks strong from a functional accuracy perspective. However, I don't think it's clear yet why and when users should use a retry savepoint vs. a savepoint for nested transactions. We'll return to the topic of transaction and retry docs more in Q2, but for now, I'd love if we could find a way to make this clearer. The content is spread across many difference pages, so I'm not sure where it's most effective to do this. Perhaps wherever we contrast these at a high-level.

If I've missed a clear description of when/why to use each, please point me to it.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @knz, and @rmloveland)

_includes/v20.1/misc/customizing-the-savepoint-name.md, line 1 at r7 (raw file):
I would remove this parenthetical and add it to the last paragraph, e.g.:

This feature exists to support applications that need to use the advanced client-side transaction retry protocol, but cannot customize the name of savepoints to be cockroach_restart. For example, this may be necessary when using an ORM that requires its own names for savepoints.

I've changed "want" to "need" because I think saying an application "wants" something is too colloquial for docs.

v20.1/transactions.md, line 175 at r7 (raw file):

<span class="version-tag">New in v20.1:</span> CockroachDB supports the nesting of transactions using [savepoints](savepoint.html).  These nested transactions are also known as sub-transactions.

Just as [`COMMIT`][commit] and [`ROLLBACK`][rollback_transaction] are used to commit and discard entire transactions, respectively, [`RELEASE SAVEPOINT`][release_savepoint] and [`ROLLBACK TO SAVEPOINT`][rollback_to_savepoint] are used to commit and discard nested transactions.  This relationship is shown in the table below:

Rollback link is broken. I think you need to use rollback instead of rollback_transaction.

In generally, I'd prefer that we use links consistently across the docs. It's hard to have them inline in some place and footnoted in others. Inline takes less scanning in my opinion.

knz · 2020-04-23T18:27:36Z

why and when users should use a retry savepoint vs. a savepoint for nested transactions.

in all fairness to Rich, we don't know either!

We're providing nested transactions because users told us they want them, but we didn't really bother to check why they were needed.

In the blog post that I wrote about them, I concluded (and reminded the reader) that nested transactions are actually a poor abstraction and should be discouraged in new apps.

In fact, I would prefer if our doc site would not promote their use too much.

I don't think we can properly phrase any explanation that sounds like "If you have situation X, then you should use a nested transaction." There is no good situation X that fits this model.

To me the only thing we know are true are:

we want users to always implement a retry loop on the outer transaction using the restart protocol. That's the same in 20.1 as it was before.
for those users who are troubleshooting legacy code using nested transactions, we need to present reference-level docs that explain what is available in CockroachDB for whom wants to know. But we don't want to advertise it really. Especially not to new users.

rmloveland

Jesse, thanks for the review! I've made the tactical repairs from your other comments.

Re: the higher level point of where/when to use:

Jesse and Raphael, looking at the comments from both of you, would it make sense/"split the difference" between your comments to add 1-2 sentences to the "Nested transactions" section of the 'Transactions' page re: the fact that we actually don't recommend using nested transactions in new applications?

Raphael, I was thinking of adapting a condensed version of some info from your draft blog post, something like

Even though nested transactions are part of the SQL standard, supported by Postgres, and requested by many of our users, we do not recommend their use in new applications. They can easily lead to tightly coupled designs, and transactions spanning multiple components. This can lead to the following problems in modern, distributed applications:

Poor horizontal scalability due to increased coupling

Decreased performance

Increased chance of encountering transaction conflicts

Higher chance of correctness errors due to multi-level rollbacks vs. possible side effects performed by application components holding the transaction (external API calls, etc.)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @jseldess, @knz, and @lnhsingh)

_includes/v20.1/misc/customizing-the-savepoint-name.md, line 1 at r7 (raw file):

Previously, jseldess (Jesse Seldess) wrote…

I would remove this parenthetical and add it to the last paragraph, e.g.:

This feature exists to support applications that need to use the advanced client-side transaction retry protocol, but cannot customize the name of savepoints to be cockroach_restart. For example, this may be necessary when using an ORM that requires its own names for savepoints.

I've changed "want" to "need" because I think saying an application "wants" something is too colloquial for docs.

Updated!

_includes/v20.1/sql/retry-savepoints.md, line 1 at r6 (raw file):