rfcs: SQL savepoints

[knz]

Informs #10735
Fixes #46075

Release justification: doc only change

Latest edits found at: https://github.com/knz/cockroach/blob/20191014-rfc-savepoints/docs/RFCS/20191014_savepoints.md

[cockroach-teamcity]

This change is 

[knz]

cc @andreimatei

[bdarnell]

Questions about requirements: Do the anticipated uses of nested transactions involve schema changes? Do we need to support transactions that go through many savepoint/rollback cycles (which might lead to accumulated metadata in some implementations)?

Reviewable status: complete! 0 of 0 LGTMs obtained

[knz]

That's a valid question. I fear that yes there's something to be done about that. It would be nice if someone familiar with the hibernate and sqlalchemy test suites could dig in and check what they do. -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[knz]

My previous comment pertained to schema changes under save points. For the other question I know part of the the answer already: the unit tests that use save points indeed use many SPs and rollbacks under a real txn. So functionally we should not force a limit on the number of uses. But I don't think we care about their performance, just correctness. Whether real world apps use many SPs is unknown to me. But I'd say once we implement the feature we can iterate on performance in a second phase.

…

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[knz]

FWIW I checked and pg supports DDL under savepoints and rolls them back appropriately:

kena=> begin; \
  savepoint foo; \
  create table u(x int); \
  rollback to savepoint foo; \
  create table u(w int); \
  insert into u(x) values (1); \
commit;
BEGIN
SAVEPOINT
CREATE TABLE
ROLLBACK
CREATE TABLE
ERROR:  column "x" of relation "u" does not exist
LINE 1: insert into u(x) values (1);
                      ^
ROLLBACK
kena=>

[knz]

One idea to consider that will get us the correct functional semantics at the expense of performance, is to disable descriptor caching across statements under a savepoint (multiple accesses to the same savepoint inside a single statement using a cache would still be OK since they would use the same sequence number).

[knz]

@andreimatei and I just chatted and that invalidates my idea above. DDL inside a txn installs transient state in the SQL executor, beyond what's stored in KV. That would need to be rolled back, which would be complicated.

The next action item is thus to ascertain what specific requirements we have from users. That's what @jordanlewis and @awoods187 should help with:

• do users expect to do DDL under savepoints
• do the interesting test suites do so too
• what about temp table usage under savepoints, is this a thing in the target use cases and is this a requirement for the work

[knz]

FWIW I'm kickstarting a hibernate test run now to gather all failed test outputs related to savepoints. It's going to take several hours.

[knz]

filed #41612 for the storage work

[knz]

Input from team meeting:

• @rkruze can provide real-world use cases of how savepoints are used in Django
• we can do online searches to find how active record uses nested txns, for example in https://api.rubyonrails.org/classes/ActiveRecord/Transactions/ClassMethods.html

[awoods187]

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

---

docs/RFCS/20191014_savepoints.md, line 4 at r1 (raw file):

- Status: draft
- Start Date: 2019-10-14
- Authors: andrei knz

nit ,

---

docs/RFCS/20191014_savepoints.md, line 37 at r1 (raw file):

The proposal here also incidentally addresses CockroachDB's own
Halloween problem (issue #28842) without additional work.

I couldn't see in the doc how this was the case

---

docs/RFCS/20191014_savepoints.md, line 117 at r1 (raw file):

This inserts 1 into u and 'a' into t. The table `t` with an INT column
does not exist after the transaction commits.

How does this work with our own limited support for schema changes inside transactions? As I understand it, the schema change is applied after the transaction commits. What if instead of rolling back an entire schema change you rollback some of the data written to schema changed table?

---

docs/RFCS/20191014_savepoints.md, line 199 at r1 (raw file):

	RELEASE SAVEPOINT bar; -- error: savepoint "bar" does not exist
COMMIT;

I assume this will result in no values inserted since it rollsback to foo?

---

docs/RFCS/20191014_savepoints.md, line 211 at r1 (raw file):

COMMIT;

So this would insert 1 and 2?

---

docs/RFCS/20191014_savepoints.md, line 249 at r1 (raw file):

(i.e. the second insert succeeds even though the first insert encountered an error)

so basically if you become aware that you made a mistake you can rollback your mistake and still continue with the transaction. You do not need a new transaction.

[knz]

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

---

docs/RFCS/20191014_savepoints.md, line 37 at r1 (raw file):

Previously, awoods187 (Andy Woods) wrote…

I couldn't see in the doc how this was the case

It's incidental so I'm not sure it deserves a particular explanation.

---

docs/RFCS/20191014_savepoints.md, line 117 at r1 (raw file):

Previously, awoods187 (Andy Woods) wrote…

How does this work with our own limited support for schema changes inside transactions? As I understand it, the schema change is applied after the transaction commits. What if instead of rolling back an entire schema change you rollback some of the data written to schema changed table?

That part is rolled back, and the rest remains.
Do you have an example in mind which we could use here to clarify?

---

docs/RFCS/20191014_savepoints.md, line 199 at r1 (raw file):

Previously, awoods187 (Andy Woods) wrote…

I assume this will result in no values inserted since it rollsback to foo?

Correct.

---

docs/RFCS/20191014_savepoints.md, line 211 at r1 (raw file):

Previously, awoods187 (Andy Woods) wrote…

So this would insert 1 and 2?

Yes.

---

docs/RFCS/20191014_savepoints.md, line 249 at r1 (raw file):

Previously, awoods187 (Andy Woods) wrote…

so basically if you become aware that you made a mistake you can rollback your mistake and still continue with the transaction. You do not need a new transaction.

Correct, yes. This is what ORMs and object-oriented apps do.

[knz]

I have updated the RFC based on learnings so far (current impl PR #43051).
I have removed several paragraphs that are now irrelevant.

I also have added a new section that summarizes the real work to be done (including all the items that I didn't perceive back in October): Savepoint and rollback semantics

Then I also produced a layering of the work into multiple "compatibility levels", which will enable tracking progress towards the release: Layering of work

[andreimatei]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @awoods187, @bdarnell, @irfansharif, @knz, @nvanbenschoten, and @tbg)

---

docs/RFCS/20191014_savepoints.md, line 381 at r9 (raw file):

- L4: rewind over DDL statements that don't create jobs.
- L5: rewind over DDL statements that creates jobs.
- L6: rewind over retry errors that are not aborts.

I think L6 might simply mean restoring the txn's old epoch. Perhaps it won't just work because of assumptions we might have made about the epoch not going backwards, I'm not sure. But if it just works, I would move up L6. In fact I'd generally move L6 up because without it I think savepoints will be hard to use in practice; having to handle the case in which rollbacks fail seems like a big burden on applications.
Transactions that do schema changes I personally don't care about very much.

---

docs/RFCS/20191014_savepoints.md, line 389 at r9 (raw file):

| Situation                                                                       | L1                   | L2                             | L3                                           | L4                                           | L5                         | L6                                |
|---------------------------------------------------------------------------------|----------------------|--------------------------------|----------------------------------------------|----------------------------------------------|----------------------------|-----------------------------------|
| Txn state "open", read-only so far                                              | ok, rewind KV writes | ditto L1                       | ditto L1                                     | ditto L1                                     | ditto L1                   | ditto L1                          |

the "read-only so far" doesn't seem to jibe with "rewind KV writes"

---

docs/RFCS/20191014_savepoints.md, line 408 at r9 (raw file):

reports that it is not in error (i.e. the error is SQL-only).

L3: whitelist certain KV errors and call a (new) method on client.Txn

"Certain KV errors"? Not all the non-retriable ones?
I was thinking that we wouldn't particularly look at any errors; we'd just be looking at the transaction proto to see its state.

But now I'm thinking that there's also ambiguous errors coming from a commit to worry about. Or is the plan for those to dissallow rolling back to a savepoint after attempting a COMMIT sql statement in general?

[knz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei, @awoods187, @bdarnell, @irfansharif, @nvanbenschoten, and @tbg)

---

docs/RFCS/20191014_savepoints.md, line 511 at r5 (raw file):

Previously, RaduBerinde wrote…

Thanks. Agree with your example and the overall point, not so much with the "common" characterization. I think simple inserts tend to be more common (and they allow this optimization, short of self-ref). Updates which change a foreign key column are more rare I believe (as an example, TPCC has none, you never change the warehouse or whatever after you insert a row).

Thanks point taken.

---

docs/RFCS/20191014_savepoints.md, line 780 at r6 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

It absolutely can. It's going to fail every KV request from then on, but that doesn't matter. A SQL client would detect the subsequent requests and decide (on its own, without help from TxnCoordSender) to abandon the txn

Perhaps we're discussing semantics, but not returning an error here from the leaf to the root is not an option (or at least I don't see how it would be). The current statement that's being executed needs to get an error. We can't pretend that reads were successful and didn't return any results.

I removed that section from the RFC (into the TCS tech note).

---

docs/RFCS/20191014_savepoints.md, line 974 at r6 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Currently, this is any non-retriable error.
I think the particular example you give is not the best for the reader - it's true that BatchTimestampBeforeGCError is not retriable, but one might think/argue it should be (like other "pushes"). I'd use duplicate key error or something like that.

ditto

---

docs/RFCS/20191014_savepoints.md, line 1005 at r6 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Turns out we have https://reviewable.io/reviews/cockroachdb/cockroach/35140 in flight which gets rid of this limitation.

Thanks for figuring this out.

---

docs/RFCS/20191014_savepoints.md, line 1007 at r7 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

s/two/three :)

moved to tech note

---

docs/RFCS/20191014_savepoints.md, line 1010 at r7 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I think it'd be useful in these paragraphs to be more clear about what the different "operations" are in the different contexts. Here, it's a "batch" or a "request" (or a BatchRequest :) ). In the txn restart bullet before, it's a statement (or more generally a prefix of a transaction)

ditto

---

docs/RFCS/20191014_savepoints.md, line 1037 at r7 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Pushes are also generally handled this way. Like the deferred WTOE, they're not errors - they're just updates to the transaction's timestamp field, which get turned to retriable errors at EndTransaction time.

And as per our side conversations, it turns out that currently WTOE are actually never deferred (and I want to make sure they stay that way).

Moved to TCS t ech note.

---

docs/RFCS/20191014_savepoints.md, line 736 at r8 (raw file):

What error other than a TransactionAbortedError cannot be recovered by a savepoint rollback? It seems to me that everything can be recovered from. The savepoint is supposed to perfectly capture the state of the transaction at the point where it was created, isn't it?

• assertion errors presumably leaves txns in an unknown state and the conservative decision is to let the txn abort with no further operations.
• I would recommend that storage errors (corruptions etc) don't let apps paper over them with a rollback

---

docs/RFCS/20191014_savepoints.md, line 381 at r9 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I think L6 might simply mean restoring the txn's old epoch. Perhaps it won't just work because of assumptions we might have made about the epoch not going backwards, I'm not sure. But if it just works, I would move up L6. In fact I'd generally move L6 up because without it I think savepoints will be hard to use in practice; having to handle the case in which rollbacks fail seems like a big burden on applications.
Transactions that do schema changes I personally don't care about very much.

Good point. Added a sentence to clarify here and below.

---

docs/RFCS/20191014_savepoints.md, line 389 at r9 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

the "read-only so far" doesn't seem to jibe with "rewind KV writes"

Woops, fixed.

---

docs/RFCS/20191014_savepoints.md, line 408 at r9 (raw file):

"Certain KV errors"? Not all the non-retriable ones?

Added a sentence to clarify.

But now I'm thinking that there's also ambiguous errors coming from a commit to worry about. Or is the plan for those to dissallow rolling back to a savepoint after attempting a COMMIT sql statement in general?

That's nothing to worry about. After a client issues the "COMMIT" statement, it's not valid to issue anything related to the SQL txn any more, including a rollback.

You may be thinking about ambiguous errors coming from a RELEASE SAVEPOINT cockroach_restart, which performs a KV commit but leaves the SQL txn open. Today we abort the SQL txn in that case and it's not recoverable. In this RFC this remains the case because we do not allow cockroach_restart to be nested under a regular savepoint.

[knz]

I have made final adjustments to the RFC to reflect the actual implementation.

The main change is the exclusion of DDL rollbacks in the guide-level explanation.
However I also added a more complete note about:

1. the interaction between regular savepoints and restart savepoints. @rmloveland this is for you (see section "Relationship with client-side retries")

2. I added a section on row locks, after the discussion on sql: ROLLBACK TO SAVEPOINT does not release locks #46075

I plan to merge this PR soon and thereby close the RFC discussion. Please phrase remaining comments/concerns ASAP.

[knz]

@andreimatei please complete the review of the RFC and let me know if/when this can merge.

[knz]

updated the text as suggested. RFAL

[andreimatei]

LGTM

…

On Wed, Mar 18, 2020 at 2:18 PM kena ***@***.***> wrote: updated the text as suggested. RFAL — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#41569 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAC4C4I5LJAGCWYKM5I5VSDRIEF6LANCNFSM4JARAAEQ> .

andrei says "LGTM" in comment

[knz]

bors r+

[craig]

Build failed

• GitHub CI (Cockroach)

[knz]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)