client,kv: new savepoint API

[knz]

This patch introduces the KV savepoint API as discussed in the
savepoints RFC:

	// CreateSavepoint establishes a savepoint.
	// This method is only valid when called on RootTxns.
	CreateSavepoint(context.Context) (SavepointToken, error)

	// RollbackToSavepoint rolls back to the given savepoint. The
	// savepoint must not have been rolled back or released already.
	// All savepoints "under" the savepoint being rolled back
	// are also rolled back and their token must not be used any more.
	// This method is only valid when called on RootTxns.
	RollbackToSavepoint(context.Context, SavepointToken) error

	// ReleaseSavepoint releases the given savepoint. The savepoint
	// must not have been rolled back or released already.
	// All savepoints "under" the savepoint being released
	// are also released and their token must not be used any more.
	// This method is only valid when called on RootTxns.
	ReleaseSavepoint(context.Context, SavepointToken) error

Ths initial implementation does not (yet) enable clients to roll back over errors.

Release note: None

[cockroach-teamcity]

This change is 

[knz]

Note from Peter: extend the new roachpb test to also assert compatibility with the earlier-introduced enginepb.TxnSeqIsIgnored().

[andreimatei]

Please tell me if/when I should be looking at this.

[knz]

Yes now is a good time to look at this. I'm expecting there may be a few more iterations on this until I finish polishing the followup #43051, but I'd appreciate your early feedback until then.

[andreimatei]

This looks good to me, but we still

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

---

pkg/internal/client/sender.go, line 278 at r1 (raw file):

}

// SavepointToken represents a savepoint.

how about simply Savepoint?
https://www.youtube.com/watch?v=PEgk2v6KntY

---

pkg/internal/client/txn.go, line 1185 at r1 (raw file):

// GetSavepoint establishes a savepoint.
// This method is only valid when called on RootTxns.
func (txn *Txn) GetSavepoint(ctx context.Context) (SavepointToken, error) {

nit: I'd call it CreateSavepoint(). "Get" seems ambiguous.

---

pkg/internal/client/txn.go, line 1192 at r1 (raw file):

// RollbackToSavepoint rolls back to the given savepoint. The
// savepoint must not have been rolled back or released already.

Why not allow rolling back to a savepoint multiple times? In fact, doesn't SQL actually need to be able to do this?
Or by "rolled back" here did you mean "released implicitly by releasing a parent savepoint"?

---

pkg/internal/client/txn.go, line 1207 at r1 (raw file):

// are also released and their token must not be used any more.
// This method is only valid when called on RootTxns.
func (txn *Txn) ReleaseSavepoint(ctx context.Context, s SavepointToken) error {

This doesn't do actually do anything, does it?
I thing defensive programming here is a good idea, but then I'd orient the interface this way - CheckSavepoint or such.

---

pkg/kv/txn_coord_sender_savepoints.go, line 23 at r1 (raw file):

)

type savepointToken struct {

pls comment
And I'd also name it simply savepoint or txnSavepoint.

---

pkg/kv/txn_coord_sender_savepoints.go, line 23 at r1 (raw file):

)

type savepointToken struct {

Don't we also need the current size of txnSpanRefresher.refreshSpans the list of tracked reads, such that upon rollback we tell the refresher interceptor to discard further reads?
And also something about in-flight writes (txnPipeliner.ifWrites). There I guess we need to take some sort of snapshot of the current in-flight writes and, on rollback, discard in-flight writes that are not part of the savepoint. But, on rollback, I don't think we should (nor am I sure that we could) simply overwrite the set of in-flight writes with the ones from the savepoint because writes that have been verified since the snapshot has been taken should continue to be verified. Basically, on rollback I think we need to intersect the savepoint with the current set of in-flight writes.

---

pkg/kv/txn_coord_sender_savepoints.go, line 28 at r1 (raw file):

}

var _ client.SavepointToken = (*savepointToken)(nil)

I think the idiomatic way is

var _ client.SavepointToken = &savepointToken{}

---

pkg/kv/txn_coord_sender_savepoints.go, line 30 at r1 (raw file):

var _ client.SavepointToken = (*savepointToken)(nil)

func (s *savepointToken) SavepointToken() {}

say that this implements whatever

---

pkg/kv/txn_coord_sender_savepoints.go, line 40 at r1 (raw file):

	tc.mu.Lock()
	defer tc.mu.Unlock()
	if tc.mu.txnState != txnPending || tc.mu.closed {

Looking at this tc.mu.closed here and elsewhere bothers me. This field is supposed to be very narrow, tied to making cleanupTxnLocked() idempotent.
tc.mu.txnState seems to always be set to something before tc.cleanupTxnLocked() is called, except when handling a TransactionAbortedError where I guess we don't bother because the TCS is not supposed to be accessible to the txn after the error is returned. So I think it's fine to ignore that case (since GetSavepoint() can't be called on that TCS any more) or we could set txnState.

---

pkg/kv/txn_coord_sender_savepoints.go, line 45 at r1 (raw file):

	}
	return &savepointToken{
		seqNum: tc.interceptorAlloc.txnSeqNumAllocator.writeSeq,

and the epoch?

---

pkg/kv/txn_coord_sender_savepoints.go, line 50 at r1 (raw file):

// RollbackToSavepoint is part of the client.TxnSender interface.
func (tc *TxnCoordSender) RollbackToSavepoint(ctx context.Context, s client.SavepointToken) error {

since we're all about defense, I'd also save a transaction ID in the savepoint and compare it to the current ID. This is to protect in particular for attempts to rollback after a TransactionAbortedError.

---

pkg/kv/txn_coord_sender_savepoints.go, line 79 at r1 (raw file):

		})

	return nil

and the epoch from the savepoint? Did you mean to do something with it?

---

pkg/kv/txn_coord_sender_savepoints.go, line 91 at r1 (raw file):

	defer tc.mu.Unlock()

	_, err := tc.checkSavepointLocked(false /*allowError*/, s)

What's the thinking behind passing allowError = false here?

---

pkg/roachpb/ignored_seqnums.go, line 27 at r1 (raw file):

//    also overlaps with every subsequent range in the list.
//
// 2) the new range's "end" seqnum is larger or equal to the "end"

doesn't 2) imply 1) ?

[knz]

Thanks for the detailed review!
See my updates below.

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

---

pkg/internal/client/sender.go, line 278 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

how about simply Savepoint?
https://www.youtube.com/watch?v=PEgk2v6KntY

Meh. 'd prefer not.

---

pkg/internal/client/txn.go, line 1185 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

nit: I'd call it CreateSavepoint(). "Get" seems ambiguous.

Good idea. Done.

---

pkg/internal/client/txn.go, line 1192 at r1 (raw file):

Why not allow rolling back to a savepoint multiple times?

Because it doesn't bring us anything. We should not offer more guarantees than strictly necessary for the goals. We can relax this later if deemed both useful and necessary.

In fact, doesn't SQL actually need to be able to do this?

No.

Or by "rolled back" here did you mean "released implicitly by releasing a parent savepoint"?

No that's not what I meant.

---

pkg/internal/client/txn.go, line 1207 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

This doesn't do actually do anything, does it?
I thing defensive programming here is a good idea, but then I'd orient the interface this way - CheckSavepoint or such.

The client code to this API does not need to know that this doesn't do anything. Better have the API look and feel like a savepoint API in public docs, and keep the details of the internals to the TCS implementation.

---

pkg/kv/txn_coord_sender_savepoints.go, line 23 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

pls comment
And I'd also name it simply savepoint or txnSavepoint.

Done.

---

pkg/kv/txn_coord_sender_savepoints.go, line 23 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Don't we also need the current size of txnSpanRefresher.refreshSpans the list of tracked reads, such that upon rollback we tell the refresher interceptor to discard further reads?
And also something about in-flight writes (txnPipeliner.ifWrites). There I guess we need to take some sort of snapshot of the current in-flight writes and, on rollback, discard in-flight writes that are not part of the savepoint. But, on rollback, I don't think we should (nor am I sure that we could) simply overwrite the set of in-flight writes with the ones from the savepoint because writes that have been verified since the snapshot has been taken should continue to be verified. Basically, on rollback I think we need to intersect the savepoint with the current set of in-flight writes.

I agree with all these points. Added a TODO with your sentences. I'll address this in a followup PR where we improve what can be rolled back. I am considering the current PR as a MVP to enable the SQL work. After this merges, we can make progress on the KV and SQL aspects independently from each other.

---

pkg/kv/txn_coord_sender_savepoints.go, line 28 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I think the idiomatic way is

var _ client.SavepointToken = &savepointToken{}

That ship has sailed - both are idiomatic now. Mine also clarifies that no allocation needs to take place.

---

pkg/kv/txn_coord_sender_savepoints.go, line 30 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

say that this implements whatever

Done.

---

pkg/kv/txn_coord_sender_savepoints.go, line 40 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Looking at this tc.mu.closed here and elsewhere bothers me. This field is supposed to be very narrow, tied to making cleanupTxnLocked() idempotent.
tc.mu.txnState seems to always be set to something before tc.cleanupTxnLocked() is called, except when handling a TransactionAbortedError where I guess we don't bother because the TCS is not supposed to be accessible to the txn after the error is returned. So I think it's fine to ignore that case (since GetSavepoint() can't be called on that TCS any more) or we could set txnState.

Can you show me more precisely what the good condition could be?

---

pkg/kv/txn_coord_sender_savepoints.go, line 45 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

and the epoch?

Oops! Well spotted. Fixed and added the missing test.

---

pkg/kv/txn_coord_sender_savepoints.go, line 50 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

since we're all about defense, I'd also save a transaction ID in the savepoint and compare it to the current ID. This is to protect in particular for attempts to rollback after a TransactionAbortedError.

Good idea. Done.

---

pkg/kv/txn_coord_sender_savepoints.go, line 79 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

and the epoch from the savepoint? Did you mean to do something with it?

Not here. Added a TODO in the struct definition.

---

pkg/kv/txn_coord_sender_savepoints.go, line 91 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

What's the thinking behind passing allowError = false here?

• during a rollback, it's ok if the txn is currently in error (we'll want a rollback to "restore" a txn to a non-error state soon)
• during a release, it's not ok if the txn is currently in error - the SQL client expects the release to fail in that case

---

pkg/kv/txn_coord_sender_savepoints_test.go, line 71 at r2 (raw file):

			case "abort":
				// TODO(knz): add code missing here: simulate an abort error

@andreimatei can you show me how to simulate a client.Txn processing a txn abort error. I'd like to add it here to exercise the code.

---

pkg/roachpb/ignored_seqnums.go, line 27 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

doesn't 2) imply 1) ?

No, the start value in the new range could also be greater. Added a clarifying example.

[andreimatei]

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

---

pkg/internal/client/txn.go, line 1192 at r1 (raw file):

In fact, doesn't SQL actually need to be able to do this?

No.

How come / how will it work? The Postgres docs on ROLLBACK TO SAVEPOINT say:
"The savepoint remains valid and can be rolled back to again later, if needed."
https://www.postgresql.org/docs/current/sql-rollback-to.html

---

pkg/internal/client/txn.go, line 1207 at r1 (raw file):

Previously, knz (kena) wrote…

The client code to this API does not need to know that this doesn't do anything. Better have the API look and feel like a savepoint API in public docs, and keep the details of the internals to the TCS implementation.

Well but if you name it Release, then I wonder if I'm supposed to always call this, or when do I have to and when don't I. If you keep it named like this, you should document it. I'd much rather not...

---

pkg/kv/txn_coord_sender_savepoints.go, line 23 at r1 (raw file):

Previously, knz (kena) wrote…

I agree with all these points. Added a TODO with your sentences. I'll address this in a followup PR where we improve what can be rolled back. I am considering the current PR as a MVP to enable the SQL work. After this merges, we can make progress on the KV and SQL aspects independently from each other.

k

---

pkg/kv/txn_coord_sender_savepoints.go, line 40 at r1 (raw file):

Previously, knz (kena) wrote…

Can you show me more precisely what the good condition could be?

just tc.mu.txnState != txnPending

---

pkg/kv/txn_coord_sender_savepoints.go, line 91 at r1 (raw file):

during a rollback, it's ok if the txn is currently in error (we'll want a rollback to "restore" a txn to a non-error state soon)

But at least for now RollbackToSavepoint() returns an error immediately after the checkSavepointLocked if the txn is in the error state...

during a release, it's not ok if the txn is currently in error - the SQL client expects the release to fail in that case

But returning that error would presumably be generated in the connExecutor, where most statements are rejected when it (the SQL layer) is not in the "open" state. I don't think the TCS should worry about generating an error in this case. I think ReleaseSavepoint should stick to checking whether the savepoint has been released before.

I really think this allowError argument makes checkSavepointLocked a confusing function. (As with all bool arguments), if the logic can reasonably be lifted to the callers, it should be lifted to the callers. And in this case it definitely can. And then given what I said above, I don't think either of the two callers even need the respective logic.

---

pkg/kv/txn_coord_sender_savepoints.go, line 145 at r2 (raw file):

	if (tc.mu.txnState != txnPending &&
		(!allowError || tc.mu.txnState != txnError)) ||
		tc.mu.closed {

ditto about getting rid of tc.mu.closed

---

pkg/kv/txn_coord_sender_savepoints_test.go, line 30 at r2 (raw file):

	datadriven.Walk(t, "testdata/savepoints", func(t *testing.T, path string) {
		// New database for each test file.
		s := createTestDB(t)

Unless there's a good reason, please do me a favor and don't use this createTestDB() which creates some ancient LocalTestCluster. Use a TestServer instead like it's 2016.

---

pkg/kv/txn_coord_sender_savepoints_test.go, line 71 at r2 (raw file):

Previously, knz (kena) wrote…

@andreimatei can you show me how to simulate a client.Txn processing a txn abort error. I'd like to add it here to exercise the code.

Well, I guess you can hack it similarly to how GenerateForcedRetryableError() is implemented - it calls a ManualRestart method on the TCS that simulates the TCS having received an error. Or, you can have the transaction send a request and inject a TransactionAbortedError in the evaluation of that request using StoreTestingKnobs.TestingRequestFilter.

[knz]

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

---

pkg/internal/client/txn.go, line 1192 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

In fact, doesn't SQL actually need to be able to do this?

No.

How come / how will it work? The Postgres docs on ROLLBACK TO SAVEPOINT say:
"The savepoint remains valid and can be rolled back to again later, if needed."
https://www.postgresql.org/docs/current/sql-rollback-to.html

woah TIL. I had misunderstood the semantics.
I'll rework the code and ping you.

---

pkg/internal/client/txn.go, line 1207 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Well but if you name it Release, then I wonder if I'm supposed to always call this, or when do I have to and when don't I. If you keep it named like this, you should document it. I'd much rather not...

Good question. I'll mull over this and decide and let you know.

---

pkg/kv/txn_coord_sender_savepoints.go, line 40 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

just tc.mu.txnState != txnPending

thx will do

---

pkg/kv/txn_coord_sender_savepoints.go, line 91 at r1 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

during a rollback, it's ok if the txn is currently in error (we'll want a rollback to "restore" a txn to a non-error state soon)

But at least for now RollbackToSavepoint() returns an error immediately after the checkSavepointLocked if the txn is in the error state...

during a release, it's not ok if the txn is currently in error - the SQL client expects the release to fail in that case

But returning that error would presumably be generated in the connExecutor, where most statements are rejected when it (the SQL layer) is not in the "open" state. I don't think the TCS should worry about generating an error in this case. I think ReleaseSavepoint should stick to checking whether the savepoint has been released before.

I really think this allowError argument makes checkSavepointLocked a confusing function. (As with all bool arguments), if the logic can reasonably be lifted to the callers, it should be lifted to the callers. And in this case it definitely can. And then given what I said above, I don't think either of the two callers even need the respective logic.

now I understand your argument. I'll fix it.

---

pkg/kv/txn_coord_sender_savepoints_test.go, line 71 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Well, I guess you can hack it similarly to how GenerateForcedRetryableError() is implemented - it calls a ManualRestart method on the TCS that simulates the TCS having received an error. Or, you can have the transaction send a request and inject a TransactionAbortedError in the evaluation of that request using StoreTestingKnobs.TestingRequestFilter.

so yeah sorry I don't understand how to do that nor how does it work. Can you point me to another test that does this so I can follow an example?

[knz]

Thanks for the feedback so far. RFAL.

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

---

pkg/internal/client/txn.go, line 1192 at r1 (raw file):

Previously, knz (kena) wrote…

woah TIL. I had misunderstood the semantics.
I'll rework the code and ping you.

Done.

---

pkg/internal/client/txn.go, line 1207 at r1 (raw file):

Previously, knz (kena) wrote…

Good question. I'll mull over this and decide and let you know.

I stand by the semantics but enhanced the comment to explain.

---

pkg/kv/txn_coord_sender_savepoints.go, line 40 at r1 (raw file):

Previously, knz (kena) wrote…

thx will do

Done.

---

pkg/kv/txn_coord_sender_savepoints.go, line 91 at r1 (raw file):

Previously, knz (kena) wrote…

now I understand your argument. I'll fix it.

Done.

---

pkg/kv/txn_coord_sender_savepoints.go, line 145 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

ditto about getting rid of tc.mu.closed

Done.

---

pkg/kv/txn_coord_sender_savepoints_test.go, line 30 at r2 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

Unless there's a good reason, please do me a favor and don't use this createTestDB() which creates some ancient LocalTestCluster. Use a TestServer instead like it's 2016.

Done.

---

pkg/kv/txn_coord_sender_savepoints_test.go, line 71 at r2 (raw file):

Previously, knz (kena) wrote…

so yeah sorry I don't understand how to do that nor how does it work. Can you point me to another test that does this so I can follow an example?

I found how to do it. PTAL.

[knz]

friendly ping

[andreimatei]

LGTM

looks good

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

---

pkg/kv/txn_coord_sender_savepoints_test.go, line 40 at r3 (raw file):

		// TxnCoordSender, from storage.
		params := base.TestServerArgs{}
		doAbort := false

I think you need to access this with sync.atomic, otherwise the updates will race with random other background requests. And I think you need to find a way to make the aborting targeted to the test's txn because again you don't want to abort random background requests.

[andreimatei]

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

---

pkg/kv/txn_coord_sender_savepoints_test.go, line 40 at r3 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

I think you need to access this with sync.atomic, otherwise the updates will race with random other background requests. And I think you need to find a way to make the aborting targeted to the test's txn because again you don't want to abort random background requests.

more targeted by filter for the txn id in req.Hdr.Txn

[knz]

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

---

pkg/kv/txn_coord_sender_savepoints_test.go, line 40 at r3 (raw file):

Previously, andreimatei (Andrei Matei) wrote…

more targeted by filter for the txn id in req.Hdr.Txn

Well spotted. Done.

[knz]

TFYR!

bors r=andreimatei

[craig]

Build succeeded

• GitHub CI (Cockroach)